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

[Ira Weiny <ira.weiny@intel.com>]

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/20220419170649.1022246-1-ira.weiny@intel.com/

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