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

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

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

 Documentation/vm/highmem.rst     | 101 +++++++++++++++++++------------
 include/linux/highmem-internal.h |  14 ++++-
 include/linux/highmem.h          |  15 +----
 3 files changed, 75 insertions(+), 55 deletions(-)

-- 
2.34.1

[PATCH 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: Mike Rapoport <rppt@linux.ibm.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Suggested-by: Matthew Wilcox <willy@infradead.org>
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..b099a08e29d3 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
+ *
+ * Unmap 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..c3d562b5f0c1 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:	Virtual address to be unmapped
  *
  * 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 2/4] Documentation/vm: Include kdocs from highmem*.h 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: Ira Weiny <ira.weiny@intel.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Peter Zijlstra <peterz@infradead.org>
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 3/4] Documentation/vm: Remove "Using kmap-atomic" from highmem.rst.

[Fabio M. De Francesco]

The use of kmap_atomic() is deprecated in favor of kmap_local_page(). For
this reason the "Using kmap_atomic" section in highmem.rst is obsolete and
unnecessary.

Therefore, just remove it.

Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 Documentation/vm/highmem.rst | 35 -----------------------------------
 1 file changed, 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
 ==========================
 
-- 
2.34.1

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

[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: Ira Weiny <ira.weiny@intel.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 Documentation/vm/highmem.rst | 71 ++++++++++++++++++++++++++++++------
 1 file changed, 60 insertions(+), 11 deletions(-)

diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/highmem.rst
index e05bf5524174..960f61e7a552 100644
--- a/Documentation/vm/highmem.rst
+++ b/Documentation/vm/highmem.rst
@@ -50,26 +50,75 @@ 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.
+
+  It is assumed that kmap_local_page() always returns the virtual address
+  of the mapping, therefore they won't ever 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.
 
 * 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 more 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 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h

[Mike Rapoport]

On Thu, Apr 21, 2022 at 08:01:57PM +0200, Fabio M. De Francesco wrote:
> `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: Mike Rapoport <rppt@linux.ibm.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> 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..b099a08e29d3 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
> + *
> + * Unmap an address previously mapped by kmap_atomic() and re-enables

Unmap ... and re-enable

or 

Unmaps ... and re-enables

Other than that

Acked-by: Mike Rapoport <rppt@linux.ibm.com>

> + * 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..c3d562b5f0c1 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:	Virtual address to be unmapped
>   *
>   * 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
> 
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH 2/4] Documentation/vm: Include kdocs from highmem*.h into highmem.rst

[Mike Rapoport]

On Thu, Apr 21, 2022 at 08:01:58PM +0200, Fabio M. De Francesco wrote:
> 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: Ira Weiny <ira.weiny@intel.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Acked-by: Mike Rapoport <rppt@linux.ibm.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
> 
> 

-- 
Sincerely yours,
Mike.

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

[Fabio M. De Francesco]

On venerdì 22 aprile 2022 10:24:14 CEST Mike Rapoport wrote:
> On Thu, Apr 21, 2022 at 08:01:57PM +0200, Fabio M. De Francesco wrote:
> > `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: Mike Rapoport <rppt@linux.ibm.com>
> > Cc: Ira Weiny <ira.weiny@intel.com>
> > Suggested-by: Matthew Wilcox <willy@infradead.org>
> > 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(-)
> >
> > [...]
> >
> > + *
> > + * Unmap an address previously mapped by kmap_atomic() and re-enables
> 
> Unmap ... and re-enable
> 
> or 
> 
> Unmaps ... and re-enables

Sorry, I should have read it twice before submitting :(

This entire series has already been taken by Andrew Morton for "-mm" 
immediately after submission. I think that probably the better suited 
solution is to send a correction when they show upstream. 

Do you agree with me or you prefer that I resubmit the whole series as a v2 
now?

> 
> Other than that
> 
> Acked-by: Mike Rapoport <rppt@linux.ibm.com>
> 

I also saw your "Acked-by" tag in patch 2/4. Thanks!

Regards,

Fabio

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

[Mike Rapoport]

On Fri, Apr 22, 2022 at 11:36:28AM +0200, Fabio M. De Francesco wrote:
> On venerdì 22 aprile 2022 10:24:14 CEST Mike Rapoport wrote:
> > On Thu, Apr 21, 2022 at 08:01:57PM +0200, Fabio M. De Francesco wrote:
> > > `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: Mike Rapoport <rppt@linux.ibm.com>
> > > Cc: Ira Weiny <ira.weiny@intel.com>
> > > Suggested-by: Matthew Wilcox <willy@infradead.org>
> > > 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(-)
> > >
> > > [...]
> > >
> > > + *
> > > + * Unmap an address previously mapped by kmap_atomic() and re-enables
> > 
> > Unmap ... and re-enable
> > 
> > or 
> > 
> > Unmaps ... and re-enables
> 
> Sorry, I should have read it twice before submitting :(
> 
> This entire series has already been taken by Andrew Morton for "-mm" 
> immediately after submission. I think that probably the better suited 
> solution is to send a correction when they show upstream. 

You can send a correction as an incremental patch against mmotm tree that's
mirrored here:

https://github.com/hnaz/linux-mm

I believe Andrew will add it to his mmotm queue.
 
> Do you agree with me or you prefer that I resubmit the whole series as a v2 
> now?
> 
> > 
> > Other than that
> > 
> > Acked-by: Mike Rapoport <rppt@linux.ibm.com>
> > 
> 
> I also saw your "Acked-by" tag in patch 2/4. Thanks!
> 
> Regards,
> 
> Fabio
> 
> 

-- 
Sincerely yours,
Mike.

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

[Ira Weiny]

On Thu, Apr 21, 2022 at 08:01:57PM +0200, Fabio M. De Francesco wrote:
> `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: Mike Rapoport <rppt@linux.ibm.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> 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..b099a08e29d3 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
> + *
> + * Unmap 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..c3d562b5f0c1 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:	Virtual address to be unmapped
                ^^^^^^^^^^^^^^^
		Page

Not only was the name wrong but the description of an address is wrong.

Other than that LGTM:

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

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

Re: [PATCH 2/4] Documentation/vm: Include kdocs from highmem*.h into highmem.rst

[Ira Weiny]

On Thu, Apr 21, 2022 at 08:01:58PM +0200, Fabio M. De Francesco wrote:
> 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: Ira Weiny <ira.weiny@intel.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.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
> 

Re: [PATCH 3/4] Documentation/vm: Remove "Using kmap-atomic" from highmem.rst.

[Ira Weiny]

On Thu, Apr 21, 2022 at 08:01:59PM +0200, Fabio M. De Francesco wrote:
> The use of kmap_atomic() is deprecated in favor of kmap_local_page().

I'm not sure deprecated is the right word.  And I think the fact that this
documentation is stale is a better reason for the patch as is.

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.  I've
not read the text in patch 4/4 yet.

> For
> this reason the "Using kmap_atomic" section in highmem.rst is obsolete and
> unnecessary.

A lot of the text is obsolete (and redundant) but the example code might be
useful.

Why not move the example and relevant bits into the kdoc for kmap_atomic()
which is then automatically picked up via patch 2/4.

Ira

> 
> Therefore, just remove it.
> 
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  Documentation/vm/highmem.rst | 35 -----------------------------------
>  1 file changed, 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
>  ==========================
>  
> -- 
> 2.34.1
> 

Re: [PATCH 3/4] Documentation/vm: Remove "Using kmap-atomic" from highmem.rst.

[Fabio M. De Francesco]

On venerdì 22 aprile 2022 20:38:09 CEST Ira Weiny wrote:
> On Thu, Apr 21, 2022 at 08:01:59PM +0200, Fabio M. De Francesco wrote:
> > The use of kmap_atomic() is deprecated in favor of kmap_local_page().
> 
> I'm not sure deprecated is the right word.

OK, in v2 I won't use "deprecated". Instead I'll say something about a 
strong preference to avoid its use. The reason why developers should avoid 
kmap_atomic() are explained in 4/4 (I've copy-pasted some lines here for 
your convenience):

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

> And I think the fact that this
> documentation is stale is a better reason for the patch as is.
> 
> 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.  
I've
> not read the text in patch 4/4 yet.

I'll wait for your review of 4/4 before sending v2.

> 
> > For
> > this reason the "Using kmap_atomic" section in highmem.rst is obsolete 
and
> > unnecessary.
> 
> A lot of the text is obsolete (and redundant) but the example code might 
be
> useful.
> 
> Why not move the example and relevant bits into the kdoc for 
kmap_atomic()
> which is then automatically picked up via patch 2/4.

Yes, I agree with you. I'll take into account your suggestion for v2.

However, as I said above, I'll hold v2 until you have time to review 4/4 
for the purpose to not miss any changes that you might require for that 
patch too.

Furthermore, while working on v2, I think that I'll extend this series with 
one or two patch more, in order to address other issues I noticed. 

Thanks,

Fabio 

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

[Fabio M. De Francesco]

(I don't know why it looks like my previous reply has not been sent and it 
also disappeared from drafts. I apologize if for whatever reason you 
receive this message twice)

On venerdì 22 aprile 2022 20:08:46 CEST Ira Weiny wrote:
> On Thu, Apr 21, 2022 at 08:01:57PM +0200, Fabio M. De Francesco wrote:
>
> [snip]
>
> > diff --git a/include/linux/highmem.h b/include/linux/highmem.h
> > index 39bb9b47fa9c..c3d562b5f0c1 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:	Virtual address to be unmapped
>                 ^^^^^^^^^^^^^^^
> 		Page
> 
> Not only was the name wrong but the description of an address is wrong.

Yes, correct. I'll rewrite this line the following way and send a v2 of 
this series while addressing also what it is required by your review of 3/4   
(unless you have better suited suggestions):

- * @addr:	 	Virtual address to be unmapped 
+ * @page:	 	Pointer to the page which was mapped by kmap()

> Other than that LGTM:
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Thanks!

Fabio

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

[Ira Weiny]

On Thu, Apr 21, 2022 at 08:02:00PM +0200, Fabio M. De Francesco wrote:
> 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: Ira Weiny <ira.weiny@intel.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  Documentation/vm/highmem.rst | 71 ++++++++++++++++++++++++++++++------
>  1 file changed, 60 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/highmem.rst
> index e05bf5524174..960f61e7a552 100644
> --- a/Documentation/vm/highmem.rst
> +++ b/Documentation/vm/highmem.rst
> @@ -50,26 +50,75 @@ 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.
> +
> +  It is assumed that kmap_local_page() always returns the virtual address

kmap_local_page() does return a kernel virtual address.  Why 'assume' this?

Do you mean it returns an address in the direct map?

> +  of the mapping, therefore they won't ever fail.

I don't think that returning a virtual address has anything to do with the
assumption they will not fail.

Why do you say this?

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

I think I would reference the kmap_local_page() for more details on the
ordering because there have been some conversions I've done which were
complicated by this.

>  
>  * 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 more needed, the address that
                                         ^^^^^^^^
					 no longer

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

Nice!

Ira

> +
> +  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 4/4] Documentation/vm: Rework "Temporary Virtual Mappings"

[Fabio M. De Francesco]

On lunedì 25 aprile 2022 02:59:48 CEST Ira Weiny wrote:
> On Thu, Apr 21, 2022 at 08:02:00PM +0200, Fabio M. De Francesco wrote:
> > 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: Ira Weiny <ira.weiny@intel.com>
> > Cc: Matthew Wilcox <willy@infradead.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> >  Documentation/vm/highmem.rst | 71 ++++++++++++++++++++++++++++++------
> >  1 file changed, 60 insertions(+), 11 deletions(-)
> > 
> > diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/
highmem.rst
> > index e05bf5524174..960f61e7a552 100644
> > --- a/Documentation/vm/highmem.rst
> > +++ b/Documentation/vm/highmem.rst
> > @@ -50,26 +50,75 @@ 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.
> > +
> > +  It is assumed that kmap_local_page() always returns the virtual 
address
> 
> kmap_local_page() does return a kernel virtual address.  Why 'assume' 
this?
> 
> Do you mean it returns an address in the direct map?
> 
> > +  of the mapping, therefore they won't ever fail.
> 
> I don't think that returning a virtual address has anything to do with 
the
> assumption they will not fail.
> 
> Why do you say this?

Oh, sorry! I didn't mean to say this. What I wrote is _not_ what I meant. 
My intention was to say the same that you may read below about 
k[un]map_atomic().

This sentence should have been:

+ It always returns a valid virtual address. It is assumed that
+ k[un]map_local() won't ever fail.

Is this rewording correct?

It's not my first time I make these kinds of silly mistakes when copy-
pasting lines and then rework or merge with other text that was already 
there. Recently I've made a couple of these kinds of mistakes.

I'd better read twice (maybe thrice) what I write before sending :(

> 
> > +
> > +  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.
> 
> I think I would reference the kmap_local_page()

I suppose you are talking about the kdocs comments in code. If so, please 
remember that the kmap_local_page() kdocs have already been included in  
highmem.rst.

Am I misunderstanding what you write?

> for more details on the
> ordering because there have been some conversions I've done which were
> complicated by this.
> 
> >  
> >  * 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 more needed, the address 
that
>                                          ^^^^^^^^
> 					 no longer

Yes, correct. I'll fix it.

> > +  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.
> 
> Nice!

Now that I have your reviews for all the four patches of this series I'll 
send next version on Monday.

Thanks you so much,

Fabio

> 
> Ira
> 
> > +
> > +  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 4/4] Documentation/vm: Rework "Temporary Virtual Mappings"

