Date:   Mon, 25 Apr 2022 09:51:10 -0700
From:   Ira Weiny <ira.weiny@...el.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Catalin Marinas <catalin.marinas@....com>,
        "Matthew Wilcox (Oracle)" <willy@...radead.org>,
        Will Deacon <will@...nel.org>,
        Peter Collingbourne <pcc@...gle.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        linux-kernel@...r.kernel.org, Jonathan Corbet <corbet@....net>,
        linux-doc@...r.kernel.org, outreachy@...ts.linux.dev,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH 4/4] Documentation/vm: Rework "Temporary Virtual Mappings"

On Mon, Apr 25, 2022 at 03:42:46AM +0200, Fabio M. De Francesco wrote:
> On lunedì 25 aprile 2022 02:59:48 CEST Ira Weiny wrote:
> > On Thu, Apr 21, 2022 at 08:02:00PM +0200, Fabio M. De Francesco wrote:
> > > Extend and rework the "Temporary Virtual Mappings" section of the 
> highmem.rst
> > > documentation.
> > > 
> > > Despite the local kmaps were introduced by Thomas Gleixner in October 
> 2020,
> > > documentation was still missing information about them. These additions 
> rely
> > > largely on Gleixner's patches, Jonathan Corbet's LWN articles, comments 
> by
> > > Ira Weiny and Matthew Wilcox, and in-code comments from
> > > ./include/linux/highmem.h.
> > > 
> > > 1) Add a paragraph to document kmap_local_page().
> > > 2) Reorder the list of functions by decreasing order of preference of
> > > use.
> > > 3) Rework part of the kmap() entry in list.
> > > 
> > > Cc: Jonathan Corbet <corbet@....net>
> > > Cc: Thomas Gleixner <tglx@...utronix.de>
> > > Cc: Ira Weiny <ira.weiny@...el.com>
> > > Cc: Matthew Wilcox <willy@...radead.org>
> > > Cc: Peter Zijlstra <peterz@...radead.org>
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> > > ---
> > >  Documentation/vm/highmem.rst | 71 ++++++++++++++++++++++++++++++------
> > >  1 file changed, 60 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/
> highmem.rst
> > > index e05bf5524174..960f61e7a552 100644
> > > --- a/Documentation/vm/highmem.rst
> > > +++ b/Documentation/vm/highmem.rst
> > > @@ -50,26 +50,75 @@ space when they use mm context tags.
> > >  Temporary Virtual Mappings
> > >  ==========================
> > >  
> > > -The kernel contains several ways of creating temporary mappings:
> > > +The kernel contains several ways of creating temporary mappings. The 
> following
> > > +list shows them in order of preference of use.
> > >  
> > > -* vmap().  This can be used to make a long duration mapping of 
> multiple
> > > -  physical pages into a contiguous virtual space.  It needs global
> > > -  synchronization to unmap.
> > > +* kmap_local_page().  This function is used to require short term 
> mappings.
> > > +  It can be invoked from any context (including interrupts) but the 
> mappings
> > > +  can only be used in the context which acquired them.
> > > +
> > > +  This function should be preferred, where feasible, over all the 
> others.
> > >  
> > > -* kmap().  This permits a short duration mapping of a single page.  It 
> needs
> > > -  global synchronization, but is amortized somewhat.  It is also prone 
> to
> > > -  deadlocks when using in a nested fashion, and so it is not 
> recommended for
> > > -  new code.
> > > +  These mappings are per thread, CPU local (i.e., migration from one 
> CPU to
> > > +  another is disabled - this is why they are called "local"), but they 
> don't
> > > +  disable preemption. It's valid to take pagefaults in a local kmap 
> region,
> > > +  unless the context in which the local mapping is acquired does not 
> allow
> > > +  it for other reasons.
> > > +
> > > +  It is assumed that kmap_local_page() always returns the virtual 
> address
> > 
> > kmap_local_page() does return a kernel virtual address.  Why 'assume' 
> this?
> > 
> > Do you mean it returns an address in the direct map?
> > 
> > > +  of the mapping, therefore they won't ever fail.
> > 
> > I don't think that returning a virtual address has anything to do with 
> the
> > assumption they will not fail.
> > 
> > Why do you say this?
> 
> Oh, sorry! I didn't mean to say this. What I wrote is _not_ what I meant. 
> My intention was to say the same that you may read below about 
> k[un]map_atomic().
> 
> This sentence should have been:
> 
> + It always returns a valid virtual address. It is assumed that
> + k[un]map_local() won't ever fail.
> 
> Is this rewording correct?
> 
> It's not my first time I make these kinds of silly mistakes when copy-
> pasting lines and then rework or merge with other text that was already 
> there. Recently I've made a couple of these kinds of mistakes.
> 
> I'd better read twice (maybe thrice) what I write before sending :(

