Patch: Make the iscsi lld create a WQ_HIGHPRI workqueue itself

Patch: Make the iscsi lld create a WQ_HIGHPRI workqueue itself

[Richard Sharpe]

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]

Hi folks,

We noticed recently while testing with large numbers of iSCSI
connections that iscsid was going to heroic lengths to change the nice
value of the iSCSI work queue to -20. It becomes very expensive with
hundreds of connections.

This small patch has the LLD do the work,

Feedback welcome.

A small patch will be needed for the user-space tools as well. I will
send one in if someone else does not do so.

-- 
Regards,
Richard Sharpe
(何以解憂？唯有杜康。--曹操)

[-- Attachment #2: 0001-Make-the-iscsi-lld-create-a-WQ_HIGHPRI-workqueue.patch --]
[-- Type: application/octet-stream, Size: 1505 bytes --]

From c0f04a66b29a9190109740c03ee9e65bb62afe28 Mon Sep 17 00:00:00 2001
From: Richard Sharpe <rsharpe@nutanix.com>
Date: Mon, 28 Mar 2016 13:36:18 -0700
Subject: [PATCH] Make the iscsi lld create a WQ_HIGHPRI workqueue.

The open-iscsi tools go to heroic efforts to change the nice value of
the iscsi workqueue to -20. It scans all processes in /proc and
when it finds the one it is interested in calls setpriority.

When you are connecting to a reasonable number of iSCSI targets this
can be costly.

This small patch has the iscsi LLD do the work itself. It seems like
the least intrusive way to acheive the result.

A small patch will be needed in the open-iscsi userspace tools
as well.

Signed-off-by: Richard Sharpe <realrichardsharpe@gmail.com>
---
 drivers/scsi/libiscsi.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 6bffd91..abaefe1 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -2615,9 +2615,11 @@ struct Scsi_Host *iscsi_host_alloc(struct scsi_host_template *sht,
 	ihost = shost_priv(shost);
 
 	if (xmit_can_sleep) {
+		int wq_flags = __WQ_LEGACY | WQ_MEM_RECLAIM | WQ_HIGHPRI;
 		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_ordered_workqueue("%s", wq_flags,
+						ihost->workq_name);
 		if (!ihost->workq)
 			goto free_host;
 	}
-- 
1.7.1

Re: Patch: Make the iscsi lld create a WQ_HIGHPRI workqueue itself

[Mike Christie]

On 03/28/2016 03:46 PM, Richard Sharpe wrote:
> Hi folks,
> 
> We noticed recently while testing with large numbers of iSCSI
> connections that iscsid was going to heroic lengths to change the nice
> value of the iSCSI work queue to -20. It becomes very expensive with
> hundreds of connections.
> 
> This small patch has the LLD do the work,
> 
> Feedback welcome.
> 
> A small patch will be needed for the user-space tools as well. I will
> send one in if someone else does not do so.
> 
> -- Regards, Richard Sharpe (何以解憂？唯有杜康。--曹操)
> 
> 
> 0001-Make-the-iscsi-lld-create-a-WQ_HIGHPRI-workqueue.patch
> 
> 
> From c0f04a66b29a9190109740c03ee9e65bb62afe28 Mon Sep 17 00:00:00 2001
> From: Richard Sharpe <rsharpe@nutanix.com>
> Date: Mon, 28 Mar 2016 13:36:18 -0700
> Subject: [PATCH] Make the iscsi lld create a WQ_HIGHPRI workqueue.
> 
> The open-iscsi tools go to heroic efforts to change the nice value of
> the iscsi workqueue to -20. It scans all processes in /proc and
> when it finds the one it is interested in calls setpriority.
> 
> When you are connecting to a reasonable number of iSCSI targets this
> can be costly.
> 
> This small patch has the iscsi LLD do the work itself. It seems like
> the least intrusive way to acheive the result.
> 
> A small patch will be needed in the open-iscsi userspace tools
> as well.
> 
> Signed-off-by: Richard Sharpe <realrichardsharpe@gmail.com>
> ---
>  drivers/scsi/libiscsi.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 6bffd91..abaefe1 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -2615,9 +2615,11 @@ struct Scsi_Host *iscsi_host_alloc(struct scsi_host_template *sht,
>  	ihost = shost_priv(shost);
>  
>  	if (xmit_can_sleep) {
> +		int wq_flags = __WQ_LEGACY | WQ_MEM_RECLAIM | WQ_HIGHPRI;


I do not think we need to or are supposed to be setting __WQ_LEGACY. It
looks like it was only to catch the case here

https://lkml.org/lkml/2016/1/29/179



>  		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_ordered_workqueue("%s", wq_flags,
> +						ihost->workq_name);
>  		if (!ihost->workq)
>  			goto free_host;
>  	}
> -- 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Patch: Make the iscsi lld create a WQ_HIGHPRI workqueue itself

[Richard Sharpe]

On Mon, Mar 28, 2016 at 4:53 PM, Mike Christie <mchristi@redhat.com> wrote:
> On 03/28/2016 03:46 PM, Richard Sharpe wrote:
>> Hi folks,
>>
>> We noticed recently while testing with large numbers of iSCSI
>> connections that iscsid was going to heroic lengths to change the nice
>> value of the iSCSI work queue to -20. It becomes very expensive with
>> hundreds of connections.
>>
>> This small patch has the LLD do the work,
>>
>> Feedback welcome.
>>
>> A small patch will be needed for the user-space tools as well. I will
>> send one in if someone else does not do so.
>>
>> -- Regards, Richard Sharpe (何以解憂？唯有杜康。--曹操)
>>
>>
>> 0001-Make-the-iscsi-lld-create-a-WQ_HIGHPRI-workqueue.patch
>>
>>
>> From c0f04a66b29a9190109740c03ee9e65bb62afe28 Mon Sep 17 00:00:00 2001
>> From: Richard Sharpe <rsharpe@nutanix.com>
>> Date: Mon, 28 Mar 2016 13:36:18 -0700
>> Subject: [PATCH] Make the iscsi lld create a WQ_HIGHPRI workqueue.
>>
>> The open-iscsi tools go to heroic efforts to change the nice value of
>> the iscsi workqueue to -20. It scans all processes in /proc and
>> when it finds the one it is interested in calls setpriority.
>>
>> When you are connecting to a reasonable number of iSCSI targets this
>> can be costly.
>>
>> This small patch has the iscsi LLD do the work itself. It seems like
>> the least intrusive way to acheive the result.
>>
>> A small patch will be needed in the open-iscsi userspace tools
>> as well.
>>
>> Signed-off-by: Richard Sharpe <realrichardsharpe@gmail.com>
>> ---
>>  drivers/scsi/libiscsi.c |    4 +++-
>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
>> index 6bffd91..abaefe1 100644
>> --- a/drivers/scsi/libiscsi.c
>> +++ b/drivers/scsi/libiscsi.c
>> @@ -2615,9 +2615,11 @@ struct Scsi_Host *iscsi_host_alloc(struct scsi_host_template *sht,
>>       ihost = shost_priv(shost);
>>
>>       if (xmit_can_sleep) {
>> +             int wq_flags = __WQ_LEGACY | WQ_MEM_RECLAIM | WQ_HIGHPRI;
>
>
> I do not think we need to or are supposed to be setting __WQ_LEGACY. It
> looks like it was only to catch the case here

OK, I will fix that and resubmit.

> https://lkml.org/lkml/2016/1/29/179
>
>
>
>>               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_ordered_workqueue("%s", wq_flags,
>> +                                             ihost->workq_name);
>>               if (!ihost->workq)
>>                       goto free_host;
>>       }
>> --
>



-- 
Regards,
Richard Sharpe
(何以解憂？唯有杜康。--曹操)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Patch: Make the iscsi lld create a WQ_HIGHPRI workqueue itself

[Richard Sharpe]

[-- Attachment #1: Type: text/plain, Size: 1267 bytes --]

On Mon, Mar 28, 2016 at 4:53 PM, Mike Christie <mchristi@redhat.com> wrote:
> On 03/28/2016 03:46 PM, Richard Sharpe wrote:
>> Hi folks,
>>
>> We noticed recently while testing with large numbers of iSCSI
>> connections that iscsid was going to heroic lengths to change the nice
>> value of the iSCSI work queue to -20. It becomes very expensive with
>> hundreds of connections.
>>
>> This small patch has the LLD do the work,
>>
>> Feedback welcome.

[...]

>> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
>> index 6bffd91..abaefe1 100644
>> --- a/drivers/scsi/libiscsi.c
>> +++ b/drivers/scsi/libiscsi.c
>> @@ -2615,9 +2615,11 @@ struct Scsi_Host *iscsi_host_alloc(struct scsi_host_template *sht,
>>       ihost = shost_priv(shost);
>>
>>       if (xmit_can_sleep) {
>> +             int wq_flags = __WQ_LEGACY | WQ_MEM_RECLAIM | WQ_HIGHPRI;
>
>
> I do not think we need to or are supposed to be setting __WQ_LEGACY. It
> looks like it was only to catch the case here

Attached is a patch the removes that flag and also WQ_MEM_RECLAIM
because none of the work items placed on the queue do allocations with
GPF_KERNEL, as far as I can tell.

-- 
Regards,
Richard Sharpe
(何以解憂？唯有杜康。--曹操)

[-- Attachment #2: 0001-Make-the-iscsi-lld-create-a-WQ_HIGHPRI-workqueue.patch --]
[-- Type: application/octet-stream, Size: 1993 bytes --]

From dfab98de6c0bd84684efa4692f573d46f8b3d798 Mon Sep 17 00:00:00 2001
From: Richard Sharpe <rsharpe@nutanix.com>
Date: Mon, 28 Mar 2016 13:36:18 -0700
Subject: [PATCH] Make the iscsi lld create a WQ_HIGHPRI workqueue.

The open-iscsi tools go to heroic efforts to change the nice value of
the iscsi workqueue to -20. It scans all processes in /proc and
when it finds the one it is interested in calls setpriority.

When you are connecting to a reasonable number of iSCSI targets this
can be costly.

This small patch has the iscsi LLD do the work itself. It seems like
the least intrusive way to acheive the result.

Also removed the WQ_MEM_RECLAIM flag as libiscsi does not do any
memory allocs involving GPF_KERNEL in its workqueues.

A small patch will be needed in the open-iscsi userspace tools
as well.

Signed-off-by: Richard Sharpe <realrichardsharpe@gmail.com>
---
 drivers/scsi/libiscsi.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 6bffd91..2c864d0 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -2602,6 +2602,9 @@ EXPORT_SYMBOL_GPL(iscsi_host_add);
  *
  * This should be called by partial offload and software iscsi drivers.
  * To access the driver specific memory use the iscsi_host_priv() macro.
+ *
+ * Note that we are creating an ordered workqueue. You must not place work
+ * on it that allocates memory with GPF_KERNEL.
  */
 struct Scsi_Host *iscsi_host_alloc(struct scsi_host_template *sht,
 				   int dd_data_size, bool xmit_can_sleep)
@@ -2617,7 +2620,8 @@ 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_ordered_workqueue("%s", WQ_HIGHPRI,
+						ihost->workq_name);
 		if (!ihost->workq)
 			goto free_host;
 	}
-- 
1.7.1

Re: Patch: Make the iscsi lld create a WQ_HIGHPRI workqueue itself

[Richard Sharpe]

On Wed, Mar 30, 2016 at 9:04 AM, Richard Sharpe
<realrichardsharpe@gmail.com> wrote:
> On Mon, Mar 28, 2016 at 4:53 PM, Mike Christie <mchristi@redhat.com> wrote:
>> On 03/28/2016 03:46 PM, Richard Sharpe wrote:
>>> Hi folks,
>>>
>>> We noticed recently while testing with large numbers of iSCSI
>>> connections that iscsid was going to heroic lengths to change the nice
>>> value of the iSCSI work queue to -20. It becomes very expensive with
>>> hundreds of connections.
>>>
>>> This small patch has the LLD do the work,
>>>
>>> Feedback welcome.
>
> [...]
>
>>> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
>>> index 6bffd91..abaefe1 100644
>>> --- a/drivers/scsi/libiscsi.c
>>> +++ b/drivers/scsi/libiscsi.c
>>> @@ -2615,9 +2615,11 @@ struct Scsi_Host *iscsi_host_alloc(struct scsi_host_template *sht,
>>>       ihost = shost_priv(shost);
>>>
>>>       if (xmit_can_sleep) {
>>> +             int wq_flags = __WQ_LEGACY | WQ_MEM_RECLAIM | WQ_HIGHPRI;
>>
>>
>> I do not think we need to or are supposed to be setting __WQ_LEGACY. It
>> looks like it was only to catch the case here
>
> Attached is a patch the removes that flag and also WQ_MEM_RECLAIM
> because none of the work items placed on the queue do allocations with
> GPF_KERNEL, as far as I can tell.

It turns out that my second patch is also wrong because we do need
WQ_MEM_RECLAIM on the workqueue.

However, the fact that no one has noticed this problem for a long time
(probably since the workqueue model was changed) suggests that it does
not matter that the iSCSI workqueue is not WG_HIGHPRI.

Internally, we are doing some measurements to see if it makes any difference.

I will report back with the results, probably in a few days.

-- 
Regards,
Richard Sharpe
(何以解憂？唯有杜康。--曹操)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html