`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 <[email protected]>
Cc: Acked-by: Mike Rapoport <[email protected]>
Cc: Reviewed-by: Ira Weiny <[email protected]>
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
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
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
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/[email protected]/
>
> 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
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/[email protected]/
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
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
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/[email protected]/
>
> > + * 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
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
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