On martedì 26 aprile 2022 09:01:32 CEST Sebastian Andrzej Siewior wrote:
> On 2022-04-25 18:23:57 [+0200], Fabio M. De Francesco wrote:
> > index a77be5630209..aa22daeed617 100644
> > --- a/include/linux/highmem-internal.h
> > +++ b/include/linux/highmem-internal.h
> > @@ -236,9 +236,17 @@ static inline unsigned long totalhigh_pages(void)
{ return 0UL; }
> >
> > #endif /* CONFIG_HIGHMEM */
> >
> > -/*
> > - * Prevent people trying to call kunmap_atomic() as if it were
kunmap()
> > - * kunmap_atomic() should get the return value of kmap_atomic, not the
page.
> > +/**
> > + * kunmap_atomic - Unmap the virtual address mapped by kmap_atomic()
> > + * @__addr: Virtual address to be unmapped
> > + *
> > + * Unmaps an address previously mapped by kmap_atomic() and re-enables
> > + * pagefaults and preemption. Mappings should be unmapped in the
reverse
>
> You mind adding "Deprecated!" like kmap_atomic() has?
I might add "Deprecated!", however Ira Weiny asked me to rephrase an
earlier version of one of the patch which is is this series. I wrote that
"The use of kmap_atomic() is deprecated in favor of kmap_local_page()." and
Ira replied "I'm not sure deprecated is the right word. [] This series
should end up indicating the desire to stop growing kmap() and
kmap_atomic() call sites and that their deprecation is on the horizon.".
What Ira suggested is exactly what I'm doing in v2.
@Ira: what about adding "Deprecated!" for consistency with kmap_atomic()
kdoc?
> The part about
> disabling/ enabling preemption is true for !PREEMPT_RT.
To me it looks that this is not what Thomas Gleixner wrote in the cover
letter of his series ("[patch V2 00/18] mm/highmem: Preemptible variant of
kmap_atomic & friends") at
https://lore.kernel.org/lkml/20201029221806.189523375@xxxxxxxxxxxxx/
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
