* [PATCH v3 0/2] Documentation/vm: Rework and extend highmem.rst
@ 2022-04-15 23:19 Fabio M. De Francesco
2022-04-15 23:19 ` [PATCH v3 1/2] Documentation/vm: Extend "Temporary Virtual Mappings" section Fabio M. De Francesco
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Fabio M. De Francesco @ 2022-04-15 23:19 UTC (permalink / raw)
To: Jonathan Corbet, Andrew Morton, SeongJae Park, Jiajian Ye,
Thomas Gleixner, Ira Weiny, Matthew Wilcox, Peter Zijlstra,
outreachy, linux-doc, linux-kernel
Cc: 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. Re-order paragraphs
in decreasing order of preference of usage.
Include kerneldoc comments from include/linux/highmem.h and remove
redundant and obsolete section about kmap_atomic().
v1-v2:
1/2 - According to comments on v1 by Matthew Wilcox and Ira Weiny,
correct a mistake in text, format paragraphs to stay within
the 75 characters limit, re-order the flow of the same
paragraphs in decreasing order of preference of usage.
2/2 - No patch.
v2->v3:
1/2 - No changes.
2/2 - Added patch to include kernel-doc to highmem.rst and
remove the now redundant section about kmap_atomic().
Fabio M. De Francesco (2):
Documentation/vm: Extend "Temporary Virtual Mappings" section in
highmem.rst
Documentation/vm: Include kernel-doc to highmem.rst
Documentation/vm/highmem.rst | 96 +++++++++++++++++++++---------------
1 file changed, 56 insertions(+), 40 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 1/2] Documentation/vm: Extend "Temporary Virtual Mappings" section
2022-04-15 23:19 [PATCH v3 0/2] Documentation/vm: Rework and extend highmem.rst Fabio M. De Francesco
@ 2022-04-15 23:19 ` Fabio M. De Francesco
2022-04-18 21:30 ` Ira Weiny
2022-04-15 23:19 ` [PATCH v3 2/2] Documentation/vm: Include kernel-doc to highmem.rst Fabio M. De Francesco
2022-04-21 17:51 ` [PATCH v3 0/2] Documentation/vm: Rework and extend highmem.rst Fabio M. De Francesco
2 siblings, 1 reply; 9+ messages in thread
From: Fabio M. De Francesco @ 2022-04-15 23:19 UTC (permalink / raw)
To: Jonathan Corbet, Andrew Morton, SeongJae Park, Jiajian Ye,
Thomas Gleixner, Ira Weiny, Matthew Wilcox, Peter Zijlstra,
outreachy, linux-doc, linux-kernel
Cc: 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. 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@lwn.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.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
+ 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.
* 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
+ 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.
+
+* 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.
Using kmap_atomic
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 2/2] Documentation/vm: Include kernel-doc to highmem.rst
2022-04-15 23:19 [PATCH v3 0/2] Documentation/vm: Rework and extend highmem.rst Fabio M. De Francesco
2022-04-15 23:19 ` [PATCH v3 1/2] Documentation/vm: Extend "Temporary Virtual Mappings" section Fabio M. De Francesco
@ 2022-04-15 23:19 ` Fabio M. De Francesco
2022-04-18 21:32 ` Ira Weiny
2022-04-21 17:51 ` [PATCH v3 0/2] Documentation/vm: Rework and extend highmem.rst Fabio M. De Francesco
2 siblings, 1 reply; 9+ messages in thread
From: Fabio M. De Francesco @ 2022-04-15 23:19 UTC (permalink / raw)
To: Jonathan Corbet, Andrew Morton, SeongJae Park, Jiajian Ye,
Thomas Gleixner, Ira Weiny, Matthew Wilcox, Peter Zijlstra,
outreachy, linux-doc, linux-kernel
Cc: Fabio M. De Francesco
Include kernel-doc comments to highmem.rst from
include/kernel/highmem.h.
From now on, the "Using kmap_atomic" section is redundant and obsolete,
therefore remove it.
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
Documentation/vm/highmem.rst | 41 ++++++------------------------------
1 file changed, 6 insertions(+), 35 deletions(-)
diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/highmem.rst
index 12dcfbee094d..5dcee6233cd5 100644
--- a/Documentation/vm/highmem.rst
+++ b/Documentation/vm/highmem.rst
@@ -117,41 +117,6 @@ The kernel contains several ways of creating temporary mappings:
synchronization to unmap.
-Using kmap_atomic
-=================
-
-When and where to use kmap_atomic() is straightforward. It is used when code
-wants to access the contents of a page that might be allocated from high memory
-(see __GFP_HIGHMEM), for example a page in the pagecache. The API has two
-functions, and they can be used in a manner similar to the following::
-
- /* Find the page of interest. */
- struct page *page = find_get_page(mapping, offset);
-
- /* Gain access to the contents of that page. */
- void *vaddr = kmap_atomic(page);
-
- /* Do something to the contents of that page. */
- memset(vaddr, 0, PAGE_SIZE);
-
- /* Unmap that page. */
- kunmap_atomic(vaddr);
-
-Note that the kunmap_atomic() call takes the result of the kmap_atomic() call
-not the argument.
-
-If you need to map two pages because you want to copy from one page to
-another you need to keep the kmap_atomic calls strictly nested, like::
-
- vaddr1 = kmap_atomic(page1);
- vaddr2 = kmap_atomic(page2);
-
- memcpy(vaddr1, vaddr2, PAGE_SIZE);
-
- kunmap_atomic(vaddr2);
- kunmap_atomic(vaddr1);
-
-
Cost of Temporary Mappings
==========================
@@ -190,3 +155,9 @@ The general recommendation is that you don't use more than 8GiB on a 32-bit
machine - although more might work for you and your workload, you're pretty
much on your own - don't expect kernel developers to really care much if things
come apart.
+
+
+Functions
+=========
+
+.. kernel-doc:: include/linux/highmem.h
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] Documentation/vm: Extend "Temporary Virtual Mappings" section
2022-04-15 23:19 ` [PATCH v3 1/2] Documentation/vm: Extend "Temporary Virtual Mappings" section Fabio M. De Francesco
@ 2022-04-18 21:30 ` Ira Weiny
2022-04-19 14:52 ` Fabio M. De Francesco
0 siblings, 1 reply; 9+ messages in thread
From: Ira Weiny @ 2022-04-18 21:30 UTC (permalink / raw)
To: Fabio M. De Francesco
Cc: Jonathan Corbet, Andrew Morton, SeongJae Park, Jiajian Ye,
Thomas Gleixner, Matthew Wilcox, Peter Zijlstra, outreachy,
linux-doc, linux-kernel
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@lwn.net>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.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().
> + 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.
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/
>
> * 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()?
> + 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!
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.
Thanks for the great work so far!
Ira
> +
> +* 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.
>
>
> Using kmap_atomic
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/2] Documentation/vm: Include kernel-doc to highmem.rst
2022-04-15 23:19 ` [PATCH v3 2/2] Documentation/vm: Include kernel-doc to highmem.rst Fabio M. De Francesco
@ 2022-04-18 21:32 ` Ira Weiny
0 siblings, 0 replies; 9+ messages in thread
From: Ira Weiny @ 2022-04-18 21:32 UTC (permalink / raw)
To: Fabio M. De Francesco
Cc: Jonathan Corbet, Andrew Morton, SeongJae Park, Jiajian Ye,
Thomas Gleixner, Matthew Wilcox, Peter Zijlstra, outreachy,
linux-doc, linux-kernel
On Sat, Apr 16, 2022 at 01:19:17AM +0200, Fabio M. De Francesco wrote:
> Include kernel-doc comments to highmem.rst from
> include/kernel/highmem.h.
>
> From now on, the "Using kmap_atomic" section is redundant and obsolete,
> therefore remove it.
>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> ---
> Documentation/vm/highmem.rst | 41 ++++++------------------------------
> 1 file changed, 6 insertions(+), 35 deletions(-)
>
> diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/highmem.rst
> index 12dcfbee094d..5dcee6233cd5 100644
> --- a/Documentation/vm/highmem.rst
> +++ b/Documentation/vm/highmem.rst
> @@ -117,41 +117,6 @@ The kernel contains several ways of creating temporary mappings:
> synchronization to unmap.
>
>
> -Using kmap_atomic
> -=================
> -
> -When and where to use kmap_atomic() is straightforward. It is used when code
> -wants to access the contents of a page that might be allocated from high memory
> -(see __GFP_HIGHMEM), for example a page in the pagecache. The API has two
> -functions, and they can be used in a manner similar to the following::
> -
> - /* Find the page of interest. */
> - struct page *page = find_get_page(mapping, offset);
> -
> - /* Gain access to the contents of that page. */
> - void *vaddr = kmap_atomic(page);
> -
> - /* Do something to the contents of that page. */
> - memset(vaddr, 0, PAGE_SIZE);
> -
> - /* Unmap that page. */
> - kunmap_atomic(vaddr);
> -
> -Note that the kunmap_atomic() call takes the result of the kmap_atomic() call
> -not the argument.
> -
> -If you need to map two pages because you want to copy from one page to
> -another you need to keep the kmap_atomic calls strictly nested, like::
> -
> - vaddr1 = kmap_atomic(page1);
> - vaddr2 = kmap_atomic(page2);
> -
> - memcpy(vaddr1, vaddr2, PAGE_SIZE);
> -
> - kunmap_atomic(vaddr2);
> - kunmap_atomic(vaddr1);
> -
> -
> Cost of Temporary Mappings
> ==========================
>
> @@ -190,3 +155,9 @@ The general recommendation is that you don't use more than 8GiB on a 32-bit
> machine - although more might work for you and your workload, you're pretty
> much on your own - don't expect kernel developers to really care much if things
> come apart.
> +
> +
> +Functions
> +=========
> +
> +.. kernel-doc:: include/linux/highmem.h
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] Documentation/vm: Extend "Temporary Virtual Mappings" section
2022-04-18 21:30 ` Ira Weiny
@ 2022-04-19 14:52 ` Fabio M. De Francesco
0 siblings, 0 replies; 9+ messages in thread
From: Fabio M. De Francesco @ 2022-04-19 14:52 UTC (permalink / raw)
To: Ira Weiny
Cc: Jonathan Corbet, Andrew Morton, SeongJae Park, Jiajian Ye,
Thomas Gleixner, Matthew Wilcox, Peter Zijlstra, outreachy,
linux-doc, linux-kernel
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@lwn.net>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ira Weiny <ira.weiny@intel.com>
> > Cc: Matthew Wilcox <willy@infradead.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 0/2] Documentation/vm: Rework and extend highmem.rst
2022-04-15 23:19 [PATCH v3 0/2] Documentation/vm: Rework and extend highmem.rst Fabio M. De Francesco
2022-04-15 23:19 ` [PATCH v3 1/2] Documentation/vm: Extend "Temporary Virtual Mappings" section Fabio M. De Francesco
2022-04-15 23:19 ` [PATCH v3 2/2] Documentation/vm: Include kernel-doc to highmem.rst Fabio M. De Francesco
@ 2022-04-21 17:51 ` Fabio M. De Francesco
2 siblings, 0 replies; 9+ messages in thread
From: Fabio M. De Francesco @ 2022-04-21 17:51 UTC (permalink / raw)
To: Jonathan Corbet, Andrew Morton, SeongJae Park, Jiajian Ye,
Thomas Gleixner, Ira Weiny, Matthew Wilcox, Peter Zijlstra,
outreachy, linux-doc, linux-kernel
On sabato 16 aprile 2022 01:19:15 CEST 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.
>
> Include kerneldoc comments from include/linux/highmem.h and remove
> redundant and obsolete section about kmap_atomic().
>
> v1-v2:
> 1/2 - According to comments on v1 by Matthew Wilcox and Ira
Weiny,
> correct a mistake in text, format paragraphs to stay within
> the 75 characters limit, re-order the flow of the same
> paragraphs in decreasing order of preference of usage.
> 2/2 - No patch.
>
> v2->v3:
> 1/2 - No changes.
> 2/2 - Added patch to include kernel-doc to highmem.rst and
> remove the now redundant section about kmap_atomic().
>
> Fabio M. De Francesco (2):
> Documentation/vm: Extend "Temporary Virtual Mappings" section in
> highmem.rst
> Documentation/vm: Include kernel-doc to highmem.rst
>
> Documentation/vm/highmem.rst | 96 +++++++++++++++++++++---------------
> 1 file changed, 56 insertions(+), 40 deletions(-)
>
> --
> 2.34.1
>
Dear Maintainers,
Please drop this patch because it has been reworked and inserted as part of
a new series whose subject is "Extend and reorganize Highmem's
documentation" and which is about to be submitted.
Thanks,
Fabio M. De Francesco
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 0/2] Documentation/vm: Rework and extend highmem.rst
2022-04-15 22:48 Fabio M. De Francesco
@ 2022-04-15 23:15 ` Fabio M. De Francesco
0 siblings, 0 replies; 9+ messages in thread
From: Fabio M. De Francesco @ 2022-04-15 23:15 UTC (permalink / raw)
To: Jonathan Corbet, Andrew Morton, SeongJae Park, Jiajian Ye,
Thomas Gleixner, Ira Weiny, Matthew Wilcox, Peter Zijlstra,
outreachy
On sabato 16 aprile 2022 00:48:57 CEST 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.
>
> Include kerneldoc comments from include/linux/highmem.h and remove
> redundant and obsolete section about kmap_atomic().
>
> v1-v2:
> 1/2 - According to comments on v1 by Matthew Wilcox and Ira
Weiny,
> correct a mistake in text, format paragraphs to stay within
> the 75 characters limit, re-order the flow of the same
> paragraphs in decreasing order of preference of usage.
> 2/2 - No patch.
>
> v2->v3:
> 1/2 - No changes.
> 2/2 - Added patch to include kernel-doc to highmem.rst and
> remove the now redundant section about kmap_atomic().
>
> Fabio M. De Francesco (2):
> Documentation/vm: Extend "Temporary Virtual Mappings" section in
> highmem.rst
> Documentation/vm: Include kernel-doc to highmem.rst
>
> Documentation/vm/highmem.rst | 96 +++++++++++++++++++++---------------
> 1 file changed, 56 insertions(+), 40 deletions(-)
>
> --
> 2.34.1
I'm sorry but I need to resend the whole series because for some reasons I
forgot to CC the relevant lists.
Again, sorry for the noise,
Fabio M. De Francesco
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 0/2] Documentation/vm: Rework and extend highmem.rst
@ 2022-04-15 22:48 Fabio M. De Francesco
2022-04-15 23:15 ` Fabio M. De Francesco
0 siblings, 1 reply; 9+ messages in thread
From: Fabio M. De Francesco @ 2022-04-15 22:48 UTC (permalink / raw)
To: Jonathan Corbet, Andrew Morton, SeongJae Park, Jiajian Ye,
Thomas Gleixner, Ira Weiny, Matthew Wilcox, Peter Zijlstra,
outreachy
Cc: 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. Re-order paragraphs
in decreasing order of preference of usage.
Include kerneldoc comments from include/linux/highmem.h and remove
redundant and obsolete section about kmap_atomic().
v1-v2:
1/2 - According to comments on v1 by Mattew Wilcox and Ira Weiny,
correct a mistake in text, format paragraphs to stay within
the 75 characters limit, re-order the flow of the same
paragraphs in decreasing order of preference of usage.
2/2 - No patch.
v2->v3:
1/2 - No changes.
2/2 - Added patch to include kernel-doc to highmem.rst and
remove the now redundant section about kmap_atomic().
Fabio M. De Francesco (2):
Documentation/vm: Extend "Temporary Virtual Mappings" section in
highmem.rst
Documentation/vm: Include kernel-doc to highmem.rst
Documentation/vm/highmem.rst | 96 +++++++++++++++++++++---------------
1 file changed, 56 insertions(+), 40 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-04-21 17:51 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-15 23:19 [PATCH v3 0/2] Documentation/vm: Rework and extend highmem.rst Fabio M. De Francesco
2022-04-15 23:19 ` [PATCH v3 1/2] Documentation/vm: Extend "Temporary Virtual Mappings" section Fabio M. De Francesco
2022-04-18 21:30 ` Ira Weiny
2022-04-19 14:52 ` Fabio M. De Francesco
2022-04-15 23:19 ` [PATCH v3 2/2] Documentation/vm: Include kernel-doc to highmem.rst Fabio M. De Francesco
2022-04-18 21:32 ` Ira Weiny
2022-04-21 17:51 ` [PATCH v3 0/2] Documentation/vm: Rework and extend highmem.rst Fabio M. De Francesco
-- strict thread matches above, loose matches on Subject: below --
2022-04-15 22:48 Fabio M. De Francesco
2022-04-15 23:15 ` Fabio M. De Francesco
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.