Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Jeff Moyer <jmoyer@redhat.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Cc:     Alexander Viro <viro@zeniv.linux.org.uk>,
        Benjamin LaHaise <bcrl@kvack.org>,
        linux-fsdevel@vger.kernel.org, linux-aio@kvack.org,
        linux-kernel@vger.kernel.org,
        "Venkataramanan\, Anirudh" <anirudh.venkataramanan@intel.com>,
        Ira Weiny <ira.weiny@intel.com>
Subject: Re: [RESEND PATCH] fs/aio: Replace kmap{,_atomic}() with kmap_local_page()
References: <20221016150656.5803-1-fmdefrancesco@gmail.com>
        <x49h6zzvn1a.fsf@segfault.boston.devel.redhat.com>
        <2851287.e9J7NaK4W3@mypc>
Date:   Wed, 19 Oct 2022 15:07:34 -0400
In-Reply-To: <2851287.e9J7NaK4W3@mypc> (Fabio M. De Francesco's message of
        "Wed, 19 Oct 2022 20:52:04 +0200")
Message-ID: <x494jvztyx5.fsf@segfault.boston.devel.redhat.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

"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:
>>=20
>> > 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=E2=80=99s 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.
>>=20
>> 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 a=
lso=20
> 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=20
> inspection and by testing (as it is better detailed some lines below) tha=
t=20
> these conversions (1) don't break any of the rules of use of local mappin=
g=20
> when converting kmap() (please read highmem.rst about these) and (2) the =
call=20
> sites of kmap_atomic() didn't rely on its side effects (pagefaults disabl=
e and=20
> potential preemption disables).=20

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 bre=
ak=20
> the code, therefore the newer API is preferred".
>
> I hope it makes my argument clearer.

Yes, thank you for explaining!

-Jeff

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