[PATCH v2 0/4] Extend and reorganize Highmem's documentation

[PATCH v2 0/4] Extend and reorganize Highmem's documentation

[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.

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

 Documentation/vm/highmem.rst     | 103 +++++++++++++++++++------------
 include/linux/highmem-internal.h |  14 ++++-
 include/linux/highmem.h          |  44 +++++++++----
 3 files changed, 107 insertions(+), 54 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h

[Fabio M. De Francesco]

`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, and (3) using correct parameter names.

Cc: Matthew Wilcox <willy@infradead.org>
Cc: Acked-by: Mike Rapoport <rppt@linux.ibm.com>
Cc: Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 include/linux/highmem-internal.h | 14 +++++++++++---
 include/linux/highmem.h          | 15 +++------------
 2 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
index a77be5630209..aa22daeed617 100644
--- a/include/linux/highmem-internal.h
+++ b/include/linux/highmem-internal.h
@@ -236,9 +236,17 @@ 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()
+ * @__addr:       Virtual address to be unmapped
+ *
+ * Unmaps an address previously mapped by kmap_atomic() and re-enables
+ * pagefaults and preemption. 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..3456dc1d38db 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.
@@ -145,17 +145,6 @@ static inline void *kmap_local_folio(struct folio *folio, size_t offset);
  */
 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 +180,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

[PATCH v2 2/4] Documentation/vm: Include kdocs into highmem.rst

[Fabio M. De Francesco]

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

[PATCH v2 3/4] Documentation/vm: Move section from highmem.rst to highmem.h

[Fabio M. De Francesco]

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 3456dc1d38db..6b2d59e025c5 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -142,6 +142,37 @@ static inline void *kmap_local_folio(struct folio *folio, size_t offset);
  * and preemption.
  *
  * 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

[PATCH v2 4/4] Documentation/vm: Rework "Temporary Virtual Mappings" section

[Fabio M. De Francesco]

Extend and rework the "Temporary Virtual Mappings" section of the highmem.rst
documentation.

Despite the local kmaps were introduced by Thomas Gleixner in October 2020,
documentation was still missing information about them. These additions rely
largely on Gleixner's patches, Jonathan Corbet's LWN articles, comments by
Ira Weiny and Matthew Wilcox, and in-code comments from
./include/linux/highmem.h.

1) Add a paragraph to document kmap_local_page().
2) Reorder the list of functions by decreasing order of preference of
use.
3) Rework part of the kmap() entry in list.

Cc: Jonathan Corbet <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 | 73 ++++++++++++++++++++++++++++++------
 1 file changed, 62 insertions(+), 11 deletions(-)

diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/highmem.rst
index e05bf5524174..b09f1f9a81f2 100644
--- a/Documentation/vm/highmem.rst
+++ b/Documentation/vm/highmem.rst
@@ -50,26 +50,77 @@ 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 per thread, 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. As the maps are
+  strictly CPU local, it is guaranteed that the task stays on the CPU and
+  that 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

Re: [PATCH v2 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h

[Sebastian Andrzej Siewior]

On 2022-04-25 18:23:57 [+0200], Fabio M. De Francesco wrote:
> index a77be5630209..aa22daeed617 100644
> --- a/include/linux/highmem-internal.h
> +++ b/include/linux/highmem-internal.h
> @@ -236,9 +236,17 @@ 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()
> + * @__addr:       Virtual address to be unmapped
> + *
> + * Unmaps an address previously mapped by kmap_atomic() and re-enables
> + * pagefaults and preemption. Mappings should be unmapped in the reverse

You mind adding "Deprecated!" like kmap_atomic() has? The part about
disabling/ enabling preemption is true for !PREEMPT_RT. The part that
worries me is that people use it and rely on disabled preemption like
some did in the past. 
I've been told this API is about to be removed (or so I have been told)
so I hope that it will be gone soon ;)

> + * 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 {								\

Sebastian

Re: [PATCH v2 4/4] Documentation/vm: Rework "Temporary Virtual Mappings" section

[Sebastian Andrzej Siewior]

On 2022-04-25 18:24:00 [+0200], Fabio M. De Francesco wrote:
> index e05bf5524174..b09f1f9a81f2 100644
> --- a/Documentation/vm/highmem.rst
> +++ b/Documentation/vm/highmem.rst
> @@ -50,26 +50,77 @@ 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.

feasible? It should always be used. I don't see a reason why using
kmap_local_page() would not be feasible.

> -* 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 per thread, 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. As the maps are
> +  strictly CPU local, it is guaranteed that the task stays on the CPU and

Maybe "thread local" instead CPU local? Another thread on the same CPU
can not use this mapping.

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

While they can be nested I wouldn't encourage that.

>  * 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.

Please add a note this function is deprecated and must not be used in
new code.

Sebastian

Re: [PATCH v2 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h

[Fabio M. De Francesco]

On martedì 26 aprile 2022 09:01:32 CEST Sebastian Andrzej Siewior wrote:
> On 2022-04-25 18:23:57 [+0200], Fabio M. De Francesco wrote:
> > index a77be5630209..aa22daeed617 100644
> > --- a/include/linux/highmem-internal.h
> > +++ b/include/linux/highmem-internal.h
> > @@ -236,9 +236,17 @@ 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()
> > + * @__addr:       Virtual address to be unmapped
> > + *
> > + * Unmaps an address previously mapped by kmap_atomic() and re-enables
> > + * pagefaults and preemption. Mappings should be unmapped in the 
reverse
> 
> You mind adding "Deprecated!" like kmap_atomic() has? 

I might add "Deprecated!", however Ira Weiny asked me to rephrase an 
earlier version of one of the patch which is is this series. I wrote that 
"The use of kmap_atomic() is deprecated in favor of kmap_local_page()." and 
Ira replied "I'm not sure deprecated is the right word. [] This series 
should end up indicating the desire to stop growing kmap() and
kmap_atomic() call sites and that their deprecation is on the horizon.".

What Ira suggested is exactly what I'm doing in v2. 

@Ira: what about adding "Deprecated!" for consistency with kmap_atomic() 
kdoc?

> The part about
> disabling/ enabling preemption is true for !PREEMPT_RT.

To me it looks that this is not what Thomas Gleixner wrote in the cover 
letter of his series ("[patch V2 00/18] mm/highmem: Preemptible variant of 
kmap_atomic & friends") at 
https://lore.kernel.org/lkml/20201029221806.189523375@linutronix.de/

For your convenience:

"[] there is not a real reason anymore to confine migration disabling to 
RT. [] Removing the RT dependency from migrate_disable/enable()".

Is there anything I'm still missing?

> The part that
> worries me is that people use it and rely on disabled preemption like
> some did in the past. 

This is something I'd prefer to hear also from other developers who are 
CC'ed for this patch :) 

> I've been told this API is about to be removed (or so I have been told)
> so I hope that it will be gone soon ;)
> 
> > + * 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 {								
\
> 
> Sebastian
> 

Thanks for your review,

Fabio

Re: [PATCH v2 4/4] Documentation/vm: Rework "Temporary Virtual Mappings" section

[Fabio M. De Francesco]

On martedì 26 aprile 2022 09:17:06 CEST Sebastian Andrzej Siewior wrote:
> On 2022-04-25 18:24:00 [+0200], Fabio M. De Francesco wrote:
> > index e05bf5524174..b09f1f9a81f2 100644
> > --- a/Documentation/vm/highmem.rst
> > +++ b/Documentation/vm/highmem.rst
> > @@ -50,26 +50,77 @@ 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.
> 
> feasible? It should always be used. 

No, it cannot always be used. Please read again few lines above this that 
"The mapping can only be used in the context which acquired them". We 
cannot do blind s/kmap/kmap_local_page/.

> I don't see a reason why using
> kmap_local_page() would not be feasible.

Ditto.

> > -* 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 per thread, 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. As the maps 
are
> > +  strictly CPU local, it is guaranteed that the task stays on the CPU 
and
> 
> Maybe "thread local" instead CPU local? Another thread on the same CPU
> can not use this mapping.
> 

Hmm, I might add "thread local" to convey that the local mappings should 
stay in the same context that acquired them. 

However, kmap_local_page() also disable migration. This is how Thomas 
Gleixner talks about kmap_local_page() in his patch where it introduced 
this function: 

"The kmap_local.*() functions can be invoked from both preemptible and
atomic context. kmap local sections disable migration to keep the resulting
virtual mapping address correct, but disable neither pagefaults nor
preemption.".

Therefore, if it "disable migration" it is "CPU local". I mean that I might 
also add "thread local" but I think (at least at this moment) that I won't 
remove "CPU local".

@Ira: what about this proposal?

> > +  that 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.
> 
> While they can be nested I wouldn't encourage that.

I'm not encouraging this kinds of usages. I'm only saying that "it is 
allowed to a certain extent".

> >  * 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.
> 
> Please add a note this function is deprecated and must not be used in
> new code.

I have already responded (in my other reply for 1/4) about the possible 
addition of a notice of deprecation. But, as said, I also need to take into 
account what other people think about it.

However, I agree that, since we have that "deprecated!" in the kdocs of  
kmap_atomic(), I should be consistent everywhere.

Please let me wait for more reviews before making further changes.

Thanks,

Fabio

> 
> Sebastian
> 

Re: [PATCH v2 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h

[Sebastian Andrzej Siewior]

On 2022-04-26 11:43:03 [+0200], Fabio M. De Francesco wrote:
> I might add "Deprecated!", however Ira Weiny asked me to rephrase an 
> earlier version of one of the patch which is is this series. I wrote that 
> "The use of kmap_atomic() is deprecated in favor of kmap_local_page()." and 
> Ira replied "I'm not sure deprecated is the right word. [] This series 
> should end up indicating the desire to stop growing kmap() and
> kmap_atomic() call sites and that their deprecation is on the horizon.".
> 
> What Ira suggested is exactly what I'm doing in v2. 
> 
> @Ira: what about adding "Deprecated!" for consistency with kmap_atomic() 
> kdoc?

I would prefer to keep the documentation symmetric.

> > The part about
> > disabling/ enabling preemption is true for !PREEMPT_RT.
> 
> To me it looks that this is not what Thomas Gleixner wrote in the cover 
> letter of his series ("[patch V2 00/18] mm/highmem: Preemptible variant of 
> kmap_atomic & friends") at 
> https://lore.kernel.org/lkml/20201029221806.189523375@linutronix.de/
> 
> For your convenience:
> 
> "[] there is not a real reason anymore to confine migration disabling to 
> RT. [] Removing the RT dependency from migrate_disable/enable()".
> 
> Is there anything I'm still missing?

Hmm. We had migrate_disable() initially limited to RT for a few reasons.
Then Linus complained about this and that and mentioned something about
Highmem is dying or not used that widely anymore (or so) and then the
local interface came up which required the migrate_disable() interface
to work for everyone. Back then the atomic interface should go away and
I remember that hch wanted to remove some of the callers from the DMA
API.
That is just on top of my head.

Looking at kmap_atomic() there is this:

| static inline void *kmap_atomic_prot(struct page *page, pgprot_t prot)
| {
|         if (IS_ENABLED(CONFIG_PREEMPT_RT))
|                 migrate_disable();
|         else
|                 preempt_disable();
| 
|         pagefault_disable();
|         return __kmap_local_page_prot(page, prot);
| }
| 
| static inline void *kmap_atomic(struct page *page)
| {
|         return kmap_atomic_prot(page, kmap_prot);
| }

as of v5.18-rc4. As you see, pagefaults are disabled for everyone. RT disables
migration only and !RT disables preemption. 
Internally __kmap_local_page_prot() ends up in __kmap_local_pfn_prot()
which uses migrate_disable() for the lifetime of the mapping. So it
disables additionally migration for the life time of the mapping but
preemption has been also disabled (and only for !RT).

We _could_ only disable migration in kmap_atomic_prot() for everyone but
we can't easily proof that none of the kmap_atomic() user rely on the
preempt-disable part. RT never disabled preemption here so it is safe to
assume that nothing on RT relies on that.

> > The part that
> > worries me is that people use it and rely on disabled preemption like
> > some did in the past. 
> 
> This is something I'd prefer to hear also from other developers who are 
> CC'ed for this patch :) 

Eitherway, according to the code kmap_atomic() does not always disable
preemption and the other comments around indicate that it is deprecated,
see commit
   f3ba3c710ac5a ("mm/highmem: Provide kmap_local*")
   https://git.kernel.org/torvalds/c/f3ba3c710ac5a

> Thanks for your review,
> 
> Fabio

Sebastian

Re: [PATCH v2 4/4] Documentation/vm: Rework "Temporary Virtual Mappings" section

[Sebastian Andrzej Siewior]

On 2022-04-26 12:45:12 [+0200], Fabio M. De Francesco wrote:
> > > +* 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.
> > 
> > feasible? It should always be used. 
> 
> No, it cannot always be used. Please read again few lines above this that 
> "The mapping can only be used in the context which acquired them". We 
> cannot do blind s/kmap/kmap_local_page/.

I'm sorry it seems I slipped while reading and replying.
The kmap_atomic() part has "should be only used if it is absolutely
required" … " otherwise kmap_local_page()". The kmap_atomic() should be
used in new code. The alternative kmap() and kmap_local*() should be
enough.

> > Maybe "thread local" instead CPU local? Another thread on the same CPU
> > can not use this mapping.
> > 
> 
> Hmm, I might add "thread local" to convey that the local mappings should 
> stay in the same context that acquired them. 
> 
> However, kmap_local_page() also disable migration. This is how Thomas 
> Gleixner talks about kmap_local_page() in his patch where it introduced 
> this function: 
> 
> "The kmap_local.*() functions can be invoked from both preemptible and
> atomic context. kmap local sections disable migration to keep the resulting
> virtual mapping address correct, but disable neither pagefaults nor
> preemption.".
> 
> Therefore, if it "disable migration" it is "CPU local". I mean that I might 
> also add "thread local" but I think (at least at this moment) that I won't 
> remove "CPU local".

Hmm. It is thread-local in the end. There are slots 0 … KM_MAX_IDX for
the mappings. Slot 0 for task A can be different from slot 0 for task B
while both run on CPU0. So the same address, that is returned from
kmap_local(), will point to a different page for both tasks. Both tasks
can't be migrated to another CPU while the mapping is active.
"CPU local" sounds like something that is same to everyone on the same
CPU which is what this_cpu_read() for instance does.

> @Ira: what about this proposal?
> 
> Thanks,
> 
> Fabio

Sebastian

Re: [PATCH v2 4/4] Documentation/vm: Rework "Temporary Virtual Mappings" section

[Fabio M. De Francesco]

On martedì 26 aprile 2022 13:47:34 CEST Sebastian Andrzej Siewior wrote:
> 
> Hmm. It is thread-local in the end. There are slots 0 … KM_MAX_IDX for
> the mappings. Slot 0 for task A can be different from slot 0 for task B
> while both run on CPU0. So the same address, that is returned from
> kmap_local(), will point to a different page for both tasks. Both tasks
> can't be migrated to another CPU while the mapping is active.
> "CPU local" sounds like something that is same to everyone on the same
> CPU which is what this_cpu_read() for instance does.
> 

OK, I agree with you :) 

I just got three notices from Greg K-H stating that he has applied three of 
my driver / Android patches. The patches are some conversions from kmap() 
and / or kmap_atomic() to kmap_local_page() (or wrappers around it):

https://lore.kernel.org/lkml/20220425175754.8180-4-fmdefrancesco@gmail.com/
https://lore.kernel.org/lkml/20220425175754.8180-3-fmdefrancesco@gmail.com/
https://lore.kernel.org/lkml/20220425175754.8180-2-fmdefrancesco@gmail.com/

I had forgotten that I wrote the following sentence in all three commit 
messages: "[] With kmap_local_page(), the mapping is per thread, CPU local 
and not globally visible. []"

Therefore, I'll add "thread-local" or "per thread". I probably like your 
wording more than mine: "thread-local" is more suitable.

For consistency (again) I like the other change you proposed, which is to 
add "deprecated!" also in kunmap_atomic(), exactly as it is already in 
kmap_atomic() kernel-docs.

However, I will wait one more day before sending v3, in case there are 
other people who want to suggest further changes.

If I remember correctly, I'm overlooking nothing else. Do I overlook 
something?

Thanks for your help,

Fabio

Re: [PATCH v2 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h

[Fabio M. De Francesco]

On martedì 26 aprile 2022 13:04:42 CEST Sebastian Andrzej Siewior wrote:
> On 2022-04-26 11:43:03 [+0200], Fabio M. De Francesco wrote:
> 
> Either way, according to the code kmap_atomic() does not always disable
> preemption 

Hi Sebastian,

In my last email (for patch 4/4) I wrote that I would add a deprecation 
notice to kunmap_local() and would talk about thread locality in 
kmap_local_page(). I want to confirm that I'll do these changes in v3.

Then I closed that email asking if I was still overlooking something. 
Consider that I'm relatively new to kernel development and that it's just 
something I do in my spare time. So I was pretty sure I was still missing 
something :)

After reading again the code of kmap_atomic() (as you suggested - thanks!) 
I noted that you correctly say that kmap_atomic() does not always disables 
preemption (i.e., preemption is disabled only for !PREEMPT_RT kernel's  
configurations).

Therefore I'll also change the first sentence of kunmap_local() to the 
following:

"[It] Unmaps an address previously mapped by kmap_atomic() and re-enables 
pagefaults and preemption (the latter disabled only for !PREEMP_RT 
configurations).".

I will also be making this change in v3.

Can you please say if I'm still missing something?

Thanks,

Fabio

Re: [PATCH v2 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h

[Ira Weiny]

On Tue, Apr 26, 2022 at 09:01:32AM +0200, Sebastian Andrzej Siewior wrote:
> On 2022-04-25 18:23:57 [+0200], Fabio M. De Francesco wrote:
> > index a77be5630209..aa22daeed617 100644
> > --- a/include/linux/highmem-internal.h
> > +++ b/include/linux/highmem-internal.h
> > @@ -236,9 +236,17 @@ 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()
> > + * @__addr:       Virtual address to be unmapped
> > + *
> > + * Unmaps an address previously mapped by kmap_atomic() and re-enables
> > + * pagefaults and preemption. Mappings should be unmapped in the reverse
> 
> You mind adding "Deprecated!" like kmap_atomic() has? The part about
> disabling/ enabling preemption is true for !PREEMPT_RT. The part that
> worries me is that people use it and rely on disabled preemption like
> some did in the past. 
> I've been told this API is about to be removed (or so I have been told)
> so I hope that it will be gone soon ;)

I think some discussion needs to happen around this API.

Highmem has little use.  I don't think anyone disagrees with Linus there.
(Although I think there are still a few users out there.)

kmap may be a poor name for an API without the highmem functionality.  But
perhaps not.  One could interpret it to mean simply getting the kernel mapping
of the page rather than creating one.  After all that is what 64bit has done
all along.

This interpretation helps when you consider features which attempt to layer the
direct map with additional protections like PKS.[1]  Those protections mean
that a simple page_address() is insufficient to access the direct map.

As far as calling kmap() and kmap_atomic() deprecated I'm ok with that if the
community is.

The current kmap() call sites need work and Fabio's work on auditing them is
extremely helpful.  That said, if we officially deprecate kmap_atomic() then
those sites could be added to the list for rework.

Ira

[1] https://lore.kernel.org/lkml/20220419170649.1022246-1-ira.weiny@intel.com/

> 
> > + * 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 {								\
> 
> Sebastian

Re: [PATCH v2 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h

[Sebastian Andrzej Siewior]

On 2022-04-29 08:59:26 [-0700], Ira Weiny wrote:
> I think some discussion needs to happen around this API.
> 
> Highmem has little use.  I don't think anyone disagrees with Linus there.
> (Although I think there are still a few users out there.)

arm32 is still built and they have sometimes 1 - 2 GiB of memory.

> kmap may be a poor name for an API without the highmem functionality.  But
> perhaps not.  One could interpret it to mean simply getting the kernel mapping
> of the page rather than creating one.  After all that is what 64bit has done
> all along.
> 
> This interpretation helps when you consider features which attempt to layer the
> direct map with additional protections like PKS.[1]  Those protections mean
> that a simple page_address() is insufficient to access the direct map.
> 
> As far as calling kmap() and kmap_atomic() deprecated I'm ok with that if the
> community is.
> 
> The current kmap() call sites need work and Fabio's work on auditing them is
> extremely helpful.  That said, if we officially deprecate kmap_atomic() then
> those sites could be added to the list for rework.

Maybe I oversee something obvious but there is no problem with removing
kmap_atomic*() and keeping only kmap_local*() around, is there?
I never intended to deprecated kmap(), only kmap_atomic*() in favour of
kmap_local*().

> Ira

Sebastian

Re: [PATCH v2 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h

[Ira Weiny]

On Wed, May 25, 2022 at 11:34:16AM +0200, Sebastian Andrzej Siewior wrote:
> On 2022-04-29 08:59:26 [-0700], Ira Weiny wrote:
> > I think some discussion needs to happen around this API.
> > 
> > Highmem has little use.  I don't think anyone disagrees with Linus there.
> > (Although I think there are still a few users out there.)
> 
> arm32 is still built and they have sometimes 1 - 2 GiB of memory.

Yep :-) I was thinking of arm when I said this.

> 
> > kmap may be a poor name for an API without the highmem functionality.  But
> > perhaps not.  One could interpret it to mean simply getting the kernel mapping
> > of the page rather than creating one.  After all that is what 64bit has done
> > all along.
> > 
> > This interpretation helps when you consider features which attempt to layer the
> > direct map with additional protections like PKS.[1]  Those protections mean
> > that a simple page_address() is insufficient to access the direct map.
> > 
> > As far as calling kmap() and kmap_atomic() deprecated I'm ok with that if the
> > community is.
> > 
> > The current kmap() call sites need work and Fabio's work on auditing them is
> > extremely helpful.  That said, if we officially deprecate kmap_atomic() then
> > those sites could be added to the list for rework.
> 
> Maybe I oversee something obvious but there is no problem with removing
> kmap_atomic*() and keeping only kmap_local*() around, is there?

No there is not.  But some kmap_atomic() sites may have to open code the
preempt_disable() while others may not.

I have not done a full audit of the kmap_atomic() sites but I suspect most
don't really need the preempt_disable() but many may need to.  I just don't
know.

Regardless marking it deprecated can stop the growth of kmap_atomic() calls.

> I never intended to deprecated kmap(), only kmap_atomic*() in favour of
> kmap_local*().

Ok.  But I do want to see kmap() use removed.  The PKS code can't work with
kmap() and in general we are seeing more and more restrictions on the direct
map which may or may not be compatible with kmap().  What I presented at LSFmm
was to turn the kmap* interfaces into a more generic 'get me a temp kernel
address' interface instead of a highmem interface.

Any user who needs a long term address will need something other than kmap().
To that end there was some discussion on making vmap() more efficient or other
alternatives.

First we need to focus on reducing the kmap() call sites.  This documentation
change, making kmap() deprecated, will help ensure the kernel does not grow
more of them.

Ira

> 
> > Ira
> 
> Sebastian