[RFC RESEND PATCH v2] scsi: iscsi: register sysfs for iscsi workqueue

[Bob Liu]

Motivation:
This patch enable setting cpu affinity through "cpumask" for iscsi workqueues
(iscsi_q_xx and iscsi_eh), so as to get performance isolation.

The max number of active worker was changed form 1 to 2, because "cpumask" of
ordered workqueue isn't allowed to change.

Notes:
- Having 2 workers break the current ordering guarantees, please let me know
if anyone depends on this.

- __WQ_LEGACY have to be left because of
23d11a5(workqueue: skip flush dependency checks for legacy workqueues)

Signed-off-by: Bob Liu <bob...@oracle.com>
---
drivers/scsi/libiscsi.c | 4 +++-
drivers/scsi/scsi_transport_iscsi.c | 4 +++-
2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 70b99c0..adf9bb4 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -2627,7 +2627,9 @@ struct Scsi_Host *iscsi_host_alloc(struct scsi_host_template *sht,
if (xmit_can_sleep) {
snprintf(ihost->workq_name, sizeof(ihost->workq_name),
"iscsi_q_%d", shost->host_no);
- ihost->workq = create_singlethread_workqueue(ihost->workq_name);
+ ihost->workq = alloc_workqueue("%s",
+ WQ_SYSFS | __WQ_LEGACY | WQ_MEM_RECLAIM | WQ_UNBOUND,
+ 2, ihost->workq_name);
if (!ihost->workq)
goto free_host;
}
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index dfc726f..bdbc4a2 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -4602,7 +4602,9 @@ static __init int iscsi_transport_init(void)
goto unregister_flashnode_bus;
}

- iscsi_eh_timer_workq = create_singlethread_workqueue("iscsi_eh");
+ iscsi_eh_timer_workq = alloc_workqueue("%s",
+ WQ_SYSFS | __WQ_LEGACY | WQ_MEM_RECLAIM | WQ_UNBOUND,
+ 2, "iscsi_eh");
if (!iscsi_eh_timer_workq) {
err = -ENOMEM;
goto release_nls;
--
2.9.5

[Martin K. Petersen]

Chris/Lee: Please review!

Martin K. Petersen Oracle Linux Engineering

[The Lee-Man]

On Monday, May 4, 2020 at 6:24:20 PM UTC-7, Bob Liu wrote:

Motivation:
This patch enable setting cpu affinity through "cpumask" for iscsi workqueues
(iscsi_q_xx and iscsi_eh), so as to get performance isolation.

Please summarize for this performance-idiot how setting thee CPU affinity helps you in any way? Is it for testing purposes only, does it cause some performance gains in some cases, and if so, which ones?

The max number of active worker was changed form 1 to 2, because "cpumask" of
ordered workqueue isn't allowed to change.

Notes:
- Having 2 workers break the current ordering guarantees, please let me know
  if anyone depends on this.

Have you tested with normal iSCSI IO from multiple initiators and targets?

- __WQ_LEGACY have to be left because of
23d11a5(workqueue: skip flush dependency checks for legacy workqueues)

I have no issue with this part (now), but normally, when you send out a second version of a patch, you add a section that says something like:

> Changes since V1:

> * change 1

> * ...

And you change the subject from "[PATCH] ..." to "[PATCHv2] ..." or "[PATCH v2] ...". This helps folks that review lots of patches to recognize they might only need to review the new bits.

In your case, you may have only changed the Description, but even that's worthy of a mention IMHO. But I won't (normally) reject a patch for this, but I appreciate when it's done correctly.

If you answer is that is has been tested, then I'll be glad to add my reviewed-by.

[Bob Liu]

friendly ping.

[Lee Duncan]

Reviewed-by: Lee Duncan <ldu...@suse.com>

[Martin K. Petersen]

On Tue, 5 May 2020 09:19:08 +0800, Bob Liu wrote:

> Motivation:
> This patch enable setting cpu affinity through "cpumask" for iscsi workqueues
> (iscsi_q_xx and iscsi_eh), so as to get performance isolation.
>
> The max number of active worker was changed form 1 to 2, because "cpumask" of
> ordered workqueue isn't allowed to change.
>

> [...]

Applied to 5.8/scsi-queue, thanks!

[1/1] scsi: iscsi: Register sysfs for iscsi workqueue
https://git.kernel.org/mkp/scsi/c/3ce419662dd4