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:     Chaitanya Kulkarni <chaitanyak@nvidia.com>
Cc:     "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
        Christoph Hellwig <hch@lst.de>,
        Chaitanya Kulkarni <chaitanyak@nvidia.com>,
        Sagi Grimberg <sagi@grimberg.me>,
        Ira Weiny <ira.weiny@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH] nvmet-tcp: Don't kmap() pages which can't come from HIGHMEM
Date:   Tue, 16 Aug 2022 20:16:54 +0200
Message-ID: <8992226.CDJkKcVGEf@opensuse>
In-Reply-To: <3e8a0bb1-4c69-93d2-71f9-81bb8466cb14@nvidia.com>
References: <20220816091808.23236-1-fmdefrancesco@gmail.com> <3e8a0bb1-4c69-93d2-71f9-81bb8466cb14@nvidia.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"
Precedence: bulk

On marted=C3=AC 16 agosto 2022 15:12:08 CEST Chaitanya Kulkarni wrote:
> Fabio,
>=20
> On 8/16/22 02:18, Fabio M. De Francesco wrote:
>=20
> > kmap() is being deprecated in favor of kmap_local_page().
> >=20
> > There are two main problems with kmap(): (1) It comes with an overhead =
as
> > 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.
> > =20
>=20
> so I believe this should give us better performance under heavy
> workload ?
>

Yes, correct. Can you please take a look at the highmem official documentat=
ion=20
(highmem.rst)? I reworked and extended it with two series of patches.=20
Everything about the deprecation of kmap() is explained there and in a patc=
h=20
from Ira: "checkpatch: Add kmap and kmap_atomic to the deprecated list" whi=
ch=20
you reviewed at https://lore.kernel.org/all/91f708ed-f456-dc83-281e-fc18a0b=
4b981@nvidia.com/
=20
> > 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 are still valid.
> >=20
> > However, there is a huge constraint which might block some conversions
> > to kmap_local_page(): the kernel virtual address cannot be handed across
> > different threads. Ira made me notice that the kmap() and kunmap() in t=
his
> > driver happen in two different workqueues. Therefore, kunmap_local() wi=
ll
> > try to unmap an invalid address.
> >=20
> > Let me explain why I'm sending an RFC. When I hit the above mentioned
> > issues I tried to refactor the code in ways where mapping and unmapping
> > happen in a single thread (to not break the rules of threads locality).
> >=20
> > However, while reading this code again I think I noticed an important
> > prerequisite which may lead to a simpler solution... If I'm not wrong, =
it
> > looks like the pages are allocated in nvmet_tcp_map_data(), using the
> > GFP_KERNEL flag.
> >=20
> > This would assure that those pages _cannot_ come from HIGHMEM. If I'm n=
ot
> > missing something (again!), a plain page_address() could replace the=20
kmap()
> > of sg_page(sg); furthermore, we shouldn't need the un-mappings any long=
er.
> >=20
> > Unfortunately, I don't know this protocol and I'm not so experienced wi=
th
> > kernel development to be able to understand this code properly.
> >=20
> > Therefore, I have two questions: am I right about thinking that the pag=
es
> > mapped in nvmet_tcp_map_pdu_iovec() are allocated with GFP_KERNEL? If s=
o,
> > can anyone with more knowledge than mine please say if my changes make =
any
> > sense?
> >=20
> > Suggested-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>=20
>=20
> Thanks a lot for detailed explanation.

You are welcome!

> Quick question what kind of performance benefits you have seen with
> this change ? we need to document the performance numbers since commit
> log mentions here that kmap_local_page() is faster than kmap().

OK, but kmap_local_page() was discarded because not applicable here without=
=20
heavy refactoring.=20

> In case you are not aware please have a look at the blktests to create
> a simple loopback setup with nvme-loop transport.

I have nothing against learning how blktests works and running this tool.
I'll do as you requested.

However, please read the implementation of kmap():

#ifdef CONFIG_HIGHMEM

static inline void *kmap(struct page *page)
{
	void *addr;

	might_sleep();
	if (!PageHighMem(page))
		addr =3D page_address(page);
	else
		addr =3D kmap_high(page);
	kmap_flush_tlb((unsigned long)addr);
	return addr;
}

If page is not from HIGHMEM it is a simple page_address(), like it is in my=
=20
RFC patch.

#else /* CONFIG_HIGHMEM */

static inline void *kmap(struct page *page)
{
	might_sleep();
	return page_address(page);
}

Again, a plain page_address().
=46urthermore, with a simple page_address() we avoid the calls to kunmap().

I think it implicitly say all we need to know about why we should prefer=20
page_address() whenever we are _sure_ that pages cannot come from HIGHMEM.

Thanks for your comments and questions,

=46abio

> -ck
>