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

[Fabio M. De Francesco]

On martedì 26 aprile 2022 09:01:32 CEST Sebastian Andrzej Siewior wrote:
> On 2022-04-25 18:23:57 [+0200], Fabio M. De Francesco wrote:
> > index a77be5630209..aa22daeed617 100644
> > --- a/include/linux/highmem-internal.h
> > +++ b/include/linux/highmem-internal.h
> > @@ -236,9 +236,17 @@ static inline unsigned long totalhigh_pages(void) 
{ return 0UL; }
> >  
> >  #endif /* CONFIG_HIGHMEM */
> >  
> > -/*
> > - * Prevent people trying to call kunmap_atomic() as if it were 
kunmap()
> > - * kunmap_atomic() should get the return value of kmap_atomic, not the 
page.
> > +/**
> > + * kunmap_atomic - Unmap the virtual address mapped by kmap_atomic()
> > + * @__addr:       Virtual address to be unmapped
> > + *
> > + * Unmaps an address previously mapped by kmap_atomic() and re-enables
> > + * pagefaults and preemption. Mappings should be unmapped in the 
reverse
> 
> You mind adding "Deprecated!" like kmap_atomic() has? 

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

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

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

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

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