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:     Keith Busch <kbusch@kernel.org>, Sagi Grimberg <sagi@grimberg.me>,
        Christoph Hellwig <hch@lst.de>,
        Chaitanya Kulkarni <kch@nvidia.com>
Cc:     linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org,
        Ira Weiny <ira.weiny@intel.com>
Subject: Re: [RFC PATCH] nvmet-tcp: Don't kmap() pages which can't come from HIGHMEM
Date:   Wed, 17 Aug 2022 14:02:54 +0200
Message-ID: <3177256.oiGErgHkdL@opensuse>
In-Reply-To: <c11e62ae-3439-e3e3-adba-d7f5f951791c@grimberg.me>
References: <20220816091808.23236-1-fmdefrancesco@gmail.com> <YvvpJbOT9iv6rnm2@kbusch-mbp.dhcp.thefacebook.com> <c11e62ae-3439-e3e3-adba-d7f5f951791c@grimberg.me>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"
Precedence: bulk

On mercoled=C3=AC 17 agosto 2022 11:44:09 CEST Sagi Grimberg wrote:
> >> Therefore, I have two questions: am I right about thinking that the pa=
ges
> >> mapped in nvmet_tcp_map_pdu_iovec() are allocated with GFP_KERNEL?
> >=20
> > I think you are correct.
>=20
> It is correct. It is the same model for the linux scsi target, sunrpc
> etc.

I'll try to address the comments from the two last messages from Keith and=
=20
Sagi with this email (I replied yesterday to Chaitanya).

=46irst of all: good to know that it is the same model for other subsystem.=
 This=20
is useful to know. Thanks!

> >> If so, can anyone with more knowledge than mine please say if my chang=
es
> >> make
> >> any sense?
> >=20
> > I think it does make sense.

Thanks, I'm glad I was not wrong :-)

> > I like the code simplification, though this use
> > was't really paying the kmap penalty since, as you mentioned, this is=20
never
> > highmem.

Correct, however everybody like code simplification. I added a couple of=20
sentences to kmap_local_page() documentation in highmem.rst. They clearly=20
state that, when users know that pages cannot come from Highmem, they may=20
better prefer page_address().

The changes to nvmet-tcp started with trying to convert kmap() / kunmap() t=
o=20
kmap_local_page() /kunmap_local(), but it ended up to code shortening and=20
simplification with a plain use of page_address().=20

Obviously, due to my little experience with kernel developing and less than=
=20
little knowledge of this protocol I had to ask whether or not I was right i=
n=20
identifying the site of the allocations.

The reasons why I had to use page_address() will be clearer reading what=20
follows...

> Yes, its the same code-path. Would be great if we still had an
> abstraction that would do the right thing regardless of highmem or
> not like kmap provides though.

It would be great and it is already possible (this is why Thomas Gleixner=20
created this kmap_local_page() API) but here we have a huge issue. kmap() a=
nd=20
kmap_atomic() have recently been deprecated and they shouldn't any longer b=
e=20
used in new code: https://lore.kernel.org/all/20220813220034.806698-1-ira.w=
einy@intel.com/ ("[PATCH] checkpatch: Add kmap and kmap_atomic to the=20
deprecated list").

kmap_local_page() always does the right thing: users can call it with or=20
without HIGHMEM enabled, in-atomic (also in interrupts) or in preemptible=20
contexts, they can take page faults.=20

It doesn't require global lock for synchronization and doesn't require glob=
al=20
TLB invalidation when the kmap's pool wraps and doesn't block waiting for f=
ree=20
slots.=20

Nice, isn't it?

However, with nvmet-tcp we cannot easily use kmap_local_page() because it=20
comes with a major problem: it's local to the thread. If users handed the=20
kernel virtual addresses returned by this function to other threads, the=20
pointers would be invalid.

Here kmap() and kunmap() call sites are in two different workqueues.=20
Therefore, there is no way to convert kmap() to kmap_local_page(), unless t=
his=20
code is heavily refactored.

Knowing that the pages cannot come from Highmem avoids this refactoring and=
 in=20
the meantime it allows us to delete the kmap() and kunmap() calls sites.

> > You should also remove the cmd's 'nr_mapped' field while you're at it,
> > otherwise you'll hit the WARN in nvmet_tcp_free_cmd_buffers().
>=20
> Not remove nr_mapped because we use it to know the iovec entries, but
> we can just remove the WARN statement.

Ah, OK. I'll take care of this too. That was not my first concern when I di=
d=20
the RFC. The "real" patch must also address this detail.

@Chaitanya:

Since this is a mere simplification and shorten of code, I suppose I can sk=
ip=20
the performance tests. Ira and I have still hundreds of call sites with kma=
p()=20
and kmap_atomic() which we should care of, therefore we prefer to leave alo=
ne=20
everything that is not strictly necessary for the deprecated API deletions.

Thanks to you all,

=46abio