Message-ID: <2455960.4XsnlVU6TS@leap>
Date:   Tue, 19 Apr 2022 16:52:11 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Ira Weiny <ira.weiny@...el.com>
Cc:     Jonathan Corbet <corbet@....net>,
        Andrew Morton <akpm@...ux-foundation.org>,
        SeongJae Park <sj@...nel.org>,
        Jiajian Ye <yejiajian2018@...il.szu.edu.cn>,
        Thomas Gleixner <tglx@...utronix.de>,
        Matthew Wilcox <willy@...radead.org>,
        Peter Zijlstra <peterz@...radead.org>,
        outreachy@...ts.linux.dev, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] Documentation/vm: Extend "Temporary Virtual Mappings" section

On lunedì 18 aprile 2022 23:30:12 CEST Ira Weiny wrote:
> On Sat, Apr 16, 2022 at 01:19:16AM +0200, Fabio M. De Francesco wrote:
> > Extend and rework the "Temporary Virtual Mappings" section of the 
highmem.rst
> > documentation.
> > 
> > Do a partial rework of the paragraph related to kmap() and add a new 
paragraph
> > in order to document the set of kmap_local_*() functions. Re-order 
paragraphs
> > in decreasing order of preference of usage.
> > 
> > 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.
> > 
> > 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 | 67 ++++++++++++++++++++++++++++++------
> >  1 file changed, 56 insertions(+), 11 deletions(-)
> > 
> > diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/
highmem.rst
> > index 0f69a9fec34d..12dcfbee094d 100644
> > --- a/Documentation/vm/highmem.rst
> > +++ b/Documentation/vm/highmem.rst
> > @@ -52,24 +52,69 @@ Temporary Virtual Mappings
> >  
> >  The kernel contains several ways of creating temporary mappings:
> >  
> > -* 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().  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.
> > +* kmap_local_*().  These provide a set of functions that are used to 
require
> > +  short term mappings. They can be invoked from any context (including
> > +  interrupts) but the mappings can only be used in the context which 
acquired
> > +  it.
> > +
> > +  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.
> > +
> > +  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.*() and kmap_atomic.*() mappings is allowed to a 
certain
> 
> NIT: Throughout this doc why say 'kmap_local.*'?  There is only one call 
named
> kmap_local_page().

Let me explain why throughout this text I used "kmap_local.*()"... As I 
wrote, I relied largely also on Thomas Gleixner's series. He used that way 
of referring to local kmaps.

I think that this was probably due to his introduction of several local 
kmaps related functions: kmap_local_page(), kmap_local_page_prot(), 
kmap_local_pfn(). 

However, I understand your "NIT" and I'll use a plain kmap_local_page() in 
the next version of this patch /series. After all, this paragraph is only 
talking about kmap_local_page().

> 
> > +  extent (up to KMAP_TYPE_NR). Nested kmap_local.*() and 
kunmap_local.*()
> > +  invocations have to be strictly ordered because the map 
implementation is
> > +  stack based.
> 
> This type of documentation should (and I believe is, in the kdoc of
> kmap_local_page().  Why not just add this text (or enhance what is there) 
and
> include that here?
> 
> Ah I see that patch 2/2 does add the kdocs for the functions... ah ok  
:-/
> 
> Perhaps this section should focus on why to use each of the kmap calls 
and not
> how?  Leave the how to the kdoc?  Although all this information would be 
nice
> inside the header for programmers who are looking at using these 
functions.

As I wrote in another email I sent yesterday, I agree with you.

In the next version, highmem.rst file will only have a little introduction 
about why we need to differentiate High Memory (Highmem) and the 
permanently mapped Low Memory (Normal), a brief description of the set 
functions we have to map Highmem, why we have them and which of them 
developers should avoid to use in new code.

Most of the information, especially how to use them, will be moved to 
highmem.h and highmem-internal.h.

Of course highmem.rst will automatically include this information as the 
kdoc directives are processed.

> Here is an example of how I dealt with this on a recent auxiliary bus
> documentation update:
> 
> https://lore.kernel.org/lkml/20211202044305.4006853-8-ira.weiny@intel.com/

Thanks for this link. I have already skimmed through it. I think I 
understand the overall design. I'll rework this series taking into account 
what I read in your patch.

> 
> >  
> >  * 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 caller too can 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 the corresponding kmap_local_*() 
variant should
> > +  be used if it is feasible.
> > +
> > +  On 64-bit systems, calls to kmap() and kmap_atomic() have no real 
work to do
>                                  ^^^^^^^^^^^^^^^^^^^^^^^^
> What about kmap_local_page()?

Yes, correct. I overlooked it.

> > +  because a 64-bit address space is more than sufficient to address 
all the
> > +  physical memory, so all of physical memory appears in the direct 
mapping.
> > +
> > +  It is assumed that k[un]map_atomic() won't fail.
> >  
> > -  It may be 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
> > +  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.
> 
> I like this paragraph especially the last sentence!  Nicely put!

Thanks!

> But this is another reason I think this text in the code next to the kmap
> calls.  Then it is more readily available to programmers who are looking 
at the
> functions.

As I wrote above, I will be reworking this series with your suggestions in 
mind, but probably (due to other commitments) I will not be able to submit 
the next version before the weekend.

> Thanks for the great work so far!
> Ira

Thanks to you for this review and all the suggestions you have provided me,

Fabio


Fabio

Powered by blists - more mailing lists