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.
Changes from v2 to v3:
1/4 - Add a deprecation notice to kunmap_atomic() for consistency
with the same notice in kmap_atomic() (Sebastian Andrzej
Siewior); shorten subject and extend commit message.
2/4 - No changes.
3/4 - No changes.
4/4 - Correct a technical inaccuracy about preemption disabling /
re-enabling in kmap_atomic() / kunmap_atomic() (Sebastian
Andrzej Siewior).
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: Move "Using kmap-atomic" to highmem.h
Documentation/vm: Rework "Temporary Virtual Mappings" section
Documentation/vm/highmem.rst | 104 +++++++++++++++++++------------
include/linux/highmem-internal.h | 15 ++++-
include/linux/highmem.h | 49 +++++++++++----
3 files changed, 112 insertions(+), 56 deletions(-)
--
2.34.1
`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, (3) using correct parameter names, (4)
adding description of return value of alloc_zeroed_user_highpage_movable(),
(5) correcting some technical inaccuracies in comments, and (5) adding a
deprecation notice in kunmap_atomic() for consistency with kmap_atomic().
Cc: Matthew Wilcox <[email protected]>
Cc: Sebastian Andrzej Siewior <[email protected]>
Acked-by: Mike Rapoport <[email protected]>
Reviewed-by: Ira Weiny <[email protected]>
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
include/linux/highmem-internal.h | 15 ++++++++++++---
include/linux/highmem.h | 20 ++++++--------------
2 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
index a77be5630209..9968a1282317 100644
--- a/include/linux/highmem-internal.h
+++ b/include/linux/highmem-internal.h
@@ -236,9 +236,18 @@ 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() - deprecated!
+ * @__addr: Virtual address to be unmapped
+ *
+ * Unmaps an address previously mapped by kmap_atomic() and re-enables
+ * pagefaults, migration, preemption (the latter was disabled only for
+ * !PREEMP_RT configurations). 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..3623319a659d 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.
@@ -138,24 +138,14 @@ static inline void *kmap_local_folio(struct folio *folio, size_t offset);
*
* Returns: The virtual address of the mapping
*
- * Effectively a wrapper around kmap_local_page() which disables pagefaults
- * and preemption.
+ * In fact a wrapper around kmap_local_page() which disables pagefaults,
+ * migration, preemption (the latter disabled only for !PREEMP_RT
+ * configurations).
*
* Do not use in new code. Use kmap_local_page() instead.
*/
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 +181,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
On giovedì 28 aprile 2022 11:11:46 CEST Sebastian Andrzej Siewior wrote:
> On 2022-04-27 20:38:18 [+0200], Fabio M. De Francesco wrote:
> > --- a/include/linux/highmem-internal.h
> > +++ b/include/linux/highmem-internal.h
> > @@ -236,9 +236,18 @@ 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() -
deprecated!
> > + * @__addr: Virtual address to be unmapped
> > + *
> > + * Unmaps an address previously mapped by kmap_atomic() and re-enables
> > + * pagefaults, migration, preemption (the latter was disabled only for
> > + * !PREEMP_RT configurations). Mappings should be unmapped in the
reverse
>
> Not sure how detailed you want to put it here as "reverses kmap_atomic()
> doing." might be okay ;)
No, it's not sufficient because Matthew Wilcox said that something like "It
is the counterpart of kmap_atomic() for unmapping" (or anything similar) is
_not_ what he wants to see.
Furthermore, a large part of this text has been written by him (I'm talking
of a couple of weeks ago, when this patch was not part of this series - it
was on its own until Ira Weiny asked me to gather 4 patches in one only
series).
> This indicates the "migration" is disabled for
> !PREEMPT_RT which is not the case.
I read again how kmap_atomic() is defined. There are lots of 'if'
statements. Only if the code gets to __kmap_local_pfn_prot(), users can be
assured that it unconditionally calls both migrate_disable() and
preempt_disable().
> So maybe something like
>
> * Unmaps an address previously mapped by kmap_atomic() and re-enables
> * pagefaults, CPU migration (CONFIG_PREEMPT_RT) or preemption
> * (!CONFIG_PREEMPT_RT). Mappings should be unmapped in the reverse
>
> will make it clear.
I'm starting to think that this level of detail is too much for users who
just need to understand how to use this function as well as
kmap_local_page().
I prefer something like the following:
+ * Unmaps an address previously mapped by kmap_atomic() and re-enables
+ * pagefaults and possibly also CPU migration and/or preemption. However,
+ * users should not count on disable of migration and/or preemption as a
+ * side effect of calling kmap_atomic(). Mappings must be unmapped in the
+ * reverse [...]
I'd also like to write the same paragraph for kmap_local_page().
What do you think of being less detailed and instead using the text I wrote
above?
Thanks,
Fabio
On 2022-04-27 20:38:18 [+0200], Fabio M. De Francesco wrote:
> --- a/include/linux/highmem-internal.h
> +++ b/include/linux/highmem-internal.h
> @@ -236,9 +236,18 @@ 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() - deprecated!
> + * @__addr: Virtual address to be unmapped
> + *
> + * Unmaps an address previously mapped by kmap_atomic() and re-enables
> + * pagefaults, migration, preemption (the latter was disabled only for
> + * !PREEMP_RT configurations). Mappings should be unmapped in the reverse
Not sure how detailed you want to put it here as "reverses kmap_atomic()
doing." might be okay ;) This indicates the "migration" is disabled for
!PREEMPT_RT which is not the case. So maybe something like
* Unmaps an address previously mapped by kmap_atomic() and re-enables
* pagefaults, CPU migration (CONFIG_PREEMPT_RT) or preemption
* (!CONFIG_PREEMPT_RT). Mappings should be unmapped in the reverse
will make it clear.
…
> --- a/include/linux/highmem.h
> +++ b/include/linux/highmem.h
> @@ -138,24 +138,14 @@ static inline void *kmap_local_folio(struct folio *folio, size_t offset);
> *
> * Returns: The virtual address of the mapping
> *
> - * Effectively a wrapper around kmap_local_page() which disables pagefaults
> - * and preemption.
> + * In fact a wrapper around kmap_local_page() which disables pagefaults,
> + * migration, preemption (the latter disabled only for !PREEMP_RT
> + * configurations).
and here.
Sebastian
On 2022-04-28 12:54:14 [+0200], Fabio M. De Francesco wrote:
> No, it's not sufficient because Matthew Wilcox said that something like "It
> is the counterpart of kmap_atomic() for unmapping" (or anything similar) is
> _not_ what he wants to see.
>
> Furthermore, a large part of this text has been written by him (I'm talking
> of a couple of weeks ago, when this patch was not part of this series - it
> was on its own until Ira Weiny asked me to gather 4 patches in one only
> series).
Sure.
> > This indicates the "migration" is disabled for
> > !PREEMPT_RT which is not the case.
>
> I read again how kmap_atomic() is defined. There are lots of 'if'
> statements. Only if the code gets to __kmap_local_pfn_prot(), users can be
> assured that it unconditionally calls both migrate_disable() and
> preempt_disable().
Right, that part. Then keep it.
> > So maybe something like
> >
> > * Unmaps an address previously mapped by kmap_atomic() and re-enables
> > * pagefaults, CPU migration (CONFIG_PREEMPT_RT) or preemption
> > * (!CONFIG_PREEMPT_RT). Mappings should be unmapped in the reverse
> >
> > will make it clear.
>
> I'm starting to think that this level of detail is too much for users who
> just need to understand how to use this function as well as
> kmap_local_page().
>
> I prefer something like the following:
>
> + * Unmaps an address previously mapped by kmap_atomic() and re-enables
> + * pagefaults and possibly also CPU migration and/or preemption. However,
> + * users should not count on disable of migration and/or preemption as a
> + * side effect of calling kmap_atomic(). Mappings must be unmapped in the
> + * reverse [...]
>
> I'd also like to write the same paragraph for kmap_local_page().
>
> What do you think of being less detailed and instead using the text I wrote
> above?
Sounds perfect.
> Thanks,
>
> Fabio
Sebastian