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

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

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/20201029221806.189523375@linutronix.de/
> 
> 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