[PATCH v2 0/2] nvmet: support unbound_wq for RDMA and TCP

[PATCH v2 0/2] nvmet: support unbound_wq for RDMA and TCP

[Ping Gan]

When running nvmf on SMP platform, current nvme target's RDMA and
TCP use bounded workqueue to handle IO, but when there is other high
workload on the system(eg: kubernetes), the competition between the 
bounded kworker and other workload is very radical. To decrease the
resource race of OS among them, this patchset will enable unbounded
workqueue for nvmet-rdma and nvmet-tcp; besides that, it can also
get some performance improvement. And this patchset bases on previous
discussion from below session.

https://lore.kernel.org/lkml/20240717005318.109027-1-jacky_gam_2001@163.com/


Ping Gan (2):
  nvmet-tcp: add unbound_wq support for nvmet-tcp
  nvmet-rdma:  add unbound_wq support for nvmet-rdma

 drivers/nvme/target/rdma.c | 10 +++++++++-
 drivers/nvme/target/tcp.c  | 12 ++++++++++--
 2 files changed, 19 insertions(+), 3 deletions(-)

-- 
2.26.2

[PATCH v2 1/2] nvmet-tcp: add unbound_wq support for nvmet-tcp

[Ping Gan]

To define a module parameter use_unbound_wq to enable unbound
workqueue to handle TCP's IO.

Signed-off-by: Ping Gan <jacky_gam_2001@163.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/target/tcp.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 5bff0d5464d1..f71d56843e1a 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -73,6 +73,10 @@ device_param_cb(idle_poll_period_usecs, &set_param_ops,
 MODULE_PARM_DESC(idle_poll_period_usecs,
 		"nvmet tcp io_work poll till idle time period in usecs: Default 0");
 
+static bool use_unbound_wq;
+module_param(use_unbound_wq, bool, 0444);
+MODULE_PARM_DESC(use_unbound_wq, "use unbound workqueue to handle IO request: Default false");
+
 #ifdef CONFIG_NVME_TARGET_TCP_TLS
 /*
  * TLS handshake timeout
@@ -2196,9 +2200,13 @@ static const struct nvmet_fabrics_ops nvmet_tcp_ops = {
 static int __init nvmet_tcp_init(void)
 {
 	int ret;
+	unsigned int flags;
+
+	flags = WQ_MEM_RECLAIM | WQ_HIGHPRI;
+	if (use_unbound_wq)
+		flags |= (WQ_UNBOUND | WQ_SYSFS);
 
-	nvmet_tcp_wq = alloc_workqueue("nvmet_tcp_wq",
-				WQ_MEM_RECLAIM | WQ_HIGHPRI, 0);
+	nvmet_tcp_wq = alloc_workqueue("nvmet_tcp_wq", flags, 0);
 	if (!nvmet_tcp_wq)
 		return -ENOMEM;
 
-- 
2.26.2

[PATCH v2 2/2] nvmet-rdma: add unbound_wq support for nvmet-rdma

[Ping Gan]

To define a module parameter use_unbound_wq to enable unbound
workqueue to handle RDMA's IO of CQ.

Signed-off-by: Ping Gan <jacky_gam_2001@163.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/target/rdma.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 1eff8ca6a5f1..bfd7106316bc 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -155,6 +155,10 @@ static int nvmet_rdma_srq_size = 1024;
 module_param_cb(srq_size, &srq_size_ops, &nvmet_rdma_srq_size, 0644);
 MODULE_PARM_DESC(srq_size, "set Shared Receive Queue (SRQ) size, should >= 256 (default: 1024)");
 
+static bool use_unbound_wq;
+module_param(use_unbound_wq, bool, 0444);
+MODULE_PARM_DESC(use_unbound_wq, "use unbound workqueue to handle IO request: Default false");
+
 static DEFINE_IDA(nvmet_rdma_queue_ida);
 static LIST_HEAD(nvmet_rdma_queue_list);
 static DEFINE_MUTEX(nvmet_rdma_queue_mutex);
@@ -1259,7 +1263,11 @@ static int nvmet_rdma_create_queue_ib(struct nvmet_rdma_queue *queue)
 	 */
 	nr_cqe = queue->recv_queue_size + 2 * queue->send_queue_size;
 
-	queue->cq = ib_cq_pool_get(ndev->device, nr_cqe + 1,
+	if (use_unbound_wq)
+		queue->cq = ib_cq_pool_get(ndev->device, nr_cqe + 1,
+				   queue->comp_vector, IB_POLL_UNBOUND_WORKQUEUE);
+	else
+		queue->cq = ib_cq_pool_get(ndev->device, nr_cqe + 1,
 				   queue->comp_vector, IB_POLL_WORKQUEUE);
 	if (IS_ERR(queue->cq)) {
 		ret = PTR_ERR(queue->cq);
-- 
2.26.2

Re: [PATCH v2 1/2] nvmet-tcp: add unbound_wq support for nvmet-tcp

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH v2 2/2] nvmet-rdma: add unbound_wq support for nvmet-rdma

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH v2 0/2] nvmet: support unbound_wq for RDMA and TCP

[Christoph Hellwig]

On Wed, Jul 17, 2024 at 05:14:49PM +0800, Ping Gan wrote:
> When running nvmf on SMP platform, current nvme target's RDMA and
> TCP use bounded workqueue to handle IO, but when there is other high
> workload on the system(eg: kubernetes), the competition between the 
> bounded kworker and other workload is very radical. To decrease the
> resource race of OS among them, this patchset will enable unbounded
> workqueue for nvmet-rdma and nvmet-tcp; besides that, it can also
> get some performance improvement. And this patchset bases on previous
> discussion from below session.

So why aren't we using unbound workqueues by default?  Who makea the
policy decision and how does anyone know which one to chose?

Re: [PATCH v2 0/2] nvmet: support unbound_wq for RDMA and TCP

[Hannes Reinecke]

On 7/19/24 07:31, Christoph Hellwig wrote:
> On Wed, Jul 17, 2024 at 05:14:49PM +0800, Ping Gan wrote:
>> When running nvmf on SMP platform, current nvme target's RDMA and
>> TCP use bounded workqueue to handle IO, but when there is other high
>> workload on the system(eg: kubernetes), the competition between the
>> bounded kworker and other workload is very radical. To decrease the
>> resource race of OS among them, this patchset will enable unbounded
>> workqueue for nvmet-rdma and nvmet-tcp; besides that, it can also
>> get some performance improvement. And this patchset bases on previous
>> discussion from below session.
> 
> So why aren't we using unbound workqueues by default?  Who makea the
> policy decision and how does anyone know which one to chose?
> 
I'd be happy to switch to unbound workqueues per default.
It actually might be a left over from the various workqueue changes;
at one point 'unbound' meant that effectively only one CPU was used
for the workqueue, and you had to remove the 'unbound' parameter to
have the workqueue run on all CPUs. That has since changed, so I guess
switching to unbound per default is the better option here.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v2 0/2] nvmet: support unbound_wq for RDMA and TCP

