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;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.300.101.1.3\))
Subject: Re: [PATCH] nvme/tcp: Add support to set the tcp worker cpu affinity
From:   Li Feng <fengli@smartx.com>
In-Reply-To: <7f553d34-9ada-426c-4847-c7cd1aba64a8@grimberg.me>
Date:   Tue, 18 Apr 2023 11:39:49 +0800
Cc:     Keith Busch <kbusch@kernel.org>, Jens Axboe <axboe@fb.com>,
        Christoph Hellwig <hch@lst.de>,
        "open list:NVM EXPRESS DRIVER" <linux-nvme@lists.infradead.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <E23A9D5C-35CF-412C-AE35-37389242BC71@smartx.com>
References: <20230413062339.2454616-1-fengli@smartx.com>
 <20230413063317.2455680-1-fengli@smartx.com>
 <7f553d34-9ada-426c-4847-c7cd1aba64a8@grimberg.me>
To:     Sagi Grimberg <sagi@grimberg.me>
Precedence: bulk

Hi Sagi,

> 2023=E5=B9=B44=E6=9C=8817=E6=97=A5 =E4=B8=8B=E5=8D=889:45=EF=BC=8CSagi =
Grimberg <sagi@grimberg.me> =E5=86=99=E9=81=93=EF=BC=9A
>=20
> Hey Li,
>=20
>> The default worker affinity policy is using all online cpus, e.g. =
from 0
>> to N-1. However, some cpus are busy for other jobs, then the nvme-tcp =
will
>> have a bad performance.
>> This patch adds a module parameter to set the cpu affinity for the =
nvme-tcp
>> socket worker threads.  The parameter is a comma separated list of =
CPU
>> numbers.  The list is parsed and the resulting cpumask is used to set =
the
>> affinity of the socket worker threads.  If the list is empty or the
>> parsing fails, the default affinity is used.
>=20
> I can see how this may benefit a specific set of workloads, but I have =
a
> few issues with this.
>=20
> - This is exposing a user interface for something that is really
> internal to the driver.
>=20
> - This is something that can be misleading and could be tricky to get
> right, my concern is that this would only benefit a very niche case.
Our storage products needs this feature~
If the user doesn=E2=80=99t know what this is, they can keep it default, =
so I thinks this is
not unacceptable.
>=20
> - If the setting should exist, it should not be global.
V2 has fixed it.
>=20
> - I prefer not to introduce new modparams.
>=20
> - I'd prefer to find a way to support your use-case without =
introducing
> a config knob for it.
>=20
I=E2=80=99m looking forward to it.
=20
> - It is not backed by performance improvements, but more importantly
> does not cover any potential regressions in key metrics (bw/iops/lat)
> or lack there of.

I can do more tests if needed.

Thanks,
Feng Li