All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/7] highmem: Extend kmap_local_page() documentation
@ 2022-07-21 21:01 Fabio M. De Francesco
  2022-07-21 21:02 ` [PATCH 1/7] highmem: Remove unneeded spaces in kmap_local_page() kdocs Fabio M. De Francesco
                   ` (6 more replies)
  0 siblings, 7 replies; 11+ messages in thread
From: Fabio M. De Francesco @ 2022-07-21 21:01 UTC (permalink / raw)
  To: Ira Weiny, Andrew Morton, Catalin Marinas,
	Matthew Wilcox (Oracle),
	Will Deacon, Peter Collingbourne, Vlastimil Babka,
	Sebastian Andrzej Siewior, Jonathan Corbet, linux-kernel,
	linux-doc
  Cc: Fabio M. De Francesco, Mike Rapoport, Thomas Gleixner

The Highmem's interface is evolving and the current documentation does not
reflect the intended uses of each of the calls. Furthermore, after a
recent series of reworks, the differences of the calls can still be
confusing and may lead to the expanded use of calls which are deprecated.

This series is the second round of changes towards an enhanced
documentation of the Highmem's interface; at this stage the patches are
only focused to kmap_local_page().

In addition it also contains some minor clean ups.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Mike Rapoport <rppt@linux.ibm.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Fabio M. De Francesco (7):
  highmem: Remove unneeded spaces in kmap_local_page() kdocs
  highmem: Specify that kmap_local_page() is callable from interrupts
  Documentation/mm: Don't kmap*() pages which can't come from HIGHMEM
  Documentation/mm: Avoid invalid use of addresses from
    kmap_local_page()
  Documentation/mm: Prefer kmap_local_page() and avoid kmap()
  highmem: Delete a sentence from kmap_local_page() kdocs
  Documentation/mm: Add details about kmap_local_page() and preemption

 Documentation/vm/highmem.rst | 31 +++++++++++++++++++++++++++----
 include/linux/highmem.h      |  7 +++----
 2 files changed, 30 insertions(+), 8 deletions(-)

-- 
2.37.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/7] highmem: Remove unneeded spaces in kmap_local_page() kdocs
  2022-07-21 21:01 [PATCH 0/7] highmem: Extend kmap_local_page() documentation Fabio M. De Francesco
@ 2022-07-21 21:02 ` Fabio M. De Francesco
  2022-07-21 21:02 ` [PATCH 2/7] highmem: Specify that kmap_local_page() is callable from interrupts Fabio M. De Francesco
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 11+ messages in thread
From: Fabio M. De Francesco @ 2022-07-21 21:02 UTC (permalink / raw)
  To: Ira Weiny, Andrew Morton, Catalin Marinas,
	Matthew Wilcox (Oracle),
	Will Deacon, Peter Collingbourne, Vlastimil Babka,
	Sebastian Andrzej Siewior, Jonathan Corbet, linux-kernel,
	linux-doc
  Cc: Fabio M. De Francesco, Mike Rapoport, Thomas Gleixner

In the kdocs of kmap_local_page(), the description of @page starts after
several unnecessary spaces.