[Ping Gan]

> On 7/19/24 07:31, Christoph Hellwig wrote:
>> On Wed, Jul 17, 2024 at 05:14:49PM +0800, Ping Gan wrote:
>>> When running nvmf on SMP platform, current nvme target's RDMA and
>>> TCP use bounded workqueue to handle IO, but when there is other high
>>> workload on the system(eg: kubernetes), the competition between the
>>> bounded kworker and other workload is very radical. To decrease the
>>> resource race of OS among them, this patchset will enable unbounded
>>> workqueue for nvmet-rdma and nvmet-tcp; besides that, it can also
>>> get some performance improvement. And this patchset bases on
>>> previous
>>> discussion from below session.
>> 
>> So why aren't we using unbound workqueues by default?  Who makea the
>> policy decision and how does anyone know which one to chose?
>> 
> I'd be happy to switch to unbound workqueues per default.
> It actually might be a left over from the various workqueue changes;
> at one point 'unbound' meant that effectively only one CPU was used
> for the workqueue, and you had to remove the 'unbound' parameter to
> have the workqueue run on all CPUs. That has since changed, so I guess
> switching to unbound per default is the better option here.

I don't fully understand what you said 'by default'. Did you mean we 
should just remove 'unbounded' parameter and create workqueue by 
WQ_UNBOUND flag or besides that, we should also add other parameter 
to switch 'unbounded' workqueue  to 'bounded' workqueue?

Thanks,
Ping

Re: [PATCH v2 0/2] nvmet: support unbound_wq for RDMA and TCP

[Hannes Reinecke]

On 7/19/24 10:07, Ping Gan wrote:
>> On 7/19/24 07:31, Christoph Hellwig wrote:
>>> On Wed, Jul 17, 2024 at 05:14:49PM +0800, Ping Gan wrote:
>>>> When running nvmf on SMP platform, current nvme target's RDMA and
>>>> TCP use bounded workqueue to handle IO, but when there is other high
>>>> workload on the system(eg: kubernetes), the competition between the
>>>> bounded kworker and other workload is very radical. To decrease the
>>>> resource race of OS among them, this patchset will enable unbounded
>>>> workqueue for nvmet-rdma and nvmet-tcp; besides that, it can also
>>>> get some performance improvement. And this patchset bases on
>>>> previous
>>>> discussion from below session.
>>>
>>> So why aren't we using unbound workqueues by default?  Who makea the
>>> policy decision and how does anyone know which one to chose?
>>>
>> I'd be happy to switch to unbound workqueues per default.
>> It actually might be a left over from the various workqueue changes;
>> at one point 'unbound' meant that effectively only one CPU was used
>> for the workqueue, and you had to remove the 'unbound' parameter to
>> have the workqueue run on all CPUs. That has since changed, so I guess
>> switching to unbound per default is the better option here.
> 
> I don't fully understand what you said 'by default'. Did you mean we
> should just remove 'unbounded' parameter and create workqueue by
> WQ_UNBOUND flag or besides that, we should also add other parameter
> to switch 'unbounded' workqueue  to 'bounded' workqueue?
> 
The former. Just remove the 'unbounded' parameter and always us
'WQ_UNBOUND' flag when creating workqueues.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v2 0/2] nvmet: support unbound_wq for RDMA and TCP

