[RFC] IB/srp: Remove create_workqueue

[RFC] IB/srp: Remove create_workqueue

[Bhaktipriya Shridhar]

alloc_workqueue replaces deprecated create_workqueue().

A dedicated workqueue has been used since the workqueue srp_remove_wq with
workitem &target->remove_work, is a work queue for the SRP target removal.
WQ_MEM_RECLAIM has been set to ensure forward progress under memory
pressure.
Since there are only a fixed number of work items, explicit
concurrency limit is unnecessary here.

Is the workqueue being used on a memory reclaim path?
Does it require WQ_MEM_RECLAIM?

Thanks.

Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 646de17..1730d1f 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -3685,7 +3685,7 @@ static int __init srp_init_module(void)
 		indirect_sg_entries = cmd_sg_entries;
 	}

-	srp_remove_wq = create_workqueue("srp_remove");
+	srp_remove_wq = alloc_workqueue("srp_remove", WQ_MEM_RECLAIM, 0);
 	if (!srp_remove_wq) {
 		ret = -ENOMEM;
 		goto out;
--
2.1.4

Re: [RFC] IB/srp: Remove create_workqueue

[Bart Van Assche]

On 06/07/2016 11:16 AM, Bhaktipriya Shridhar wrote:
> alloc_workqueue replaces deprecated create_workqueue().
>
> A dedicated workqueue has been used since the workqueue srp_remove_wq with
> workitem &target->remove_work, is a work queue for the SRP target removal.
> WQ_MEM_RECLAIM has been set to ensure forward progress under memory
> pressure.
> Since there are only a fixed number of work items, explicit
> concurrency limit is unnecessary here.
>
> Is the workqueue being used on a memory reclaim path?
> Does it require WQ_MEM_RECLAIM?

Hello Bhaktipriya,

srp_remove_wq is used for SRP target port removal work only. This work 
is neither queued from inside a shrinker nor by the page writeback code 
so I think it is safe to drop WQ_MEM_RECLAIM.

Thanks,

Bart.

Re: [RFC] IB/srp: Remove create_workqueue

[Tejun Heo]

On Tue, Jun 07, 2016 at 11:32:42AM -0700, Bart Van Assche wrote:
> On 06/07/2016 11:16 AM, Bhaktipriya Shridhar wrote:
> > alloc_workqueue replaces deprecated create_workqueue().
> > 
> > A dedicated workqueue has been used since the workqueue srp_remove_wq with
> > workitem &target->remove_work, is a work queue for the SRP target removal.
> > WQ_MEM_RECLAIM has been set to ensure forward progress under memory
> > pressure.
> > Since there are only a fixed number of work items, explicit
> > concurrency limit is unnecessary here.
> > 
> > Is the workqueue being used on a memory reclaim path?
> > Does it require WQ_MEM_RECLAIM?
> 
> Hello Bhaktipriya,
> 
> srp_remove_wq is used for SRP target port removal work only. This work is
> neither queued from inside a shrinker nor by the page writeback code so I
> think it is safe to drop WQ_MEM_RECLAIM.

It should be able to use system_wq then.

Thanks for the explanation!

-- 
tejun

Re: [RFC] IB/srp: Remove create_workqueue

[Bart Van Assche]

On 06/07/2016 12:21 PM, Tejun Heo wrote:
> On Tue, Jun 07, 2016 at 11:32:42AM -0700, Bart Van Assche wrote:
>> On 06/07/2016 11:16 AM, Bhaktipriya Shridhar wrote:
>>> alloc_workqueue replaces deprecated create_workqueue().
>>>
>>> A dedicated workqueue has been used since the workqueue srp_remove_wq with
>>> workitem &target->remove_work, is a work queue for the SRP target removal.
>>> WQ_MEM_RECLAIM has been set to ensure forward progress under memory
>>> pressure.
>>> Since there are only a fixed number of work items, explicit
>>> concurrency limit is unnecessary here.
>>>
>>> Is the workqueue being used on a memory reclaim path?
>>> Does it require WQ_MEM_RECLAIM?
>>
>> Hello Bhaktipriya,
>>
>> srp_remove_wq is used for SRP target port removal work only. This work is
>> neither queued from inside a shrinker nor by the page writeback code so I
>> think it is safe to drop WQ_MEM_RECLAIM.
>
> It should be able to use system_wq then.

No. I have tried that but that resulted in a deadlock.

Bart.

Re: [RFC] IB/srp: Remove create_workqueue

[Bart Van Assche]

On 06/07/2016 12:56 PM, Bart Van Assche wrote:
> On 06/07/2016 12:21 PM, Tejun Heo wrote:
>> On Tue, Jun 07, 2016 at 11:32:42AM -0700, Bart Van Assche wrote:
>>> On 06/07/2016 11:16 AM, Bhaktipriya Shridhar wrote:
>>>> alloc_workqueue replaces deprecated create_workqueue().
>>>>
>>>> A dedicated workqueue has been used since the workqueue srp_remove_wq with
>>>> workitem &target->remove_work, is a work queue for the SRP target removal.
>>>> WQ_MEM_RECLAIM has been set to ensure forward progress under memory
>>>> pressure.
>>>> Since there are only a fixed number of work items, explicit
>>>> concurrency limit is unnecessary here.
>>>>
>>>> Is the workqueue being used on a memory reclaim path?
>>>> Does it require WQ_MEM_RECLAIM?
>>>
>>> Hello Bhaktipriya,
>>>
>>> srp_remove_wq is used for SRP target port removal work only. This work is
>>> neither queued from inside a shrinker nor by the page writeback code so I
>>> think it is safe to drop WQ_MEM_RECLAIM.
>>
>> It should be able to use system_wq then.
>
> No. I have tried that but that resulted in a deadlock.

See also commit bcc059103591 for the details.

Bart.

Re: [RFC] IB/srp: Remove create_workqueue

[Tejun Heo]

Hello, Bart.

On Tue, Jun 07, 2016 at 01:00:13PM -0700, Bart Van Assche wrote:
> > > > srp_remove_wq is used for SRP target port removal work only. This work is
> > > > neither queued from inside a shrinker nor by the page writeback code so I
> > > > think it is safe to drop WQ_MEM_RECLAIM.
> > > 
> > > It should be able to use system_wq then.
> > 
> > No. I have tried that but that resulted in a deadlock.
> 
> See also commit bcc059103591 for the details.

So, create_workqueue() limits concurrency to 1 per cpu and if you have
a dependency between two work items and they get scheduled on the same
cpu they can deadlock.  system_wq doesn't have that restriction and
should be fine, AFAICS.

Thanks!

-- 
tejun

Re: [RFC] IB/srp: Remove create_workqueue

[Bart Van Assche]

On 06/20/2016 08:59 PM, Tejun Heo wrote:
> On Tue, Jun 07, 2016 at 01:00:13PM -0700, Bart Van Assche wrote:
>>>>> srp_remove_wq is used for SRP target port removal work only. This work is
>>>>> neither queued from inside a shrinker nor by the page writeback code so I
>>>>> think it is safe to drop WQ_MEM_RECLAIM.
>>>>
>>>> It should be able to use system_wq then.
>>>
>>> No. I have tried that but that resulted in a deadlock.
>>
>> See also commit bcc059103591 for the details.
>
> So, create_workqueue() limits concurrency to 1 per cpu and if you have
> a dependency between two work items and they get scheduled on the same
> cpu they can deadlock.  system_wq doesn't have that restriction and
> should be fine, AFAICS.

Agreed, as long as WQ_DFL_ACTIVE is not reduced from its current value 
(256) to a very low value (e.g. 1 or 2). This assumption should be 
documented but I'm not sure what the best way is to document this...

Bart.