linux-kernel - Re: [RESEND PATCH] fs/aio: Replace kmap{,_atomic}() with kmap_local

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <x494jvztyx5.fsf@segfault.boston.devel.redhat.com>
Date:   Wed, 19 Oct 2022 15:07:34 -0400
From:   Jeff Moyer <jmoyer@...hat.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Benjamin LaHaise <bcrl@...ck.org>,
        linux-fsdevel@...r.kernel.org, linux-aio@...ck.org,
        linux-kernel@...r.kernel.org,
        "Venkataramanan\, Anirudh" <anirudh.venkataramanan@...el.com>,
        Ira Weiny <ira.weiny@...el.com>
Subject: Re: [RESEND PATCH] fs/aio: Replace kmap{,_atomic}() with kmap_local_page()

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

> On Wednesday, October 19, 2022 5:41:21 PM CEST Jeff Moyer wrote:
>> "Fabio M. De Francesco" <fmdefrancesco@...il.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@...hat.com>
>> 
>
> Thank you so much for the  "Reviewed-by" tag.
>
> Regards,
>
> Fabio