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

[Ira Weiny]

On Thu, Jul 21, 2022 at 03:13:00PM -0600, 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?  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.

This is a good point.  Perhaps this is better worded as:

	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 may call a plain page_address()
	for pages which are known to not come from ZONE_HIGHMEM.

	However, it is always safe to use kmap_local_page()/kunmap_local() and
	access through those calls will be as efficient as page_address() on
	most architectures.

> But it
> would also run afoul of other semantics for kmap*(), such as PKS, should
> that ever be merged:
> 
>   https://lwn.net/Articles/894531/

PKS is yet to be merged.  As of now, there is no good reason to force users to
use the kmap_local_page() if the page zone is known.

I believe that beyond PKS there will come a time when we need to change the
page_address() callers but currently this documentation is correct and does
allow callers to optimize for the corner case of a HIGHMEM system if they
desire.

As a reference, the use of kmap* vs page_address() was discussed recently in a
couple of places[1][2] and I can't fault the logic there at this time.

Thanks for the review,
Ira

[1] https://lore.kernel.org/linux-btrfs/20220621131521.GW20633@xxxxxxxxxxxxx/
[2] https://lore.kernel.org/lkml/CANn89iK6g+4Fy2VMV7=feUAOUDHu-J38be+oU76yp+zGH6xCJQ@xxxxxxxxxxxxxx/

> 
> Thanks,
> 
> jon