Re: [RESEND PATCH] fs/aio: Replace kmap{,_atomic}() with kmap_local_page()

[Jeff Moyer <jmoyer@redhat.com>]

"Fabio M. De Francesco" <fmdefrancesco@gmail.com> writes:

> On Wednesday, October 19, 2022 5:41:21 PM CEST Jeff Moyer wrote:
>> "Fabio M. De Francesco" <fmdefrancesco@gmail.com> writes:
>> 
>> > The use of kmap() and kmap_atomic() are being deprecated in favor of
>> > kmap_local_page().
>> >
>> > There are two main problems with kmap(): (1) It comes with an overhead as
>> > the mapping space is restricted and protected by a global lock for
>> > synchronization and (2) it also requires global TLB invalidation when the
>> > kmap’s pool wraps and it might block when the mapping space is fully
>> > utilized until a slot becomes available.
>> >
>> > With kmap_local_page() the mappings are per thread, CPU local, can take
>> > page faults, and can be called from any context (including interrupts).
>> > It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore,
>> > the tasks can be preempted and, when they are scheduled to run again, the
>> > kernel virtual addresses are restored and still valid.
>> >
>> > Since its use in fs/aio.c is safe everywhere, it should be preferred.
>> 
>> That sentence is very ambiguous.  I don't know what "its" refers to, and
>> I'm not sure what "safe" means in this context.
>
> I'm sorry for not being clearer.
>
> "its use" means "the use of kmap_local_page()". Few lines above you may also 
> see "It is faster", meaning "kmap_local_page() is faster".

Got it, thanks.

> The "safety" is a very concise way to assert that I've checked, by code 
> inspection and by testing (as it is better detailed some lines below) that 
> these conversions (1) don't break any of the rules of use of local mapping 
> when converting kmap() (please read highmem.rst about these) and (2) the call 
> sites of kmap_atomic() didn't rely on its side effects (pagefaults disable and 
> potential preemption disables). 

OK, good.  I agree that the aio code wasn't relying on the side effects of
kmap_atomic.

> Therefore, you may read it as it was: "The use of kmap_local_page() in fs/
> aio.c has been carefully checked to assure that the conversions won't break 
> the code, therefore the newer API is preferred".
>
> I hope it makes my argument clearer.

Yes, thank you for explaining!

-Jeff

>
>> 
>> The patch looks okay to me.
>> 
>> Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
>> 
>
> Thank you so much for the  "Reviewed-by" tag.
>
> Regards,
>
> Fabio