[PATCH] nvmet: make nvmet_wq unbound

[PATCH] nvmet: make nvmet_wq unbound

[Sagi Grimberg]

From: Sagi Grimberg <sagi.grimberg@vastdata.com>

When deleting many controllers one-by-one, it takes a very
long time as these work elements may serialize as they are
scheduled on the executing cpu instead of spreading. In general
nvmet_wq can definitely be used for long standing work elements
so its better to make it unbound regardless.

While we are at it, expose it via sysfs to allow cpumask
control of this workqueue if a user wants to modify it.

Signed-off-by: Sagi Grimberg <sagi.grimberg@vastdata.com>
---
 drivers/nvme/target/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index e06013c5dace..5e948864ea89 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1686,7 +1686,8 @@ static int __init nvmet_init(void)
 	if (!buffered_io_wq)
 		goto out_free_zbd_work_queue;
 
-	nvmet_wq = alloc_workqueue("nvmet-wq", WQ_MEM_RECLAIM, 0);
+	nvmet_wq = alloc_workqueue("nvmet-wq",
+			WQ_MEM_RECLAIM | WQ_SYSFS | WQ_UNBOUND, 0);
 	if (!nvmet_wq)
 		goto out_free_buffered_work_queue;
 
-- 
2.40.1

Re: [PATCH] nvmet: make nvmet_wq unbound

[Christoph Hellwig]

On Sun, May 05, 2024 at 01:39:51PM +0300, Sagi Grimberg wrote:
> From: Sagi Grimberg <sagi.grimberg@vastdata.com>
> 
> When deleting many controllers one-by-one, it takes a very
> long time as these work elements may serialize as they are
> scheduled on the executing cpu instead of spreading. In general
> nvmet_wq can definitely be used for long standing work elements
> so its better to make it unbound regardless.

This looks sensible, thanks.

> While we are at it, expose it via sysfs to allow cpumask
> control of this workqueue if a user wants to modify it.

This on the other looks very questionable to me.  It needs a way better
justification and should be split into a separate patch if we really
need it.

Re: [PATCH] nvmet: make nvmet_wq unbound

[Sagi Grimberg]

On 06/05/2024 8:33, Christoph Hellwig wrote:
> On Sun, May 05, 2024 at 01:39:51PM +0300, Sagi Grimberg wrote:
>> From: Sagi Grimberg <sagi.grimberg@vastdata.com>
>>
>> When deleting many controllers one-by-one, it takes a very
>> long time as these work elements may serialize as they are
>> scheduled on the executing cpu instead of spreading. In general
>> nvmet_wq can definitely be used for long standing work elements
>> so its better to make it unbound regardless.
> This looks sensible, thanks.
>
>> While we are at it, expose it via sysfs to allow cpumask
>> control of this workqueue if a user wants to modify it.
> This on the other looks very questionable to me.  It needs a way better
> justification and should be split into a separate patch if we really
> need it.

It doesn't have any better justification. I never really saw any other 
justification
to add WQ_SYSFS to a (usually unbound) workqueue, than simply have 
visibility
or ability to control it.

What makes you think this is not harmless and useful?

Re: [PATCH] nvmet: make nvmet_wq unbound

[Christoph Hellwig]

On Mon, May 06, 2024 at 10:50:02AM +0300, Sagi Grimberg wrote:
> It doesn't have any better justification. I never really saw any other 
> justification
> to add WQ_SYSFS to a (usually unbound) workqueue, than simply have 
> visibility
> or ability to control it.
>
> What makes you think this is not harmless and useful?

Because now we have to deal with users setting more or less insane
parameters on the workqueue.

Re: [PATCH] nvmet: make nvmet_wq unbound

[Sagi Grimberg]

On 06/05/2024 15:08, Christoph Hellwig wrote:
> On Mon, May 06, 2024 at 10:50:02AM +0300, Sagi Grimberg wrote:
>> It doesn't have any better justification. I never really saw any other
>> justification
>> to add WQ_SYSFS to a (usually unbound) workqueue, than simply have
>> visibility
>> or ability to control it.
>>
>> What makes you think this is not harmless and useful?
> Because now we have to deal with users setting more or less insane
> parameters on the workqueue.
>

Isn't that an argument against WQ_SYSFS entirely?
I can drop it though, its not really important.

Re: [PATCH] nvmet: make nvmet_wq unbound

[Christoph Hellwig]

On Mon, May 06, 2024 at 05:05:13PM +0300, Sagi Grimberg wrote:
>>> What makes you think this is not harmless and useful?
>> Because now we have to deal with users setting more or less insane
>> parameters on the workqueue.
>>
>
> Isn't that an argument against WQ_SYSFS entirely?
> I can drop it though, its not really important.

Unless it's a workqueue where these settings explicitly should be user
tweakable: yes.