kmap() and kmap_atomic() have been deprecated. kmap_local_page() should
always be used in new code and the call sites of the two deprecated
functions should be converted. This latter task can lead to errors if it
is not carried out with the necessary attention to the context around
and between the maps and unmaps.
Therefore, add further information to the Highmem's documentation for the
purpose to make it clearer that (1) kmap() and kmap_atomic() must not
any longer be called in new code and (2) developers doing conversions from
kmap() amd kmap_atomic() are expected to take care of the context around
and between the maps and unmaps, in order to not break the code.
Relevant parts of this patch have been taken from messages exchanged
privately with Ira Weiny (thanks!).
Cc: Andrew Morton <[email protected]>
Cc: Ira Weiny <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: Mike Rapoport <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Sebastian Andrzej Siewior <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
v1->v2: Reworked part of the "Notes" paragraph in the kmap_atomic() section
in order to leave less room for misinterpretation. I've made these changes
by merging some suggestions from Sebastian (thanks!), although I'm not yet
entirely sure I've achieved crystal clear exposition.
Documentation/mm/highmem.rst | 41 +++++++++++++++++++++++++++---------
1 file changed, 31 insertions(+), 10 deletions(-)
diff --git a/Documentation/mm/highmem.rst b/Documentation/mm/highmem.rst
index 0f731d9196b0..59d1078f53df 100644
--- a/Documentation/mm/highmem.rst
+++ b/Documentation/mm/highmem.rst
@@ -57,7 +57,8 @@ list shows them in order of preference of use.
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.
+ This function should always be used. kmap_atomic() and kmap() have been
+ deprecated.
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 to the
@@ -100,10 +101,21 @@ list shows them in order of preference of use.
(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(). This function has been deprecated; use kmap_local_page().
+
+ NOTE: Conversions to kmap_local_page() must take care to follow the mapping
+ restrictions imposed on kmap_local_page(). Furthermore, the code between
+ calls to kmap_atomic() and kunmap_atomic() may implicitly depend on the side
+ effects of atomic mappings, i.e. disabling page faults or preemption, or both.
+ In that case, explicit calls to pagefault_disable() or preempt_disable() or
+ both must be made in conjunction with the use of kmap_local_page().
+
+ [Legacy documentation]
+
+ 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 does not
sleep and the callers too may not sleep until after kunmap_atomic() is
@@ -115,11 +127,20 @@ list shows them in order of preference of use.
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().
+* kmap(). This function has been deprecated; use kmap_local_page().
+
+ NOTE: Conversions to kmap_local_page() must take care to follow the mapping
+ restrictions imposed on kmap_local_page(). In particular, it is necessary to
+ make sure that the kernel virtual memory pointer is only valid in the thread
+ that obtained it.
+
+ [Legacy documentation]
+
+ 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
--
2.38.1
On Wed, Dec 07, 2022 at 11:53:08PM +0100, Fabio M. De Francesco wrote:
> diff --git a/Documentation/mm/highmem.rst b/Documentation/mm/highmem.rst
> index 0f731d9196b0..59d1078f53df 100644
> --- a/Documentation/mm/highmem.rst
> +++ b/Documentation/mm/highmem.rst
> @@ -57,7 +57,8 @@ list shows them in order of preference of use.
> 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.
> + This function should always be used. kmap_atomic() and kmap() have been
> + deprecated.
"... always be used, whereas ..."
> + NOTE: Conversions to kmap_local_page() must take care to follow the mapping
> + restrictions imposed on kmap_local_page(). Furthermore, the code between
> + calls to kmap_atomic() and kunmap_atomic() may implicitly depend on the side
> + effects of atomic mappings, i.e. disabling page faults or preemption, or both.
> + In that case, explicit calls to pagefault_disable() or preempt_disable() or
> + both must be made in conjunction with the use of kmap_local_page().
> +
> <snipped>...
> + NOTE: Conversions to kmap_local_page() must take care to follow the mapping
> + restrictions imposed on kmap_local_page(). In particular, it is necessary to
> + make sure that the kernel virtual memory pointer is only valid in the thread
> + that obtained it.
> +
What about using note block to signify conversion notes above?
---- >8 ----
diff --git a/Documentation/mm/highmem.rst b/Documentation/mm/highmem.rst
index 59d1078f53df57..ef53eb580d4cda 100644
--- a/Documentation/mm/highmem.rst
+++ b/Documentation/mm/highmem.rst
@@ -103,12 +103,14 @@ list shows them in order of preference of use.
* kmap_atomic(). This function has been deprecated; use kmap_local_page().
- NOTE: Conversions to kmap_local_page() must take care to follow the mapping
- restrictions imposed on kmap_local_page(). Furthermore, the code between
- calls to kmap_atomic() and kunmap_atomic() may implicitly depend on the side
- effects of atomic mappings, i.e. disabling page faults or preemption, or both.
- In that case, explicit calls to pagefault_disable() or preempt_disable() or
- both must be made in conjunction with the use of kmap_local_page().
+ .. note::
+ Conversions to kmap_local_page() must take care to follow the mapping
+ restrictions imposed on kmap_local_page(). Furthermore, the code between
+ calls to kmap_atomic() and kunmap_atomic() may implicitly depend on the
+ side effects of atomic mappings, i.e. disabling page faults or preemption,
+ or both. In that case, explicit calls to pagefault_disable() or
+ preempt_disable() or both must be made in conjunction with the use of
+ kmap_local_page().
[Legacy documentation]
@@ -129,10 +131,11 @@ list shows them in order of preference of use.
* kmap(). This function has been deprecated; use kmap_local_page().
- NOTE: Conversions to kmap_local_page() must take care to follow the mapping
- restrictions imposed on kmap_local_page(). In particular, it is necessary to
- make sure that the kernel virtual memory pointer is only valid in the thread
- that obtained it.
+ .. note::
+ Conversions to kmap_local_page() must take care to follow the mapping
+ restrictions imposed on kmap_local_page(). In particular, it is necessary
+ to make sure that the kernel virtual memory pointer is only valid in the
+ thread that obtained it.
[Legacy documentation]
Thanks.
--
An old man doll... just what I always wanted! - Clara
On gioved? 8 dicembre 2022 05:22:37 CET Bagas Sanjaya wrote:
> On Wed, Dec 07, 2022 at 11:53:08PM +0100, Fabio M. De Francesco wrote:
> > diff --git a/Documentation/mm/highmem.rst b/Documentation/mm/highmem.rst
> > index 0f731d9196b0..59d1078f53df 100644
> > --- a/Documentation/mm/highmem.rst
> > +++ b/Documentation/mm/highmem.rst
> > @@ -57,7 +57,8 @@ list shows them in order of preference of use.
> >
> > 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.
> > + This function should always be used. kmap_atomic() and kmap() have been
> > + deprecated.
>
> "... always be used, whereas ..."
>
> > + NOTE: Conversions to kmap_local_page() must take care to follow the
> > mapping + restrictions imposed on kmap_local_page(). Furthermore, the
code
> > between + calls to kmap_atomic() and kunmap_atomic() may implicitly
depend
> > on the side + effects of atomic mappings, i.e. disabling page faults or
> > preemption, or both. + In that case, explicit calls to
pagefault_disable()
> > or preempt_disable() or + both must be made in conjunction with the use
of
> > kmap_local_page(). +
> > <snipped>...
> > + NOTE: Conversions to kmap_local_page() must take care to follow the
> > mapping + restrictions imposed on kmap_local_page(). In particular, it is
> > necessary to + make sure that the kernel virtual memory pointer is only
> > valid in the thread + that obtained it.
> > +
>
> What about using note block to signify conversion notes above?
>
> ---- >8 ----
>
> diff --git a/Documentation/mm/highmem.rst b/Documentation/mm/highmem.rst
> index 59d1078f53df57..ef53eb580d4cda 100644
> --- a/Documentation/mm/highmem.rst
> +++ b/Documentation/mm/highmem.rst
> @@ -103,12 +103,14 @@ list shows them in order of preference of use.
>
> * kmap_atomic(). This function has been deprecated; use kmap_local_page().
>
> - NOTE: Conversions to kmap_local_page() must take care to follow the
mapping
> - restrictions imposed on kmap_local_page(). Furthermore, the code between
-
> calls to kmap_atomic() and kunmap_atomic() may implicitly depend on the
side
> - effects of atomic mappings, i.e. disabling page faults or preemption, or
> both. - In that case, explicit calls to pagefault_disable() or
> preempt_disable() or - both must be made in conjunction with the use of
> kmap_local_page(). + .. note::
> + Conversions to kmap_local_page() must take care to follow the mapping
> + restrictions imposed on kmap_local_page(). Furthermore, the code
between
> + calls to kmap_atomic() and kunmap_atomic() may implicitly depend on
the
> + side effects of atomic mappings, i.e. disabling page faults or
> preemption, + or both. In that case, explicit calls to
> pagefault_disable() or + preempt_disable() or both must be made in
> conjunction with the use of + kmap_local_page().
>
> [Legacy documentation]
>
> @@ -129,10 +131,11 @@ list shows them in order of preference of use.
>
> * kmap(). This function has been deprecated; use kmap_local_page().
>
> - NOTE: Conversions to kmap_local_page() must take care to follow the
mapping
> - restrictions imposed on kmap_local_page(). In particular, it is necessary
> to - make sure that the kernel virtual memory pointer is only valid in the
> thread - that obtained it.
> + .. note::
> + Conversions to kmap_local_page() must take care to follow the mapping
> + restrictions imposed on kmap_local_page(). In particular, it is
> necessary + to make sure that the kernel virtual memory pointer is only
> valid in the + thread that obtained it.
>
> [Legacy documentation]
>
> Thanks.
>
> --
> An old man doll... just what I always wanted! - Clara
You provided valid suggestions, thanks!
However, immediately after submitting this v2 patch, Andrew added it to the -
mm mm-unstable branch.
I'll do the changes that you suggested in a subsequent patch which will build
on this.
Again thanks,
Fabio
On 12/9/22 03:05, Fabio M. De Francesco wrote:
> You provided valid suggestions, thanks!
> However, immediately after submitting this v2 patch, Andrew added it to the -
> mm mm-unstable branch.
> I'll do the changes that you suggested in a subsequent patch which will build
> on this.
>
OK, thanks!
--
An old man doll... just what I always wanted! - Clara