Re: [PATCH 3/7] Documentation/mm: Don't kmap*() pages which can't come from HIGHMEM

[Fabio M. De Francesco]

On giovedì 21 luglio 2022 23:13:00 CEST Jonathan Corbet wrote:
> "Fabio M. De Francesco" <fmdefrancesco@xxxxxxxxx> writes:
> 
> > There is no need to kmap*() pages which are guaranteed to come from
> > ZONE_NORMAL (or lower). Linux has currently several call sites of
> > kmap{,_atomic,_local_page}() on pages allocated, for instance, with
> > alloc_page(GFP_NOFS) and other similar allocations.
> >
> > Therefore, add a paragraph to highmem.rst, to explain better that a
> > plain page_address() should be used for getting the address of pages
> > which cannot come from ZONE_HIGHMEM.
> >
> > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
> > Cc: Mike Rapoport <rppt@xxxxxxxxxxxxx>
> > Cc: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Suggested-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>
> > ---
> >  Documentation/vm/highmem.rst | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/
highmem.rst
> > index c9887f241c6c..f266354c82ab 100644
> > --- a/Documentation/vm/highmem.rst
> > +++ b/Documentation/vm/highmem.rst
> > @@ -71,6 +71,12 @@ list shows them in order of preference of use.
> >    kmap_local_page() always returns a valid virtual address and it is 
assumed
> >    that kunmap_local() will never fail.
> >  
> > +  On CONFIG_HIGHMEM=n kernels and for low memory pages this returns 
the
> > +  virtual address of the direct mapping. Only real highmem pages are
> > +  temporarily mapped. Therefore, users should instead call a plain
> > +  page_address() for getting the address of memory pages which, 
depending
> > +  on the GFP_* flags, cannot come from ZONE_HIGHMEM.
> > +
> 
> Is this good advice?

Well... yes and no :-) 

However yours is a legit objection. 

I'm taking most of the suggestion from Ira (from an email in this same 
thread) and send the v2 of this series.

My intention was to avoid things like those I encountered when converting 
fs/btrfs:

page = alloc_page(GFP_NOFS);
kaddr = kmap(page);

Why one should kmap*() pages allocated one or two lines above with 
GFP_NOFS? 

Furthermore, since nesting kmap_local_page() / kunmap_local() is last in 
first out (LIFO), I had several problems with several un-mappings until 
David Sterba made me notice that GFP_NOFS is not OR'ed with __GFP_HIGHMEM 
and suggested to use plain page_address() instead of those unnecessary 
mappings.

However, you are right about the fact that, with most of other allocations, 
it is not so clear where and how pages are being allocated.

Thanks,

Fabio 

> First, it requires developers to worry about
> whether their pages might be in highmem, which is kind of like worrying
> about having coins in your pocket in case you need a payphone.  But it
> would also run afoul of other semantics for kmap*(), such as PKS, should
> that ever be merged:
> 
>   https://lwn.net/Articles/894531/
>
> Thanks,
> 
> jon
>