NP  That is part of the reason we have reviews.

> 
> > 
> > > +
> > > +  If a task holding local kmaps is preempted, the maps are removed on 
> context
> > > +  switch and restored when the task comes back on the CPU. As the maps 
> are
> > > +  strictly CPU local, it is guaranteed that the task stays on the CPU 
> and
> > > +  that the CPU cannot be unplugged until the local kmaps are released.
> > > +
> > > +  Nesting kmap_local_page() and kmap_atomic() mappings is allowed to a 
> certain
> > > +  extent (up to KMAP_TYPE_NR) but their invocations have to be 
> strictly ordered
> > > +  because the map implementation is stack based.
> > 
> > I think I would reference the kmap_local_page()
> 
> I suppose you are talking about the kdocs comments in code. If so, please 
> remember that the kmap_local_page() kdocs have already been included in  
> highmem.rst.

Yes exactly.

> 
> Am I misunderstanding what you write?

I was just suggesting that the above could add.

'See kmal_local_page() kdoc for ordering details.'

To make sure that people understand those details and you don't have to rewrite
the kdoc stuff here.

> 
> > for more details on the
> > ordering because there have been some conversions I've done which were
> > complicated by this.
> > 
> > >  
> > >  * kmap_atomic().  This permits a very short duration mapping of a 
> single
> > >    page.  Since the mapping is restricted to the CPU that issued it, it
> > >    performs well, but the issuing task is therefore required to stay on 
> that
> > >    CPU until it has finished, lest some other task displace its 
> mappings.
> > >  
> > > -  kmap_atomic() may also be used by interrupt contexts, since it is 
> does not
> > > -  sleep and the caller may not sleep until after kunmap_atomic() is 
> called.
> > > +  kmap_atomic() may also be used by interrupt contexts, since it does 
> not
> > > +  sleep and the callers too may not sleep until after kunmap_atomic() 
> is
> > > +  called.
> > > +
> > > +  Each call of kmap_atomic() in the kernel creates a non-preemptible 
> section
> > > +  and disable pagefaults. This could be a source of unwanted latency, 
> so it
> > > +  should be only used if it is absolutely required, otherwise 
> kmap_local_page()
> > > +  should be used where it is feasible.
> > >  
> > > -  It may be assumed that k[un]map_atomic() won't fail.
> > > +  It is assumed that k[un]map_atomic() won't fail.
> > > +
> > > +* kmap().  This should be used to make short duration mapping of a 
> single
> > > +  page with no restrictions on preemption or migration. It comes with 
> an
> > > +  overhead as mapping space is restricted and protected by a global 
> lock
> > > +  for synchronization. When mapping is no more needed, the address 
> that
> >                                          ^^^^^^^^
> > 					 no longer
> 
> Yes, correct. I'll fix it.
> 
> > > +  the page was mapped to must be released with kunmap().
> > > +
> > > +  Mapping changes must be propagated across all the CPUs. kmap() also
> > > +  requires global TLB invalidation when the kmap's pool wraps and it 
> might
> > > +  block when the mapping space is fully utilized until a slot becomes
> > > +  available. Therefore, kmap() is only callable from preemptible 
> context.
> > > +
> > > +  All the above work is necessary if a mapping must last for a 
> relatively
> > > +  long time but the bulk of high-memory mappings in the kernel are
> > > +  short-lived and only used in one place. This means that the cost of
> > > +  kmap() is mostly wasted in such cases. kmap() was not intended for 
> long
> > > +  term mappings but it has morphed in that direction and its use is
> > > +  strongly discouraged in newer code and the set of the preceding 
> functions
> > > +  should be preferred.
> > 
> > Nice!
> 
> Now that I have your reviews for all the four patches of this series I'll 
> send next version on Monday.
> 
> Thanks you so much,

Thank you!
Ira

> 
> Fabio
> 
> > 
> > Ira
> > 
> > > +
> > > +  On 64-bit systems, calls to kmap_local_page(), kmap_atomic() and 
> kmap() have
> > > +  no real work to do because a 64-bit address space is more than 
> sufficient to
> > > +  address all the physical memory whose pages are permanently mapped.
> > > +
> > > +* vmap().  This can be used to make a long duration mapping of 
> multiple
> > > +  physical pages into a contiguous virtual space.  It needs global
> > > +  synchronization to unmap.
> > >  
> > >  
> > >  Cost of Temporary Mappings
> > > -- 
> > > 2.34.1
> > > 
> > 
> 
> 
> 
> 

Powered by blists - more mailing lists