* [PATCH v3 0/4] Extend and reorganize Highmem's documentation
@ 2022-04-27 18:38 Fabio M. De Francesco
2022-04-27 18:38 ` [PATCH v3 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h Fabio M. De Francesco
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Fabio M. De Francesco @ 2022-04-27 18:38 UTC (permalink / raw)
To: Ira Weiny, Andrew Morton, Catalin Marinas,
Matthew Wilcox (Oracle),
Will Deacon, Peter Collingbourne, Vlastimil Babka,
Sebastian Andrzej Siewior, linux-kernel, linux-doc, outreachy
Cc: Fabio M. De Francesco
This series has the purpose to extend and reorganize Highmem's
documentation.
This is still 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).
This series is composed by four patches gathered from a previous series
made of two, plus two more single patches. The subject of this cover has
changed and the layout of the changes across the four patches has
changed too. For this reason it is very hard to show a valid versions'
log. Therefore, I decided to start over and drop versions (Maintainers
of the previous patch have been told to drop them).
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).
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" to highmem.h
Documentation/vm: Rework "Temporary Virtual Mappings" section
Documentation/vm/highmem.rst | 104 +++++++++++++++++++------------
include/linux/highmem-internal.h | 15 ++++-
include/linux/highmem.h | 49 +++++++++++----
3 files changed, 112 insertions(+), 56 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h
2022-04-27 18:38 [PATCH v3 0/4] Extend and reorganize Highmem's documentation Fabio M. De Francesco
@ 2022-04-27 18:38 ` Fabio M. De Francesco
2022-04-28 9:11 ` Sebastian Andrzej Siewior
2022-04-27 18:38 ` [PATCH v3 2/4] Documentation/vm: Include kdocs from highmem*.h into highmem.rst Fabio M. De Francesco
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Fabio M. De Francesco @ 2022-04-27 18:38 UTC (permalink / raw)
To: Ira Weiny, Andrew Morton, Catalin Marinas,
Matthew Wilcox (Oracle),
Will Deacon, Peter Collingbourne, Vlastimil Babka,
Sebastian Andrzej Siewior, linux-kernel, linux-doc, outreachy
Cc: Fabio M. De Francesco, Mike Rapoport
`scripts/kernel-doc -v -none include/linux/highmem*` reports the following
warnings:
include/linux/highmem.h:160: warning: expecting prototype for kunmap_atomic(). Prototype was for nr_free_highpages() instead
include/linux/highmem.h:204: warning: No description found for return value of 'alloc_zeroed_user_highpage_movable'
include/linux/highmem-internal.h:256: warning: Function parameter or member '__addr' not described in 'kunmap_atomic'
include/linux/highmem-internal.h:256: warning: Excess function parameter 'addr' description in 'kunmap_atomic'
Fix these warnings by (1) moving the kernel-doc comments from highmem.h to
highmem-internal.h (which is the file were the kunmap_atomic() macro is
actually defined), (2) extending and merging it with the comment which was
already in highmem-internal.h, (3) using correct parameter names, (4)
adding description of return value of alloc_zeroed_user_highpage_movable(),
(5) correcting some technical inaccuracies in comments, and (5) adding a
deprecation notice in kunmap_atomic() for consistency with kmap_atomic().
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Acked-by: Mike Rapoport <rppt@linux.ibm.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
include/linux/highmem-internal.h | 15 ++++++++++++---
include/linux/highmem.h | 20 ++++++--------------
2 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
index a77be5630209..9968a1282317 100644
--- a/include/linux/highmem-internal.h
+++ b/include/linux/highmem-internal.h
@@ -236,9 +236,18 @@ static inline unsigned long totalhigh_pages(void) { return 0UL; }
#endif /* CONFIG_HIGHMEM */
-/*
- * Prevent people trying to call kunmap_atomic() as if it were kunmap()
- * kunmap_atomic() should get the return value of kmap_atomic, not the page.
+/**
+ * kunmap_atomic - Unmap the virtual address mapped by kmap_atomic() - deprecated!
+ * @__addr: Virtual address to be unmapped
+ *
+ * Unmaps an address previously mapped by kmap_atomic() and re-enables
+ * pagefaults, migration, preemption (the latter was disabled only for
+ * !PREEMP_RT configurations). Mappings should be unmapped in the reverse
+ * order that they were mapped. See kmap_local_page() for details.
+ * @__addr can be any address within the mapped page, so there is no need
+ * to subtract any offset that has been added. In contrast to kunmap(),
+ * this function takes the address returned from kmap_atomic(), not the
+ * page passed to it. The compiler will warn you if you pass the page.
*/
#define kunmap_atomic(__addr) \
do { \
diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index 39bb9b47fa9c..3623319a659d 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -37,7 +37,7 @@ static inline void *kmap(struct page *page);
/**
* kunmap - Unmap the virtual address mapped by kmap()
- * @addr: Virtual address to be unmapped
+ * @page: Pointer to the page which was mapped by kmap()
*
* Counterpart to kmap(). A NOOP for CONFIG_HIGHMEM=n and for mappings of
* pages in the low memory area.
@@ -138,24 +138,14 @@ static inline void *kmap_local_folio(struct folio *folio, size_t offset);
*
* Returns: The virtual address of the mapping
*
- * Effectively a wrapper around kmap_local_page() which disables pagefaults
- * and preemption.
+ * In fact a wrapper around kmap_local_page() which disables pagefaults,
+ * migration, preemption (the latter disabled only for !PREEMP_RT
+ * configurations).
*
* Do not use in new code. Use kmap_local_page() instead.
*/
static inline void *kmap_atomic(struct page *page);
-/**
- * kunmap_atomic - Unmap the virtual address mapped by kmap_atomic()
- * @addr: Virtual address to be unmapped
- *
- * Counterpart to kmap_atomic().
- *
- * Effectively a wrapper around kunmap_local() which additionally undoes
- * the side effects of kmap_atomic(), i.e. reenabling pagefaults and
- * preemption.
- */
-
/* Highmem related interfaces for management code */
static inline unsigned int nr_free_highpages(void);
static inline unsigned long totalhigh_pages(void);
@@ -191,6 +181,8 @@ static inline void clear_user_highpage(struct page *page, unsigned long vaddr)
* @vma: The VMA the page is to be allocated for
* @vaddr: The virtual address the page will be inserted into
*
+ * Returns: The allocated and zeroed HIGHMEM page
+ *
* This function will allocate a page for a VMA that the caller knows will
* be able to migrate in the future using move_pages() or reclaimed
*
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 2/4] Documentation/vm: Include kdocs from highmem*.h into highmem.rst
2022-04-27 18:38 [PATCH v3 0/4] Extend and reorganize Highmem's documentation Fabio M. De Francesco
2022-04-27 18:38 ` [PATCH v3 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h Fabio M. De Francesco
@ 2022-04-27 18:38 ` Fabio M. De Francesco
2022-04-27 18:38 ` [PATCH v3 3/4] Documentation/vm: Move "Using kmap-atomic" to highmem.h Fabio M. De Francesco
2022-04-27 18:38 ` [PATCH v3 4/4] Documentation/vm: Rework "Temporary Virtual Mappings" section Fabio M. De Francesco
3 siblings, 0 replies; 11+ messages in thread
From: Fabio M. De Francesco @ 2022-04-27 18:38 UTC (permalink / raw)
To: Ira Weiny, Andrew Morton, Catalin Marinas,
Matthew Wilcox (Oracle),
Will Deacon, Peter Collingbourne, Vlastimil Babka,
Sebastian Andrzej Siewior, linux-kernel, linux-doc, outreachy
Cc: Fabio M. De Francesco, Jonathan Corbet, Thomas Gleixner,
Peter Zijlstra, Mike Rapoport
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 <corbet@lwn.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Acked-by: Mike Rapoport <rppt@linux.ibm.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 3/4] Documentation/vm: Move "Using kmap-atomic" to highmem.h
2022-04-27 18:38 [PATCH v3 0/4] Extend and reorganize Highmem's documentation Fabio M. De Francesco
2022-04-27 18:38 ` [PATCH v3 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h Fabio M. De Francesco
2022-04-27 18:38 ` [PATCH v3 2/4] Documentation/vm: Include kdocs from highmem*.h into highmem.rst Fabio M. De Francesco
@ 2022-04-27 18:38 ` Fabio M. De Francesco
2022-04-27 18:38 ` [PATCH v3 4/4] Documentation/vm: Rework "Temporary Virtual Mappings" section Fabio M. De Francesco
3 siblings, 0 replies; 11+ messages in thread
From: Fabio M. De Francesco @ 2022-04-27 18:38 UTC (permalink / raw)
To: Ira Weiny, Andrew Morton, Catalin Marinas,
Matthew Wilcox (Oracle),
Will Deacon, Peter Collingbourne, Vlastimil Babka,
Sebastian Andrzej Siewior, linux-kernel, linux-doc, outreachy
Cc: Fabio M. De Francesco, Jonathan Corbet, Thomas Gleixner, Peter Zijlstra
The use of kmap_atomic() is new code is being deprecated in favor of
kmap_local_page(). For this reason the "Using kmap_atomic" section in
highmem.rst is obsolete and unnecessary, but it can still help developers
if it were moved to kdocs in highmem.h.
Therefore, move the relevant parts of this section from highmem.rst and
merge them with the kdocs in highmem.h.
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
Documentation/vm/highmem.rst | 35 -----------------------------------
include/linux/highmem.h | 31 +++++++++++++++++++++++++++++++
2 files changed, 31 insertions(+), 35 deletions(-)
diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/highmem.rst
index ccff08a8211d..e05bf5524174 100644
--- a/Documentation/vm/highmem.rst
+++ b/Documentation/vm/highmem.rst
@@ -72,41 +72,6 @@ The kernel contains several ways of creating temporary mappings:
It may be assumed that k[un]map_atomic() won't fail.
-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
==========================
diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index 3623319a659d..d1f9b298415e 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -143,6 +143,37 @@ static inline void *kmap_local_folio(struct folio *folio, size_t offset);
* configurations).
*
* Do not use in new code. Use kmap_local_page() instead.
+ *
+ * It is used in atomic context 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);
*/
static inline void *kmap_atomic(struct page *page);
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 4/4] Documentation/vm: Rework "Temporary Virtual Mappings" section
2022-04-27 18:38 [PATCH v3 0/4] Extend and reorganize Highmem's documentation Fabio M. De Francesco
` (2 preceding siblings ...)
2022-04-27 18:38 ` [PATCH v3 3/4] Documentation/vm: Move "Using kmap-atomic" to highmem.h Fabio M. De Francesco
@ 2022-04-27 18:38 ` Fabio M. De Francesco
2022-04-28 9:02 ` Sebastian Andrzej Siewior
3 siblings, 1 reply; 11+ messages in thread
From: Fabio M. De Francesco @ 2022-04-27 18:38 UTC (permalink / raw)
To: Ira Weiny, Andrew Morton, Catalin Marinas,
Matthew Wilcox (Oracle),
Will Deacon, Peter Collingbourne, Vlastimil Babka,
Sebastian Andrzej Siewior, linux-kernel, linux-doc, outreachy
Cc: Fabio M. De Francesco, Jonathan Corbet, Thomas Gleixner, Peter Zijlstra
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 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 <corbet@lwn.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
Documentation/vm/highmem.rst | 74 ++++++++++++++++++++++++++++++------
1 file changed, 63 insertions(+), 11 deletions(-)
diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/highmem.rst
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.
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 (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.
+
+ kmap_local_page() always returns a valid virtual address and it is assumed
+ that kunmap_local() will never fail.
+
+ 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.
+
+ 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, so it
+ should be only used if it is absolutely required, otherwise kmap_local_page()
+ should be used where it is feasible.
- 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 4/4] Documentation/vm: Rework "Temporary Virtual Mappings" section
2022-04-27 18:38 ` [PATCH v3 4/4] Documentation/vm: Rework "Temporary Virtual Mappings" section Fabio M. De Francesco
@ 2022-04-28 9:02 ` Sebastian Andrzej Siewior
2022-04-28 11:14 ` Fabio M. De Francesco
0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-04-28 9:02 UTC (permalink / raw)
To: Fabio M. De Francesco
Cc: Ira Weiny, Andrew Morton, Catalin Marinas,
Matthew Wilcox (Oracle),
Will Deacon, Peter Collingbourne, Vlastimil Babka, linux-kernel,
linux-doc, outreachy, Jonathan Corbet, Thomas Gleixner,
Peter Zijlstra
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.
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.
> + 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()
> + (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.
> - It may be assumed that k[un]map_atomic() won't fail.
> + It is assumed that k[un]map_atomic() won't fail.
Sebastian
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h
2022-04-27 18:38 ` [PATCH v3 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h Fabio M. De Francesco
@ 2022-04-28 9:11 ` Sebastian Andrzej Siewior
2022-04-28 10:54 ` Fabio M. De Francesco
0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-04-28 9:11 UTC (permalink / raw)
To: Fabio M. De Francesco
Cc: Ira Weiny, Andrew Morton, Catalin Marinas,
Matthew Wilcox (Oracle),
Will Deacon, Peter Collingbourne, Vlastimil Babka, linux-kernel,
linux-doc, outreachy, Mike Rapoport
On 2022-04-27 20:38:18 [+0200], Fabio M. De Francesco wrote:
> --- a/include/linux/highmem-internal.h
> +++ b/include/linux/highmem-internal.h
> @@ -236,9 +236,18 @@ static inline unsigned long totalhigh_pages(void) { return 0UL; }
>
> #endif /* CONFIG_HIGHMEM */
>
> -/*
> - * Prevent people trying to call kunmap_atomic() as if it were kunmap()
> - * kunmap_atomic() should get the return value of kmap_atomic, not the page.
> +/**
> + * kunmap_atomic - Unmap the virtual address mapped by kmap_atomic() - deprecated!
> + * @__addr: Virtual address to be unmapped
> + *
> + * Unmaps an address previously mapped by kmap_atomic() and re-enables
> + * pagefaults, migration, preemption (the latter was disabled only for
> + * !PREEMP_RT configurations). Mappings should be unmapped in the reverse
Not sure how detailed you want to put it here as "reverses kmap_atomic()
doing." might be okay ;) This indicates the "migration" is disabled for
!PREEMPT_RT which is not the case. So maybe something like
* Unmaps an address previously mapped by kmap_atomic() and re-enables
* pagefaults, CPU migration (CONFIG_PREEMPT_RT) or preemption
* (!CONFIG_PREEMPT_RT). Mappings should be unmapped in the reverse
will make it clear.
…
> --- a/include/linux/highmem.h
> +++ b/include/linux/highmem.h
> @@ -138,24 +138,14 @@ static inline void *kmap_local_folio(struct folio *folio, size_t offset);
> *
> * Returns: The virtual address of the mapping
> *
> - * Effectively a wrapper around kmap_local_page() which disables pagefaults
> - * and preemption.
> + * In fact a wrapper around kmap_local_page() which disables pagefaults,
> + * migration, preemption (the latter disabled only for !PREEMP_RT
> + * configurations).
and here.
Sebastian
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h
2022-04-28 9:11 ` Sebastian Andrzej Siewior
@ 2022-04-28 10:54 ` Fabio M. De Francesco
2022-04-28 11:05 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 11+ messages in thread
From: Fabio M. De Francesco @ 2022-04-28 10:54 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Ira Weiny, Andrew Morton, Catalin Marinas,
Matthew Wilcox (Oracle),
Will Deacon, Peter Collingbourne, Vlastimil Babka, linux-kernel,
linux-doc, outreachy, Mike Rapoport
On giovedì 28 aprile 2022 11:11:46 CEST Sebastian Andrzej Siewior wrote:
> On 2022-04-27 20:38:18 [+0200], Fabio M. De Francesco wrote:
> > --- a/include/linux/highmem-internal.h
> > +++ b/include/linux/highmem-internal.h
> > @@ -236,9 +236,18 @@ static inline unsigned long totalhigh_pages(void)
{ return 0UL; }
> >
> > #endif /* CONFIG_HIGHMEM */
> >
> > -/*
> > - * Prevent people trying to call kunmap_atomic() as if it were
kunmap()
> > - * kunmap_atomic() should get the return value of kmap_atomic, not the
page.
> > +/**
> > + * kunmap_atomic - Unmap the virtual address mapped by kmap_atomic() -
deprecated!
> > + * @__addr: Virtual address to be unmapped
> > + *
> > + * Unmaps an address previously mapped by kmap_atomic() and re-enables
> > + * pagefaults, migration, preemption (the latter was disabled only for
> > + * !PREEMP_RT configurations). Mappings should be unmapped in the
reverse
>
> Not sure how detailed you want to put it here as "reverses kmap_atomic()
> doing." might be okay ;)
No, it's not sufficient because Matthew Wilcox said that something like "It
is the counterpart of kmap_atomic() for unmapping" (or anything similar) is
_not_ what he wants to see.
Furthermore, a large part of this text has been written by him (I'm talking
of a couple of weeks ago, when this patch was not part of this series - it
was on its own until Ira Weiny asked me to gather 4 patches in one only
series).
> This indicates the "migration" is disabled for
> !PREEMPT_RT which is not the case.
I read again how kmap_atomic() is defined. There are lots of 'if'
statements. Only if the code gets to __kmap_local_pfn_prot(), users can be
assured that it unconditionally calls both migrate_disable() and
preempt_disable().
> So maybe something like
>
> * Unmaps an address previously mapped by kmap_atomic() and re-enables
> * pagefaults, CPU migration (CONFIG_PREEMPT_RT) or preemption
> * (!CONFIG_PREEMPT_RT). Mappings should be unmapped in the reverse
>
> will make it clear.
I'm starting to think that this level of detail is too much for users who
just need to understand how to use this function as well as
kmap_local_page().
I prefer something like the following:
+ * Unmaps an address previously mapped by kmap_atomic() and re-enables
+ * pagefaults and possibly also CPU migration and/or preemption. However,
+ * users should not count on disable of migration and/or preemption as a
+ * side effect of calling kmap_atomic(). Mappings must be unmapped in the
+ * reverse [...]
I'd also like to write the same paragraph for kmap_local_page().
What do you think of being less detailed and instead using the text I wrote
above?
Thanks,
Fabio
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h
2022-04-28 10:54 ` Fabio M. De Francesco
@ 2022-04-28 11:05 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 11+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-04-28 11:05 UTC (permalink / raw)
To: Fabio M. De Francesco
Cc: Ira Weiny, Andrew Morton, Catalin Marinas,
Matthew Wilcox (Oracle),
Will Deacon, Peter Collingbourne, Vlastimil Babka, linux-kernel,
linux-doc, outreachy, Mike Rapoport
On 2022-04-28 12:54:14 [+0200], Fabio M. De Francesco wrote:
> No, it's not sufficient because Matthew Wilcox said that something like "It
> is the counterpart of kmap_atomic() for unmapping" (or anything similar) is
> _not_ what he wants to see.
>
> Furthermore, a large part of this text has been written by him (I'm talking
> of a couple of weeks ago, when this patch was not part of this series - it
> was on its own until Ira Weiny asked me to gather 4 patches in one only
> series).
Sure.
> > This indicates the "migration" is disabled for
> > !PREEMPT_RT which is not the case.
>
> I read again how kmap_atomic() is defined. There are lots of 'if'
> statements. Only if the code gets to __kmap_local_pfn_prot(), users can be
> assured that it unconditionally calls both migrate_disable() and
> preempt_disable().
Right, that part. Then keep it.
> > So maybe something like
> >
> > * Unmaps an address previously mapped by kmap_atomic() and re-enables
> > * pagefaults, CPU migration (CONFIG_PREEMPT_RT) or preemption
> > * (!CONFIG_PREEMPT_RT). Mappings should be unmapped in the reverse
> >
> > will make it clear.
>
> I'm starting to think that this level of detail is too much for users who
> just need to understand how to use this function as well as
> kmap_local_page().
>
> I prefer something like the following:
>
> + * Unmaps an address previously mapped by kmap_atomic() and re-enables
> + * pagefaults and possibly also CPU migration and/or preemption. However,
> + * users should not count on disable of migration and/or preemption as a
> + * side effect of calling kmap_atomic(). Mappings must be unmapped in the
> + * reverse [...]
>
> I'd also like to write the same paragraph for kmap_local_page().
>
> What do you think of being less detailed and instead using the text I wrote
> above?
Sounds perfect.
> Thanks,
>
> Fabio
Sebastian
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 4/4] Documentation/vm: Rework "Temporary Virtual Mappings" section
2022-04-28 9:02 ` Sebastian Andrzej Siewior
@ 2022-04-28 11:14 ` Fabio M. De Francesco
2022-04-28 16:34 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 11+ messages in thread
From: Fabio M. De Francesco @ 2022-04-28 11:14 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Ira Weiny, Andrew Morton, Catalin Marinas,
Matthew Wilcox (Oracle),
Will Deacon, Peter Collingbourne, Vlastimil Babka, linux-kernel,
linux-doc, outreachy, Jonathan Corbet, Thomas Gleixner,
Peter Zijlstra
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 4/4] Documentation/vm: Rework "Temporary Virtual Mappings" section
2022-04-28 11:14 ` Fabio M. De Francesco
@ 2022-04-28 16:34 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 11+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-04-28 16:34 UTC (permalink / raw)
To: Fabio M. De Francesco
Cc: Ira Weiny, Andrew Morton, Catalin Marinas,
Matthew Wilcox (Oracle),
Will Deacon, Peter Collingbourne, Vlastimil Babka, linux-kernel,
linux-doc, outreachy, Jonathan Corbet, Thomas Gleixner,
Peter Zijlstra
On 2022-04-28 13:14:30 [+0200], Fabio M. De Francesco wrote:
> > > + 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.
Yeah, my point is that it shouldn't be required. Using a kmap_atomic()
mapping only because it is not limited to a thread/ CPU sounds wrong.
This shouldn't be a valid requirement. Therefore kmap_local() should
always be the way to go.
Anyway, I can live with that and hopefully that interface will removed
soon :) You yourself, as you pointed out, removed a user or two so I'm
confident that it will happen at some point ;)
> Thank you so much for the time you have spent for reviewing and helping,
You are welcome.
> Fabio
Sebastian
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-04-28 16:34 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-27 18:38 [PATCH v3 0/4] Extend and reorganize Highmem's documentation Fabio M. De Francesco
2022-04-27 18:38 ` [PATCH v3 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h Fabio M. De Francesco
2022-04-28 9:11 ` Sebastian Andrzej Siewior
2022-04-28 10:54 ` Fabio M. De Francesco
2022-04-28 11:05 ` Sebastian Andrzej Siewior
2022-04-27 18:38 ` [PATCH v3 2/4] Documentation/vm: Include kdocs from highmem*.h into highmem.rst Fabio M. De Francesco
2022-04-27 18:38 ` [PATCH v3 3/4] Documentation/vm: Move "Using kmap-atomic" to highmem.h Fabio M. De Francesco
2022-04-27 18:38 ` [PATCH v3 4/4] Documentation/vm: Rework "Temporary Virtual Mappings" section Fabio M. De Francesco
2022-04-28 9:02 ` Sebastian Andrzej Siewior
2022-04-28 11:14 ` Fabio M. De Francesco
2022-04-28 16:34 ` Sebastian Andrzej Siewior
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).