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:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To:     Jeff Moyer <jmoyer@redhat.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()
Date:   Wed, 19 Oct 2022 20:52:04 +0200
Message-ID: <2851287.e9J7NaK4W3@mypc>
In-Reply-To: <x49h6zzvn1a.fsf@segfault.boston.devel.redhat.com>
References: <20221016150656.5803-1-fmdefrancesco@gmail.com> <x49h6zzvn1a.fsf@segfault.boston.devel.redhat.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"
Precedence: bulk

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 t=
he
> > 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, t=
he
> > 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 als=
o=20
see "It is faster", meaning "kmap_local_page() is faster".

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) that=
=20
these conversions (1) don't break any of the rules of use of local mapping=
=20
when converting kmap() (please read highmem.rst about these) and (2) the ca=
ll=20
sites of kmap_atomic() didn't rely on its side effects (pagefaults disable =
and=20
potential preemption disables).=20

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=
=20
the code, therefore the newer API is preferred".

I hope it makes my argument clearer.

>=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,

=46abio=20