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

[Fabio M. De Francesco]

On giovedì 28 aprile 2022 11:11:46 CEST Sebastian Andrzej Siewior wrote:
> On 2022-04-27 20:38:18 [+0200], Fabio M. De Francesco wrote:
> > --- a/include/linux/highmem-internal.h
> > +++ b/include/linux/highmem-internal.h
> > @@ -236,9 +236,18 @@ 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() - 
deprecated!
> > + * @__addr:       Virtual address to be unmapped
> > + *
> > + * Unmaps an address previously mapped by kmap_atomic() and re-enables
> > + * pagefaults, migration, preemption (the latter was disabled only for
> > + * !PREEMP_RT configurations). Mappings should be unmapped in the 
reverse
> 
> Not sure how detailed you want to put it here as "reverses kmap_atomic()
> doing." might be okay ;)

No, it's not sufficient because Matthew Wilcox said that something like "It 
is the counterpart of kmap_atomic() for unmapping" (or anything similar) is 
_not_ what he wants to see. 

Furthermore, a large part of this text has been written by him (I'm talking 
of a couple of weeks ago, when this patch was not part of this series - it 
was on its own until Ira Weiny asked me to gather 4 patches in one only 
series).

> This indicates the "migration" is disabled for
> !PREEMPT_RT which is not the case.

I read again how kmap_atomic() is defined. There are lots of 'if' 
statements. Only if the code gets to __kmap_local_pfn_prot(), users can be 
assured that it unconditionally calls both migrate_disable() and 
preempt_disable(). 

> So maybe something like
> 
>  * Unmaps an address previously mapped by kmap_atomic() and re-enables
>  * pagefaults, CPU migration (CONFIG_PREEMPT_RT) or preemption
>  * (!CONFIG_PREEMPT_RT). Mappings should be unmapped in the reverse
> 
> will make it clear.

I'm starting to think that this level of detail is too much for users who 
just need to understand how to use this function as well as  
kmap_local_page().

I prefer something like the following:

+ * Unmaps an address previously mapped by kmap_atomic() and re-enables
+ * pagefaults and possibly also CPU migration and/or preemption. However, 
+ * users should not count on disable of migration and/or preemption as a 
+ * side effect of calling kmap_atomic(). Mappings must be unmapped in the 
+ * reverse [...]

I'd also like to write the same paragraph for kmap_local_page().

What do you think of being less detailed and instead using the text I wrote 
above? 

Thanks,

Fabio