[PATCH] Documentation/vm: Extend "Temporary Virtual Mappings" in highmem.rst

[PATCH] Documentation/vm: Extend "Temporary Virtual Mappings" in highmem.rst

[Fabio M. De Francesco]

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.
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 and in-code
comments from ./include/linux/highmem.h.

Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 Documentation/vm/highmem.rst | 68 ++++++++++++++++++++++++++++--------
 1 file changed, 54 insertions(+), 14 deletions(-)

diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/highmem.rst
index 0f69a9fec34d..d9ec26d921c8 100644
--- a/Documentation/vm/highmem.rst
+++ b/Documentation/vm/highmem.rst
@@ -52,25 +52,65 @@ 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.
+* 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().  This can be used to make long 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, page must be released with kunmap().
 
-* 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.
+  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.
 
-  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.
+  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; therefore,
+  newer code is discouraged from using kmap().
 
-  It may be assumed that k[un]map_atomic() won't fail.
+* 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 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 (see below).
+
+  On 64-bit systems, calls to kmap() and kmap_atomic() have no real work to do
+  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.
+
+* kmap_local_*().  These provide a set of functions similar to kmap_atomic() and
+  are used to require short term mappings. They can be invoked from any context
+  (including interrupts).
+
+  The mapping can only be used in the context which acquired it, it is 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
+  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.
 
 Using kmap_atomic
 =================
-- 
2.34.1

Re: [PATCH] Documentation/vm: Extend "Temporary Virtual Mappings" in highmem.rst

[Matthew Wilcox]

On Sat, Apr 09, 2022 at 08:49:07PM +0200, Fabio M. De Francesco wrote:
> @@ -52,25 +52,65 @@ 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.
> +* 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.

Did you change any words here?  If so, I can't see them.  Please don't
gratuitously reformat paragraphs; it obscures the real changes.  Also,
75 characters is a good limit for line length, and you're well past
that.  If in doubt, use `fmt`.

> -* 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().  This can be used to make long duration mapping of a single page with

kmap() really isn't for long duration.  But the pointer returned from
kmap() is valid across all CPUs, unlike kmap_local() or kmap_atomic().

> +  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, page must be released with kunmap().

kunmap() doesn't release the page, it releases the address that the
page was mapped to.

> +  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; therefore,
> +  newer code is discouraged from using kmap().
>  
> -* 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().  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 is 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 (see below).
> +
> +  On 64-bit systems, calls to kmap() and kmap_atomic() have no real work to do
> +  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 may be assumed that k[un]map_atomic() won't fail.
> +  It is assumed that k[un]map_atomic() won't fail.
> +
> +* kmap_local_*().  These provide a set of functions similar to kmap_atomic() and
> +  are used to require short term mappings. They can be invoked from any context
> +  (including interrupts).
> +
> +  The mapping can only be used in the context which acquired it, it is 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
> +  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.

I think the original layout of all this is flawed.  We should start by
describing the interface we want people to use first -- kmap_local*(),
then say "But if you can't use that, there's kmap_atomic()" and "If
you can't use kmap_atomic(), you can use kmap()".  And vmap() should
go right at the end because it's entirely different from the kmap()
family.  I even question if it should be in this document, but it's not
really documented anywhere else.

Also, you should probably cc the person who is listed as the author
of the document, don't you think?

Re: [PATCH] Documentation/vm: Extend "Temporary Virtual Mappings" in highmem.rst

[Fabio M. De Francesco]

Hi Matthew, 

Thank you for this review.

On domenica 10 aprile 2022 05:21:07 CEST Matthew Wilcox wrote:
> On Sat, Apr 09, 2022 at 08:49:07PM +0200, Fabio M. De Francesco wrote:
> > @@ -52,25 +52,65 @@ 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.
> > +* 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.
> 
> Did you change any words here?  If so, I can't see them.  Please don't
> gratuitously reformat paragraphs; it obscures the real changes.  Also,
> 75 characters is a good limit for line length, and you're well past
> that.  If in doubt, use `fmt`.

No, I did not change any words. I'll restore it back as it was so that 
the real changes won't be obscured.
 
> > -* 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().  This can be used to make long duration mapping of a single page with
> 
> kmap() really isn't for long duration.  But the pointer returned from
> kmap() is valid across all CPUs, unlike kmap_local() or kmap_atomic().