Therefore, remove those spaces.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Mike Rapoport <rppt@linux.ibm.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 include/linux/highmem.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index 158a3a2907dc..accd286a6af5 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -60,7 +60,7 @@ static inline void kmap_flush_unused(void);
 
 /**
  * kmap_local_page - Map a page for temporary usage
- * @page:	Pointer to the page to be mapped
+ * @page: Pointer to the page to be mapped
  *
  * Returns: The virtual address of the mapping
  *
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/7] highmem: Specify that kmap_local_page() is callable from interrupts
  2022-07-21 21:01 [PATCH 0/7] highmem: Extend kmap_local_page() documentation Fabio M. De Francesco
  2022-07-21 21:02 ` [PATCH 1/7] highmem: Remove unneeded spaces in kmap_local_page() kdocs Fabio M. De Francesco
@ 2022-07-21 21:02 ` Fabio M. De Francesco
  2022-07-21 21:02 ` [PATCH 3/7] Documentation/mm: Don't kmap*() pages which can't come from HIGHMEM Fabio M. De Francesco
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 11+ messages in thread
From: Fabio M. De Francesco @ 2022-07-21 21:02 UTC (permalink / raw)
  To: Ira Weiny, Andrew Morton, Catalin Marinas,
	Matthew Wilcox (Oracle),
	Will Deacon, Peter Collingbourne, Vlastimil Babka,
	Sebastian Andrzej Siewior, Jonathan Corbet, linux-kernel,
	linux-doc
  Cc: Fabio M. De Francesco, Mike Rapoport, Thomas Gleixner

In a recent thread about converting kmap() to kmap_local_page(), the
safety of calling kmap_local_page() was questioned.[1]

"any context" should probably be enough detail for users who want to
know whether or not kmap_local_page() can be called from interrupts.
However, Linux still has kmap_atomic() which might make users think they
must use the latter in interrupts.

Add "including interrupts" for better clarity.

[1] https://lore.kernel.org/lkml/3187836.aeNJFYEL58@opensuse/

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Mike Rapoport <rppt@linux.ibm.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 include/linux/highmem.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index accd286a6af5..0ba031ad29c2 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -64,7 +64,7 @@ static inline void kmap_flush_unused(void);
  *
  * Returns: The virtual address of the mapping
  *
- * Can be invoked from any context.
+ * Can be invoked from any context, including interrupts.
  *
  * Requires careful handling when nesting multiple mappings because the map
  * management is stack based. The unmap has to be in the reverse order of
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/7] Documentation/mm: Don't kmap*() pages which can't come from HIGHMEM
  2022-07-21 21:01 [PATCH 0/7] highmem: Extend kmap_local_page() documentation Fabio M. De Francesco
  2022-07-21 21:02 ` [PATCH 1/7] highmem: Remove unneeded spaces in kmap_local_page() kdocs Fabio M. De Francesco
  2022-07-21 21:02 ` [PATCH 2/7] highmem: Specify that kmap_local_page() is callable from interrupts Fabio M. De Francesco
@ 2022-07-21 21:02 ` Fabio M. De Francesco
  2022-07-21 21:13   ` Jonathan Corbet
  2022-07-21 21:02 ` [PATCH 4/7] Documentation/mm: Avoid invalid use of addresses from kmap_local_page() Fabio M. De Francesco
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 11+ messages in thread
From: Fabio M. De Francesco @ 2022-07-21 21:02 UTC (permalink / raw)
  To: Ira Weiny, Andrew Morton, Catalin Marinas,
	Matthew Wilcox (Oracle),
	Will Deacon, Peter Collingbourne, Vlastimil Babka,
	Sebastian Andrzej Siewior, Jonathan Corbet, linux-kernel,
	linux-doc
  Cc: Fabio M. De Francesco, Mike Rapoport, Thomas Gleixner

There is no need to kmap*() pages which are guaranteed to come from
ZONE_NORMAL (or lower). Linux has currently several call sites of
kmap{,_atomic,_local_page}() on pages allocated, for instance, with
alloc_page(GFP_NOFS) and other similar allocations.

Therefore, add a paragraph to highmem.rst, to explain better that a
plain page_address() should be used for getting the address of pages
which cannot come from ZONE_HIGHMEM.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Mike Rapoport <rppt@linux.ibm.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 Documentation/vm/highmem.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/highmem.rst
index c9887f241c6c..f266354c82ab 100644
--- a/Documentation/vm/highmem.rst
+++ b/Documentation/vm/highmem.rst
@@ -71,6 +71,12 @@ list shows them in order of preference of use.
   kmap_local_page() always returns a valid virtual address and it is assumed
   that kunmap_local() will never fail.
 
+  On CONFIG_HIGHMEM=n kernels and for low memory pages this returns the
+  virtual address of the direct mapping. Only real highmem pages are
+  temporarily mapped. Therefore, users should instead call a plain
+  page_address() for getting the address of memory pages which, depending
+  on the GFP_* flags, cannot come from ZONE_HIGHMEM.
+
   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
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 4/7] Documentation/mm: Avoid invalid use of addresses from kmap_local_page()
  2022-07-21 21:01 [PATCH 0/7] highmem: Extend kmap_local_page() documentation Fabio M. De Francesco
                   ` (2 preceding siblings ...)
  2022-07-21 21:02 ` [PATCH 3/7] Documentation/mm: Don't kmap*() pages which can't come from HIGHMEM Fabio M. De Francesco
@ 2022-07-21 21:02 ` Fabio M. De Francesco
  2022-07-21 21:02 ` [PATCH 5/7] Documentation/mm: Prefer kmap_local_page() and avoid kmap() Fabio M. De Francesco
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 11+ messages in thread
From: Fabio M. De Francesco @ 2022-07-21 21:02 UTC (permalink / raw)
  To: Ira Weiny, Andrew Morton, Catalin Marinas,
	Matthew Wilcox (Oracle),
	Will Deacon, Peter Collingbourne, Vlastimil Babka,
	Sebastian Andrzej Siewior, Jonathan Corbet, linux-kernel,
	linux-doc
  Cc: Fabio M. De Francesco, Mike Rapoport, Thomas Gleixner

Users of kmap_local_page() must be absolutely sure to not hand kernel
virtual address obtained calling kmap_local_page() on highmem pages to
other contexts because those pointers are thread local, therefore, they
are no longer valid across different contexts.

Extend the documentation of kmap_local_page() to warn users about the
above-mentioned potential invalid use of pointers returned by
kmap_local_page().

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Mike Rapoport <rppt@linux.ibm.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Suggested-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 f266354c82ab..50d2b1fac24a 100644
--- a/Documentation/vm/highmem.rst
+++ b/Documentation/vm/highmem.rst
@@ -77,6 +77,13 @@ list shows them in order of preference of use.
   page_address() for getting the address of memory pages which, depending
   on the GFP_* flags, cannot come from ZONE_HIGHMEM.
 
+  While it is significantly faster than kmap(), for the higmem case it
+  comes with restrictions about the pointers validity. Contrary to kmap()
+  mappings, the local mappings are only valid in the context of the caller
+  and cannot be handed to other contexts. This implies that users must
+  be absolutely sure to keep the use of the return address local to the
+  thread which mapped it.
+
   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
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 5/7] Documentation/mm: Prefer kmap_local_page() and avoid kmap()
  2022-07-21 21:01 [PATCH 0/7] highmem: Extend kmap_local_page() documentation Fabio M. De Francesco
                   ` (3 preceding siblings ...)
  2022-07-21 21:02 ` [PATCH 4/7] Documentation/mm: Avoid invalid use of addresses from kmap_local_page() Fabio M. De Francesco
@ 2022-07-21 21:02 ` Fabio M. De Francesco
  2022-07-21 21:02 ` [PATCH 6/7] highmem: Delete a sentence from kmap_local_page() kdocs Fabio M. De Francesco
  2022-07-21 21:02 ` [PATCH 7/7] Documentation/mm: Add details about kmap_local_page() and preemption Fabio M. De Francesco
  6 siblings, 0 replies; 11+ messages in thread
From: Fabio M. De Francesco @ 2022-07-21 21:02 UTC (permalink / raw)
  To: Ira Weiny, Andrew Morton, Catalin Marinas,
	Matthew Wilcox (Oracle),
	Will Deacon, Peter Collingbourne, Vlastimil Babka,
	Sebastian Andrzej Siewior, Jonathan Corbet, linux-kernel,
	linux-doc
  Cc: Fabio M. De Francesco, Mike Rapoport, Thomas Gleixner

The reasoning for converting kmap() to kmap_local_page() was questioned
recently.[1]

There are two main problems with kmap(): (1) It comes with an overhead as
mapping space is restricted and protected by a global lock for
synchronization and (2) kmap() also requires global TLB invalidation when
its pool wraps and it might block when the mapping space is fully utilized
until a slot becomes available.

Warn users to avoid the use of kmap() and instead use kmap_local_page(),
by designing their code to map pages in the same context the mapping will
be used.

[1] https://lore.kernel.org/lkml/1891319.taCxCBeP46@opensuse/

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Mike Rapoport <rppt@linux.ibm.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 Documentation/vm/highmem.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/highmem.rst
index 50d2b1fac24a..564017b447b1 100644
--- a/Documentation/vm/highmem.rst
+++ b/Documentation/vm/highmem.rst
@@ -84,6 +84,11 @@ list shows them in order of preference of use.
   be absolutely sure to keep the use of the return address local to the
   thread which mapped it.
 
+  Most code can be designed to use thread local mappings. User should
+  therefore try to design their code to avoid the use of kmap() by mapping
+  pages in the same thread the address will be used and prefer
+  kmap_local_page().
+
   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
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 6/7] highmem: Delete a sentence from kmap_local_page() kdocs
  2022-07-21 21:01 [PATCH 0/7] highmem: Extend kmap_local_page() documentation Fabio M. De Francesco
                   ` (4 preceding siblings ...)
  2022-07-21 21:02 ` [PATCH 5/7] Documentation/mm: Prefer kmap_local_page() and avoid kmap() Fabio M. De Francesco
@ 2022-07-21 21:02 ` Fabio M. De Francesco
  2022-07-21 21:02 ` [PATCH 7/7] Documentation/mm: Add details about kmap_local_page() and preemption Fabio M. De Francesco
  6 siblings, 0 replies; 11+ messages in thread
From: Fabio M. De Francesco @ 2022-07-21 21:02 UTC (permalink / raw)
  To: Ira Weiny, Andrew Morton, Catalin Marinas,
	Matthew Wilcox (Oracle),
	Will Deacon, Peter Collingbourne, Vlastimil Babka,
	Sebastian Andrzej Siewior, Jonathan Corbet, linux-kernel,
	linux-doc
  Cc: Fabio M. De Francesco, Mike Rapoport, Thomas Gleixner

kmap_local_page() should always be preferred in place of kmap() and
kmap_atomic(). "Only use when really necessary." is not consistent with
the Documentation/mm/highmem.rst and these kdocs it embeds.

Therefore, delete the above-mentioned sentence from kdocs.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Mike Rapoport <rppt@linux.ibm.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 include/linux/highmem.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index 0ba031ad29c2..63f25dfc6317 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -86,8 +86,7 @@ static inline void kmap_flush_unused(void);
  * temporarily mapped.
  *
  * While it is significantly faster than kmap() for the higmem case it
- * comes with restrictions about the pointer validity. Only use when really
- * necessary.
+ * comes with restrictions about the pointer validity.
  *
  * On HIGHMEM enabled systems mapping a highmem page has the side effect of
  * disabling migration in order to keep the virtual address stable across
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 7/7] Documentation/mm: Add details about kmap_local_page() and preemption
  2022-07-21 21:01 [PATCH 0/7] highmem: Extend kmap_local_page() documentation Fabio M. De Francesco
                   ` (5 preceding siblings ...)
  2022-07-21 21:02 ` [PATCH 6/7] highmem: Delete a sentence from kmap_local_page() kdocs Fabio M. De Francesco
@ 2022-07-21 21:02 ` Fabio M. De Francesco
  6 siblings, 0 replies; 11+ messages in thread
From: Fabio M. De Francesco @ 2022-07-21 21:02 UTC (permalink / raw)
  To: Ira Weiny, Andrew Morton, Catalin Marinas,
	Matthew Wilcox (Oracle),
	Will Deacon, Peter Collingbourne, Vlastimil Babka,
	Sebastian Andrzej Siewior, Jonathan Corbet, linux-kernel,
	linux-doc
  Cc: Fabio M. De Francesco, Mike Rapoport, Thomas Gleixner

What happens if a thread is preempted after mapping pages with
kmap_local_page() was questioned recently.[1]

Commit f3ba3c710ac5 ("mm/highmem: Provide kmap_local*") from Thomas
Gleixner explains clearly that on context switch, the maps of an
outgoing task are removed and the map of the incoming task are restored
and that kmap_local_page() can be invoked from both preemptible and
atomic contexts.[2]

Therefore, for the purpose to make it clearer that users can call
kmap_local_page() from contexts that allow preemption, rework a couple
of sentences and add further information in highmem.rst.

[1] https://lore.kernel.org/lkml/5303077.Sb9uPGUboI@opensuse/
[2] https://lore.kernel.org/all/20201118204007.468533059@linutronix.de/

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Mike Rapoport <rppt@linux.ibm.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 Documentation/vm/highmem.rst | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/highmem.rst
index 564017b447b1..8ce43965ddef 100644
--- a/Documentation/vm/highmem.rst
+++ b/Documentation/vm/highmem.rst
@@ -60,14 +60,19 @@ list shows them in order of preference of use.
   This function should be preferred, where feasible, over all the others.
 
   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.
+  can only be accessed from within this thread and the thread is bound to the
+  CPU while the mapping is active. Although preemption is never disabled by
+  this 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.
 
+  As said, pagefaults and preemption are never disabled. There is no need to
+  disable preemption because, when context switches to a different task, the
+  maps of the outgoing task are saved and those of the incoming one are
+  restored.
+
   kmap_local_page() always returns a valid virtual address and it is assumed
   that kunmap_local() will never fail.
 
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/7] Documentation/mm: Don't kmap*() pages which can't come from HIGHMEM
  2022-07-21 21:02 ` [PATCH 3/7] Documentation/mm: Don't kmap*() pages which can't come from HIGHMEM Fabio M. De Francesco
@ 2022-07-21 21:13   ` Jonathan Corbet
  2022-07-22  0:44     ` Ira Weiny
  2022-07-28 14:52     ` Fabio M. De Francesco
  0 siblings, 2 replies; 11+ messages in thread
From: Jonathan Corbet @ 2022-07-21 21:13 UTC (permalink / raw)
  To: Fabio M. De Francesco, Ira Weiny, Andrew Morton, Catalin Marinas,
	Matthew Wilcox (Oracle),
	Will Deacon, Peter Collingbourne, Vlastimil Babka,
	Sebastian Andrzej Siewior, linux-kernel, linux-doc
  Cc: Fabio M. De Francesco, Mike Rapoport, Thomas Gleixner

"Fabio M. De Francesco" <fmdefrancesco@gmail.com> writes:

> There is no need to kmap*() pages which are guaranteed to come from
> ZONE_NORMAL (or lower). Linux has currently several call sites of
> kmap{,_atomic,_local_page}() on pages allocated, for instance, with
> alloc_page(GFP_NOFS) and other similar allocations.
>
> Therefore, add a paragraph to highmem.rst, to explain better that a
> plain page_address() should be used for getting the address of pages
> which cannot come from ZONE_HIGHMEM.
>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
> Cc: Mike Rapoport <rppt@linux.ibm.com>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  Documentation/vm/highmem.rst | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/highmem.rst
> index c9887f241c6c..f266354c82ab 100644
> --- a/Documentation/vm/highmem.rst
> +++ b/Documentation/vm/highmem.rst
> @@ -71,6 +71,12 @@ list shows them in order of preference of use.
>    kmap_local_page() always returns a valid virtual address and it is assumed
>    that kunmap_local() will never fail.
>  
> +  On CONFIG_HIGHMEM=n kernels and for low memory pages this returns the
> +  virtual address of the direct mapping. Only real highmem pages are
> +  temporarily mapped. Therefore, users should instead call a plain
> +  page_address() for getting the address of memory pages which, depending
> +  on the GFP_* flags, cannot come from ZONE_HIGHMEM.
> +

Is this good advice?  First, it requires developers to worry about
whether their pages might be in highmem, which is kind of like worrying
about having coins in your pocket in case you need a payphone.  But it
would also run afoul of other semantics for kmap*(), such as PKS, should
that ever be merged:

  https://lwn.net/Articles/894531/

Thanks,

jon

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/7] Documentation/mm: Don't kmap*() pages which can't come from HIGHMEM
  2022-07-21 21:13   ` Jonathan Corbet
@ 2022-07-22  0:44     ` Ira Weiny
  2022-07-28 14:52     ` Fabio M. De Francesco
  1 sibling, 0 replies; 11+ messages in thread
From: Ira Weiny @ 2022-07-22  0:44 UTC (permalink / raw)
  To: Jonathan Corbet
  Cc: Fabio M. De Francesco, Andrew Morton, Catalin Marinas,
	Matthew Wilcox (Oracle),
	Will Deacon, Peter Collingbourne, Vlastimil Babka,
	Sebastian Andrzej Siewior, linux-kernel, linux-doc,
	Mike Rapoport, Thomas Gleixner

On Thu, Jul 21, 2022 at 03:13:00PM -0600, Jonathan Corbet wrote:
> "Fabio M. De Francesco" <fmdefrancesco@gmail.com> writes:
> 
> > There is no need to kmap*() pages which are guaranteed to come from
> > ZONE_NORMAL (or lower). Linux has currently several call sites of
> > kmap{,_atomic,_local_page}() on pages allocated, for instance, with
> > alloc_page(GFP_NOFS) and other similar allocations.
> >
> > Therefore, add a paragraph to highmem.rst, to explain better that a
> > plain page_address() should be used for getting the address of pages
> > which cannot come from ZONE_HIGHMEM.
> >
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
> > Cc: Mike Rapoport <rppt@linux.ibm.com>
> > Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Suggested-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> >  Documentation/vm/highmem.rst | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/highmem.rst
> > index c9887f241c6c..f266354c82ab 100644
> > --- a/Documentation/vm/highmem.rst
> > +++ b/Documentation/vm/highmem.rst
> > @@ -71,6 +71,12 @@ list shows them in order of preference of use.
> >    kmap_local_page() always returns a valid virtual address and it is assumed
> >    that kunmap_local() will never fail.
> >  
> > +  On CONFIG_HIGHMEM=n kernels and for low memory pages this returns the
> > +  virtual address of the direct mapping. Only real highmem pages are
> > +  temporarily mapped. Therefore, users should instead call a plain
> > +  page_address() for getting the address of memory pages which, depending
> > +  on the GFP_* flags, cannot come from ZONE_HIGHMEM.
> > +
> 
> Is this good advice?  First, it requires developers to worry about
> whether their pages might be in highmem, which is kind of like worrying
> about having coins in your pocket in case you need a payphone.

This is a good point.  Perhaps this is better worded as:

	On CONFIG_HIGHMEM=n kernels and for low memory pages this returns the
	virtual address of the direct mapping. Only real highmem pages are
	temporarily mapped.  Therefore, users may call a plain page_address()
	for pages which are known to not come from ZONE_HIGHMEM.

	However, it is always safe to use kmap_local_page()/kunmap_local() and
	access through those calls will be as efficient as page_address() on
	most architectures.

> But it
> would also run afoul of other semantics for kmap*(), such as PKS, should
> that ever be merged:
> 
>   https://lwn.net/Articles/894531/

PKS is yet to be merged.  As of now, there is no good reason to force users to
use the kmap_local_page() if the page zone is known.

I believe that beyond PKS there will come a time when we need to change the
page_address() callers but currently this documentation is correct and does
allow callers to optimize for the corner case of a HIGHMEM system if they
desire.

As a reference, the use of kmap* vs page_address() was discussed recently in a
couple of places[1][2] and I can't fault the logic there at this time.

Thanks for the review,
Ira

[1] https://lore.kernel.org/linux-btrfs/20220621131521.GW20633@twin.jikos.cz/
[2] https://lore.kernel.org/lkml/CANn89iK6g+4Fy2VMV7=feUAOUDHu-J38be+oU76yp+zGH6xCJQ@mail.gmail.com/

> 
> Thanks,
> 
> jon

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/7] Documentation/mm: Don't kmap*() pages which can't come from HIGHMEM
  2022-07-21 21:13   ` Jonathan Corbet
  2022-07-22  0:44     ` Ira Weiny
@ 2022-07-28 14:52     ` Fabio M. De Francesco
  1 sibling, 0 replies; 11+ messages in thread
From: Fabio M. De Francesco @ 2022-07-28 14:52 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,
	Jonathan Corbet
  Cc: Mike Rapoport, Thomas Gleixner

On giovedì 21 luglio 2022 23:13:00 CEST Jonathan Corbet wrote:
> "Fabio M. De Francesco" <fmdefrancesco@gmail.com> writes:
> 
> > There is no need to kmap*() pages which are guaranteed to come from
> > ZONE_NORMAL (or lower). Linux has currently several call sites of
> > kmap{,_atomic,_local_page}() on pages allocated, for instance, with
> > alloc_page(GFP_NOFS) and other similar allocations.
> >
> > Therefore, add a paragraph to highmem.rst, to explain better that a
> > plain page_address() should be used for getting the address of pages
> > which cannot come from ZONE_HIGHMEM.
> >
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
> > Cc: Mike Rapoport <rppt@linux.ibm.com>
> > Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Suggested-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> >  Documentation/vm/highmem.rst | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/
highmem.rst
> > index c9887f241c6c..f266354c82ab 100644
> > --- a/Documentation/vm/highmem.rst
> > +++ b/Documentation/vm/highmem.rst
> > @@ -71,6 +71,12 @@ list shows them in order of preference of use.
> >    kmap_local_page() always returns a valid virtual address and it is 
assumed
> >    that kunmap_local() will never fail.
> >  
> > +  On CONFIG_HIGHMEM=n kernels and for low memory pages this returns 
the
> > +  virtual address of the direct mapping. Only real highmem pages are
> > +  temporarily mapped. Therefore, users should instead call a plain
> > +  page_address() for getting the address of memory pages which, 
depending
> > +  on the GFP_* flags, cannot come from ZONE_HIGHMEM.
> > +
> 
> Is this good advice?

Well... yes and no :-) 

However yours is a legit objection. 

I'm taking most of the suggestion from Ira (from an email in this same 
thread) and send the v2 of this series.

My intention was to avoid things like those I encountered when converting 
fs/btrfs:

page = alloc_page(GFP_NOFS);
kaddr = kmap(page);

Why one should kmap*() pages allocated one or two lines above with 
GFP_NOFS? 

Furthermore, since nesting kmap_local_page() / kunmap_local() is last in 
first out (LIFO), I had several problems with several un-mappings until 
David Sterba made me notice that GFP_NOFS is not OR'ed with __GFP_HIGHMEM 
and suggested to use plain page_address() instead of those unnecessary 
mappings.

However, you are right about the fact that, with most of other allocations, 
it is not so clear where and how pages are being allocated.

Thanks,

Fabio 

> First, it requires developers to worry about
> whether their pages might be in highmem, which is kind of like worrying
> about having coins in your pocket in case you need a payphone.  But it
> would also run afoul of other semantics for kmap*(), such as PKS, should
> that ever be merged:
> 
>   https://lwn.net/Articles/894531/
>
> Thanks,
> 
> jon
> 





^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-07-28 14:52 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-21 21:01 [PATCH 0/7] highmem: Extend kmap_local_page() documentation Fabio M. De Francesco
2022-07-21 21:02 ` [PATCH 1/7] highmem: Remove unneeded spaces in kmap_local_page() kdocs Fabio M. De Francesco
2022-07-21 21:02 ` [PATCH 2/7] highmem: Specify that kmap_local_page() is callable from interrupts Fabio M. De Francesco
2022-07-21 21:02 ` [PATCH 3/7] Documentation/mm: Don't kmap*() pages which can't come from HIGHMEM Fabio M. De Francesco
2022-07-21 21:13   ` Jonathan Corbet
2022-07-22  0:44     ` Ira Weiny
2022-07-28 14:52     ` Fabio M. De Francesco
2022-07-21 21:02 ` [PATCH 4/7] Documentation/mm: Avoid invalid use of addresses from kmap_local_page() Fabio M. De Francesco
2022-07-21 21:02 ` [PATCH 5/7] Documentation/mm: Prefer kmap_local_page() and avoid kmap() Fabio M. De Francesco
2022-07-21 21:02 ` [PATCH 6/7] highmem: Delete a sentence from kmap_local_page() kdocs Fabio M. De Francesco
2022-07-21 21:02 ` [PATCH 7/7] Documentation/mm: Add details about kmap_local_page() and preemption 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.