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
