Date:   Thu, 28 Apr 2022 13:14:30 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     Ira Weiny <ira.weiny@...el.com>,
        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>, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org, outreachy@...ts.linux.dev,
        Jonathan Corbet <corbet@....net>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v3 4/4] Documentation/vm: Rework "Temporary Virtual Mappings" section

On giovedì 28 aprile 2022 11:02:10 CEST Sebastian Andrzej Siewior wrote:
> On 2022-04-27 20:38:21 [+0200], Fabio M. De Francesco wrote:
> > index e05bf5524174..c8aff448612b 100644
> > --- a/Documentation/vm/highmem.rst
> > +++ b/Documentation/vm/highmem.rst
> > @@ -50,26 +50,78 @@ space when they use mm context tags.
> …
> >  
> > -* 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 thread-local and 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. 
> 
> So if you replace this block with
> 
>    These mappings are thread-local and CPU-local meaning that the mapping
>    can only be accessed from within this thread and the thread is bound 
the
>    CPU while the mapping is active. Even if the thread is preempted 
(since
>    preemption is never disabled by the function) the CPU can not be
>    unplugged from the system via CPU-hotplug until the mapping is 
disposed.

OK, I'm too wordy here :(

> The you could drop the latter block
> 
> >                          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.
> 
> > +  kmap_local_page() always returns a valid virtual address and it is 
assumed
> > +  that kunmap_local() will never fail.
> 
> from here
> 
> > +  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. The maps 
are
> > +  strictly thread-local and CPU-local, therefore it is guaranteed that 
the
> > +  task stays on the CPU and the CPU cannot be unplugged until the 
local kmaps
> > +  are released.
> 
> to here since it mostly the same thing.

I agree, this is redundant.

> 
> > +  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. See kmap_local_page 
() kdocs
> 
> kmap_local_page () => kmap_local_page()

Sure, it's just a typo.

> > +  (included in the "Functions" section) for details on how to manage 
nested
> > +  mappings.
> >  
> >  * 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.
> 
> I'm not keen about the "absolutely required" wording and "feasible".
> That said, the other pieces look good, thank you for the work.

I'll rewrite the last part of this sentence as it follows:

+ should be only used if it is required, otherwise kmap_local_page()
+ should be preferred.

Thank you so much for the time you have spent for reviewing and helping,

Fabio

Powered by blists - more mailing lists