[Ping Gan]

> On 7/19/24 10:07, Ping Gan wrote:
>>> On 7/19/24 07:31, Christoph Hellwig wrote:
>>>> On Wed, Jul 17, 2024 at 05:14:49PM +0800, Ping Gan wrote:
>>>>> When running nvmf on SMP platform, current nvme target's RDMA and
>>>>> TCP use bounded workqueue to handle IO, but when there is other
>>>>> high
>>>>> workload on the system(eg: kubernetes), the competition between
>>>>> the
>>>>> bounded kworker and other workload is very radical. To decrease
>>>>> the
>>>>> resource race of OS among them, this patchset will enable
>>>>> unbounded
>>>>> workqueue for nvmet-rdma and nvmet-tcp; besides that, it can also
>>>>> get some performance improvement. And this patchset bases on
>>>>> previous
>>>>> discussion from below session.
>>>>
>>>> So why aren't we using unbound workqueues by default?  Who makea
>>>> the
>>>> policy decision and how does anyone know which one to chose?
>>>>
>>> I'd be happy to switch to unbound workqueues per default.
>>> It actually might be a left over from the various workqueue changes;
>>> at one point 'unbound' meant that effectively only one CPU was used
>>> for the workqueue, and you had to remove the 'unbound' parameter to
>>> have the workqueue run on all CPUs. That has since changed, so I
>>> guess
>>> switching to unbound per default is the better option here.
>> 
>> I don't fully understand what you said 'by default'. Did you mean we
>> should just remove 'unbounded' parameter and create workqueue by
>> WQ_UNBOUND flag or besides that, we should also add other parameter
>> to switch 'unbounded' workqueue  to 'bounded' workqueue?
>> 
> The former. Just remove the 'unbounded' parameter and always us
> 'WQ_UNBOUND' flag when creating workqueues.

Okay, will do in next version

Thanks,
Ping

Re: [PATCH v2 0/2] nvmet: support unbound_wq for RDMA and TCP

[Sagi Grimberg]

On 19/07/2024 8:31, Christoph Hellwig wrote:
> On Wed, Jul 17, 2024 at 05:14:49PM +0800, Ping Gan wrote:
>> When running nvmf on SMP platform, current nvme target's RDMA and
>> TCP use bounded workqueue to handle IO, but when there is other high
>> workload on the system(eg: kubernetes), the competition between the
>> bounded kworker and other workload is very radical. To decrease the
>> resource race of OS among them, this patchset will enable unbounded
>> workqueue for nvmet-rdma and nvmet-tcp; besides that, it can also
>> get some performance improvement. And this patchset bases on previous
>> discussion from below session.
> So why aren't we using unbound workqueues by default?  Who makea the
> policy decision and how does anyone know which one to chose?
>

The use-case presented is a case where the cpu resources are shared
between nvmet and other workloads running on the system. The ask is to
prevent nvmet to run io threads from specific cpu cores, and vice-versa, to
minimize interference.

The decision is made by the administrator that decides which resources are
dedicated to nvmet vs. other workloads (which are containers in this case).

Changing to unbound workqueues universally needs to prove that it is better
in the general case, outside of this specific use-case. Meaning that 
latency is
not affected by having unbound kthreads accessing the nvme device, the 
rdma qp
and/or the tcp socket.

Re: [PATCH v2 0/2] nvmet: support unbound_wq for RDMA and TCP

[Sagi Grimberg]

On 19/07/2024 9:28, Hannes Reinecke wrote:
> On 7/19/24 07:31, Christoph Hellwig wrote:
>> On Wed, Jul 17, 2024 at 05:14:49PM +0800, Ping Gan wrote:
>>> When running nvmf on SMP platform, current nvme target's RDMA and
>>> TCP use bounded workqueue to handle IO, but when there is other high
>>> workload on the system(eg: kubernetes), the competition between the
>>> bounded kworker and other workload is very radical. To decrease the
>>> resource race of OS among them, this patchset will enable unbounded
>>> workqueue for nvmet-rdma and nvmet-tcp; besides that, it can also
>>> get some performance improvement. And this patchset bases on previous
>>> discussion from below session.
>>
>> So why aren't we using unbound workqueues by default?  Who makea the
>> policy decision and how does anyone know which one to chose?
>>
> I'd be happy to switch to unbound workqueues per default.
> It actually might be a left over from the various workqueue changes;
> at one point 'unbound' meant that effectively only one CPU was used
> for the workqueue, and you had to remove the 'unbound' parameter to
> have the workqueue run on all CPUs. That has since changed, so I guess
> switching to unbound per default is the better option here.

A guess needs to be based with supporting data if we want to have this 
change.