I think that I used the wrong words because "long duration", in my mind, 
should have meant that there are no restrictions on preemption or migrations 
across CPUs (differently from the other functions for mappings). I decided 
to express these concepts as "This can be used to make long duration 
mappings []".

Now I understand that this is not technically accurate and that it is not 
what the Author wanted to convey. Therefore I'll restore it to how it was 
before my changes.
 
> > +  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, page must be released with kunmap().
> 
> kunmap() doesn't release the page, it releases the address that the
> page was mapped to.

It's just a mistake in expressing the correct semantics which I think I know 
properly. I should have probably read it once more and notice this mistake 
before sending my patch. 

I'll use the proper wording in v2.

> > +  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; therefore,
> > +  newer code is discouraged from using kmap().
> >  
> > -* 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().  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 is 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 (see below).
> > +
> > +  On 64-bit systems, calls to kmap() and kmap_atomic() have no real work to do
> > +  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 may be assumed that k[un]map_atomic() won't fail.
> > +  It is assumed that k[un]map_atomic() won't fail.
> > +
> > +* kmap_local_*().  These provide a set of functions similar to kmap_atomic() and
> > +  are used to require short term mappings. They can be invoked from any context
> > +  (including interrupts).
> > +
> > +  The mapping can only be used in the context which acquired it, it is 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
> > +  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.
> 
> I think the original layout of all this is flawed.  We should start by
> describing the interface we want people to use first -- kmap_local*(),
> then say "But if you can't use that, there's kmap_atomic()" and "If
> you can't use kmap_atomic(), you can use kmap()".  And vmap() should
> go right at the end because it's entirely different from the kmap()
> family.  I even question if it should be in this document, but it's not
> really documented anywhere else.
 
I agree in full. We should start by describing kmap_local_*(). Actually I 
had noticed that the layout is flawed when I had to write: "(see below)". 
Making a reference to what comes after looks very strange to my eyes. 

I'll rework the layout in v2. 

> Also, you should probably cc the person who is listed as the author
> of the document, don't you think?

Oh, yes, I'm sorry. I don't know how I could have overlooked Peter Zijlstra. 
It won't happen again with my v2 patch (I have already noticed that you added
him in Cc with your email).

Again, many thanks for taking the time to write this review,

Fabio M. De Francesco

Re: [PATCH] Documentation/vm: Extend "Temporary Virtual Mappings" in highmem.rst

[Ira Weiny]

On Sun, Apr 10, 2022 at 04:21:07AM +0100, Matthew Wilcox wrote:
> On Sat, Apr 09, 2022 at 08:49:07PM +0200, Fabio M. De Francesco wrote:
> > @@ -52,25 +52,65 @@ 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.
> > +* 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.
> 
> Did you change any words here?  If so, I can't see them.  Please don't
> gratuitously reformat paragraphs; it obscures the real changes.  Also,
> 75 characters is a good limit for line length, and you're well past
> that.  If in doubt, use `fmt`.
> 
> > -* 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().  This can be used to make long duration mapping of a single page with
> 
> kmap() really isn't for long duration.  But the pointer returned from
> kmap() is valid across all CPUs, unlike kmap_local() or kmap_atomic().

I think the problem is in how kmap() is being used now for the long duration
maps in a couple of places.

That said, I agree with Matt that we should not document this fact.  Rather we
should steer people away from making use of kmap() at all.

That said if kmap() goes away what replaces it in the areas of the code which
require a long term access to a VA?

The addition of PKS like protections on the direct map complicate this.

[snip]

> > +
> > +* kmap_local_*().  These provide a set of functions similar to kmap_atomic() and
> > +  are used to require short term mappings. They can be invoked from any context
> > +  (including interrupts).
> > +
> > +  The mapping can only be used in the context which acquired it, it is 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
> > +  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.
> 
> I think the original layout of all this is flawed.  We should start by
> describing the interface we want people to use first -- kmap_local*(),
> then say "But if you can't use that, there's kmap_atomic()" and "If
> you can't use kmap_atomic(), you can use kmap()".

...  If, and only if, one absolutely has to use kmap(), then ok...

Ira