[Fabio M. De Francesco]

On lunedì 25 aprile 2022 03:42:46 CEST Fabio M. De Francesco wrote:
> On lunedì 25 aprile 2022 02:59:48 CEST Ira Weiny wrote:
>
> [...]
> 
> > Why do you say this?
> 
> Oh, sorry! I didn't mean to say this. What I wrote is _not_ what I meant. 
> My intention was to say the same that you may read below about 
> k[un]map_atomic().
> 
> This sentence should have been:
> 
> + It always returns a valid virtual address. It is assumed that
> + k[un]map_local() won't ever fail.

These two sentences should be better rephrased as the following text...

+ kmap_local_page() always returns a valid virtual address. It is assumed 
+ that kunmap_local() won't fail.

Thanks,

Fabio

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

[Ira Weiny]

On Mon, Apr 25, 2022 at 03:42:46AM +0200, Fabio M. De Francesco wrote:
> On lunedì 25 aprile 2022 02:59:48 CEST Ira Weiny wrote:
> > On Thu, Apr 21, 2022 at 08:02:00PM +0200, Fabio M. De Francesco wrote:
> > > 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: Ira Weiny <ira.weiny@intel.com>
> > > Cc: Matthew Wilcox <willy@infradead.org>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > ---
> > >  Documentation/vm/highmem.rst | 71 ++++++++++++++++++++++++++++++------
> > >  1 file changed, 60 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/
> highmem.rst
> > > index e05bf5524174..960f61e7a552 100644
> > > --- a/Documentation/vm/highmem.rst
> > > +++ b/Documentation/vm/highmem.rst
> > > @@ -50,26 +50,75 @@ 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.
> > > +
> > > +  It is assumed that kmap_local_page() always returns the virtual 
> address
> > 
> > kmap_local_page() does return a kernel virtual address.  Why 'assume' 
> this?
> > 
> > Do you mean it returns an address in the direct map?
> > 
> > > +  of the mapping, therefore they won't ever fail.
> > 
> > I don't think that returning a virtual address has anything to do with 
> the
> > assumption they will not fail.
> > 
> > Why do you say this?
> 
> Oh, sorry! I didn't mean to say this. What I wrote is _not_ what I meant. 
> My intention was to say the same that you may read below about 
> k[un]map_atomic().
> 
> This sentence should have been:
> 
> + It always returns a valid virtual address. It is assumed that
> + k[un]map_local() won't ever fail.
> 
> Is this rewording correct?
> 
> It's not my first time I make these kinds of silly mistakes when copy-
> pasting lines and then rework or merge with other text that was already 
> there. Recently I've made a couple of these kinds of mistakes.
> 
> I'd better read twice (maybe thrice) what I write before sending :(

NP  That is part of the reason we have reviews.

> 
> > 
> > > +
> > > +  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.
> > 
> > I think I would reference the kmap_local_page()
> 
> I suppose you are talking about the kdocs comments in code. If so, please 
> remember that the kmap_local_page() kdocs have already been included in  
> highmem.rst.

Yes exactly.

> 
> Am I misunderstanding what you write?

I was just suggesting that the above could add.

'See kmal_local_page() kdoc for ordering details.'

To make sure that people understand those details and you don't have to rewrite
the kdoc stuff here.

> 
> > for more details on the
> > ordering because there have been some conversions I've done which were
> > complicated by this.
> > 
> > >  
> > >  * 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 more needed, the address 
> that
> >                                          ^^^^^^^^
> > 					 no longer
> 
> Yes, correct. I'll fix it.
> 
> > > +  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.
> > 
> > Nice!
> 
> Now that I have your reviews for all the four patches of this series I'll 
> send next version on Monday.
> 
> Thanks you so much,

Thank you!
Ira

> 
> Fabio
> 
> > 
> > Ira
> > 
> > > +
> > > +  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
> > > 
> > 
> 
> 
> 
>