2022-04-29 14:05:29

by Fabio M. De Francesco

[permalink] [raw]
Subject: [PATCH v4 0/4] Extend and reorganize Highmem's documentation

This series has the purpose to extend and reorganize Highmem's
documentation.

This is a work in progress because some information should still be moved
from highmem.rst to highmem.h and highmem-internal.h. Specifically I'm
talking about moving the "how to" information to the relevant headers, as
it as been suggested by Ira Weiny (Intel).

Also, this is a work in progress because some kdocs in highmem.h and
highmem-internal.h should be improved.

However, I think (and hope that my thoughts are also shared by Maintainers
and Reviewers) that this series constitues a sensible step forward
improving Highmem's documentation, despite some important issues are not
yet addressed.

Changes from v1 to v2:

1/4 - Fix typos (Mike Rapoport); re-write the description of @page
because the original was wrong (Ira Weiny); add Ira's and
Mike's tags in the commit message.
2/4 - Add Ira's and Mike's tags in the commit message.
3/4 - Rework the subject to better summarize what this patch
changes; merge the section which was removed from highmem.rst
with the kdocs of highmem.h (suggested by Ira Weiny); add
Ira's tag in the commit message.
4/4 - Reformulate a sentence that was incomprehensible due to my
own mistakes in copying and pasting the text of another
paragraph (problem noted by Ira Weiny); refer to the kdocs
of kmap_local_page () to show how nested mappings should be
handled; fix grammar error; add Ira's tag in the commit
message.

Changes from v2 to v3:

1/4 - Add a deprecation notice to kunmap_atomic() for consistency
with the same notice in kmap_atomic() (Sebastian Andrzej
Siewior); shorten subject and extend commit message.
2/4 - No changes.
3/4 - No changes.
4/4 - Correct a technical inaccuracy about preemption disabling /
re-enabling in kmap_atomic() / kunmap_atomic() (Sebastian
Andrzej Siewior).

Changes from v3 to v4:

1/4 - Correct some techinal inaccuracies about the side effects
of calling kmap_atomic() and kmap_local_page() (Sebastian
Andrzej Siewior); drop "Acked-by:" and "Reviewed-by:" tags
because relevant parts of this patch have changed.
2/4 - No changes.
3/4 - No changes.
4/4 - Shorten a couple of paragraphs by removing redundant
information (Sebastian Andrzej Siewior).

Fabio M. De Francesco (4):
mm/highmem: Fix kernel-doc warnings in highmem*.h
Documentation/vm: Include kdocs from highmem*.h into highmem.rst
Documentation/vm: Move "Using kmap-atomic" from highmem.rst to
highmem.h
Documentation/vm: Rework "Temporary Virtual Mappings" section

Documentation/vm/highmem.rst | 100 ++++++++++++++++++-------------
include/linux/highmem-internal.h | 18 +++++-
include/linux/highmem.h | 51 ++++++++++++----
3 files changed, 113 insertions(+), 56 deletions(-)

--
2.34.1


Subject: Re: [PATCH v4 0/4] Extend and reorganize Highmem's documentation

On 2022-04-28 23:24:51 [+0200], Fabio M. De Francesco wrote:
> This series has the purpose to extend and reorganize Highmem's
> documentation.

Thanks.

Reviewed-by: Sebastian Andrzej Siewior <[email protected]>

Sebastian

2022-05-02 23:37:49

by Ira Weiny

[permalink] [raw]
Subject: Re: [PATCH v4 0/4] Extend and reorganize Highmem's documentation

On Thu, Apr 28, 2022 at 11:24:51PM +0200, Fabio M. De Francesco wrote:
> This series has the purpose to extend and reorganize Highmem's
> documentation.
>
> This is a work in progress because some information should still be moved
> from highmem.rst to highmem.h and highmem-internal.h. Specifically I'm
> talking about moving the "how to" information to the relevant headers, as
> it as been suggested by Ira Weiny (Intel).
>
> Also, this is a work in progress because some kdocs in highmem.h and
> highmem-internal.h should be improved.
>
> However, I think (and hope that my thoughts are also shared by Maintainers
> and Reviewers) that this series constitues a sensible step forward
> improving Highmem's documentation, despite some important issues are not
> yet addressed.

For the series:

Reviewed-by: Ira Weiny <[email protected]>

