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.
>
> 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
>
>
If you answer is that is has been tested, then I'll be glad to add my
reviewed-by.
--
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/open-iscsi/f5da6f33-c444-4746-8ebf-94003efbbfc2%40googlegroups.com.
