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:     Matthew Wilcox <willy@infradead.org>, Song Liu <song@kernel.org>,
        Takashi Iwai <tiwai@suse.de>,
        Adam Manzanares <a.manzanares@samsung.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Ira Weiny <ira.weiny@intel.com>
Cc:     linux-modules@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] module: Replace kmap() with kmap_local_page()
Date:   Tue, 19 Jul 2022 11:19:24 +0200
Message-ID: <5303077.Sb9uPGUboI@opensuse>
In-Reply-To: <YtXchtEwetMvKrKY@bombadil.infradead.org>
References: <20220718002645.28817-1-fmdefrancesco@gmail.com> <YtXchtEwetMvKrKY@bombadil.infradead.org>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"
Precedence: bulk

On marted=C3=AC 19 luglio 2022 00:19:50 CEST Luis Chamberlain wrote:
> On Mon, Jul 18, 2022 at 02:26:45AM +0200, Fabio M. De Francesco wrote:
> > kmap() is being deprecated in favor of kmap_local_page().
> >=20
> > Two main problems with kmap(): (1) It comes with an overhead as mapping
> > space is restricted and protected by a global lock for synchronization=
=20
and
> > (2) it also requires global TLB invalidation when the kmap=E2=80=99s po=
ol wraps
> > and it might block when the mapping space is fully utilized until a=20
slot
> > becomes available.
>=20
> Neat!
>=20

Thanks!

> > With kmap_local_page() the mappings are per thread, CPU local, can take
> > page faults, and can be called from any context (including interrupts).
>=20
> Yes but the mapping can only be accessed from within this thread and the
> thread is bound to the CPU while the mapping is active. So, what if we
> get a preemption during decompression here? What happens?

No problems at all, please read commit f3ba3c710ac5 ("mm/highmem: Provide=20
kmap_local*") from Thomas Gleixner:

"[] Now that the kmap atomic index is stored in task struct provide a
preemptible variant. On context switch the maps of an outgoing task are
removed and the map of the incoming task are restored. That's obviously
slow, but highmem is slow anyway.
  =20
The kmap_local.*() functions can be invoked from both preemptible and
atomic context. kmap local sections disable migration to keep the resulting
virtual mapping address correct, but disable neither pagefaults nor
preemption.".

However, yours is a good question. It made me realize that this specific=20
topic is not discussed properly in highmem.rst (which also embeds kdocs=20
from highmem.h and highmem-internal.h).

Some weeks ago I reworked highmem.rst and, within a couple of days, I'll=20
also send a second round of additions and clean-ups to the documentation of=
=20
kmap_local_page().=20

I'll use this opportunity to explain more clearly what happens to the local=
=20
mappings when a task is preempted and then is restored. It looks like=20
currently this documentation lacks clarity about the fact that tasks which=
=20
use pointers returned by kmap_local_page() _can_ be preempted and that=20
these pointers are still valid when the tasks are rescheduled to run again.

In the meantime, I'll submit a v2 of this patch with a slightly extended=20
commit message which informs about this specific topic.

Again thanks for pointing it out that the commit message of this patch=20
should be more clear about what happens with preemption.

> > It is faster than kmap() in kernels with HIGHMEM enabled. Its use in
> > module_gzip_decompress() and in module_xz_decompress() is safe and
> > should be preferred.
> >=20
> > Therefore, replace kmap() with kmap_local_page().
>=20
> While this churn is going on everywhere I was wondering why not
> go ahead and adopt kmap_local_folio() instead?

I'm sorry but, due to my lack of knowledge and experience, I'm not sure to=
=20
understand how kmap_local_folio() could help here. My fault. I'm going to=20
make some research and ask for help from more experienced developers.=20

>   Luis
>=20
> > Tested on a QEMU/KVM x86_32 VM with 4GB RAM, booting kernels with
> > HIGHMEM64GB enabled. Modules compressed with XZ or GZIP decompress
> > properly.
> >=20
> > Suggested-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> >  kernel/module/decompress.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)

[snip]

Thanks,

=46abio