>
> Changes from v1 to v2:
>
> 1/4 - Fix typos (Mike Rapoport); re-write the description of @page
> because the original was wrong (Ira Weiny); add Ira's and
> Mike's tags in the commit message.
> 2/4 - Add Ira's and Mike's tags in the commit message.
> 3/4 - Rework the subject to better summarize what this patch
> changes; merge the section which was removed from highmem.rst
> with the kdocs of highmem.h (suggested by Ira Weiny); add
> Ira's tag in the commit message.
> 4/4 - Reformulate a sentence that was incomprehensible due to my
> own mistakes in copying and pasting the text of another
> paragraph (problem noted by Ira Weiny); refer to the kdocs
> of kmap_local_page () to show how nested mappings should be
> handled; fix grammar error; add Ira's tag in the commit
> message.
>
> Changes from v2 to v3:
>
> 1/4 - Add a deprecation notice to kunmap_atomic() for consistency
> with the same notice in kmap_atomic() (Sebastian Andrzej
> Siewior); shorten subject and extend commit message.
> 2/4 - No changes.
> 3/4 - No changes.
> 4/4 - Correct a technical inaccuracy about preemption disabling /
> re-enabling in kmap_atomic() / kunmap_atomic() (Sebastian
> Andrzej Siewior).
>
> Changes from v3 to v4:
>
> 1/4 - Correct some techinal inaccuracies about the side effects
> of calling kmap_atomic() and kmap_local_page() (Sebastian
> Andrzej Siewior); drop "Acked-by:" and "Reviewed-by:" tags
> because relevant parts of this patch have changed.
> 2/4 - No changes.
> 3/4 - No changes.
> 4/4 - Shorten a couple of paragraphs by removing redundant
> information (Sebastian Andrzej Siewior).
>
> Fabio M. De Francesco (4):
> mm/highmem: Fix kernel-doc warnings in highmem*.h
> Documentation/vm: Include kdocs from highmem*.h into highmem.rst
> Documentation/vm: Move "Using kmap-atomic" from highmem.rst to
> highmem.h
> Documentation/vm: Rework "Temporary Virtual Mappings" section
>
> Documentation/vm/highmem.rst | 100 ++++++++++++++++++-------------
> include/linux/highmem-internal.h | 18 +++++-
> include/linux/highmem.h | 51 ++++++++++++----
> 3 files changed, 113 insertions(+), 56 deletions(-)
>
> --
> 2.34.1
>

2022-05-03 00:18:47

by Fabio M. De Francesco

[permalink] [raw]
Subject: [PATCH v4 2/4] Documentation/vm: Include kdocs from highmem*.h into highmem.rst

kernel-docs that are in include/linux/highmem.h and in
include/linux/highmem-internal.h should be included in highmem.rst.

Use kdocs directives to include the above-mentioned comments into
highmem.rst.

Cc: Jonathan Corbet <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Sebastian Andrzej Siewior <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Suggested-by: Ira Weiny <[email protected]>
Acked-by: Mike Rapoport <[email protected]>
Reviewed-by: Ira Weiny <[email protected]>
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
Documentation/vm/highmem.rst | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/highmem.rst
index 0f69a9fec34d..ccff08a8211d 100644
--- a/Documentation/vm/highmem.rst
+++ b/Documentation/vm/highmem.rst
@@ -145,3 +145,10 @@ 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
+.. kernel-doc:: include/linux/highmem-internal.h
--
2.34.1

2022-05-03 01:22:18

by Fabio M. De Francesco

[permalink] [raw]
Subject: [PATCH v4 4/4] Documentation/vm: Rework "Temporary Virtual Mappings" section

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 <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Sebastian Andrzej Siewior <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Suggested-by: Ira Weiny <[email protected]>
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
Documentation/vm/highmem.rst | 70 ++++++++++++++++++++++++++++++------
1 file changed, 59 insertions(+), 11 deletions(-)

diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/highmem.rst
index e05bf5524174..c9887f241c6c 100644
--- a/Documentation/vm/highmem.rst
+++ b/Documentation/vm/highmem.rst
@@ -50,26 +50,74 @@ 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 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.
+
+ 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.
+
+ 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
+ (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. Therefore
+ users should prefer kmap_local_page() instead of kmap_atomic().

- 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 longer 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.
+
+ 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