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 <[email protected]>
Cc: Matthew Wilcox (Oracle) <[email protected]>
Cc: Mike Rapoport <[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]>
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
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 <[email protected]>
Cc: Matthew Wilcox (Oracle) <[email protected]>
Cc: Mike Rapoport <[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]>
---
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
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 <[email protected]>
Cc: Matthew Wilcox (Oracle) <[email protected]>
Cc: Mike Rapoport <[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 | 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
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 <[email protected]>
Cc: Matthew Wilcox (Oracle) <[email protected]>
Cc: Mike Rapoport <[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 | 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
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/[email protected]/
Cc: Andrew Morton <[email protected]>
Cc: Matthew Wilcox (Oracle) <[email protected]>
Cc: Mike Rapoport <[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 | 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
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 <[email protected]>
Cc: Matthew Wilcox (Oracle) <[email protected]>
Cc: Mike Rapoport <[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 | 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
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 <[email protected]>
Cc: Matthew Wilcox (Oracle) <[email protected]>
Cc: Mike Rapoport <[email protected]>
Cc: Sebastian Andrzej Siewior <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Suggested-by: Ira Weiny <[email protected]>
Reviewed-by: Ira Weiny <[email protected]>
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
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
"Fabio M. De Francesco" <[email protected]> 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 <[email protected]>
> Cc: Matthew Wilcox (Oracle) <[email protected]>
> Cc: Mike Rapoport <[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 | 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
On Thu, Jul 21, 2022 at 03:13:00PM -0600, Jonathan Corbet wrote:
> "Fabio M. De Francesco" <[email protected]> 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 <[email protected]>
> > Cc: Matthew Wilcox (Oracle) <[email protected]>
> > Cc: Mike Rapoport <[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 | 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/[email protected]/
[2] https://lore.kernel.org/lkml/CANn89iK6g+4Fy2VMV7=feUAOUDHu-J38be+oU76yp+zGH6xCJQ@mail.gmail.com/
>
> Thanks,
>
> jon
On giovedì 21 luglio 2022 23:13:00 CEST Jonathan Corbet wrote:
> "Fabio M. De Francesco" <[email protected]> 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 <[email protected]>
> > Cc: Matthew Wilcox (Oracle) <[email protected]>
> > Cc: Mike Rapoport <[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 | 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
>