[PATCH 1/2] workqueue: intruduce cwq_set_max_active() helper for thaw_workqueues()

[PATCH 1/2] workqueue: intruduce cwq_set_max_active() helper for thaw_workqueues()

[Lai Jiangshan]

Using a helper instead of open code makes thaw_workqueues() more clear.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 kernel/workqueue.c |   26 +++++++++++++++++++++-----
 1 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index cd05cf3..d0ca063 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3412,6 +3412,26 @@ void destroy_workqueue(struct workqueue_struct *wq)
 EXPORT_SYMBOL_GPL(destroy_workqueue);
 
 /**
+ * cwq_set_max_active - adjust max_active of a cwq
+ * @cwq: target cpu_workqueue_struct
+ * @max_active: new max_active value.
+ *
+ * Set max_active of @cwq to @max_active and activate delayed works
+ * if max_active is increased.
+ *
+ * CONTEXT:
+ * spin_lock_irq(gcwq->lock).
+ */
+static void cwq_set_max_active(struct cpu_workqueue_struct *cwq, int max_active)
+{
+	cwq->max_active = max_active;
+
+	while (!list_empty(&cwq->delayed_works) &&
+	       cwq->nr_active < cwq->max_active)
+		cwq_activate_first_delayed(cwq);
+}
+
+/**
  * workqueue_set_max_active - adjust max_active of a workqueue
  * @wq: target workqueue
  * @max_active: new max_active value.
@@ -3837,11 +3857,7 @@ void thaw_workqueues(void)
 				continue;
 
 			/* restore max_active and repopulate worklist */
-			cwq->max_active = wq->saved_max_active;
-
-			while (!list_empty(&cwq->delayed_works) &&
-			       cwq->nr_active < cwq->max_active)
-				cwq_activate_first_delayed(cwq);
+			cwq_set_max_active(cwq, wq->saved_max_active);
 		}
 
 		for_each_worker_pool(pool, gcwq)
-- 
1.7.4.4

[PATCH 2/2] workqueue: Keep activate-order equals to queue_work()-order

[Lai Jiangshan]

The whole workqueue.c keeps activate-order equals to queue_work()-order
in any given cwq except workqueue_set_max_active().

If this order is not kept, something may be not good:

first_work_fn() { release some resource; }
second_work_fn() { wait and request the resource; use resource; }

1. user queues the first work.	# ->max_active is low, is queued on ->delayed_works.
2. someone increases the >max_active via workqueue_set_max_active()
3. user queues the second work.	# queued on cwq->pool.

When the second work is launched to execute, it waits the first work
to release the resource. But the first work is still in ->delayed_works,
it waits the first work to finish and them it can be activated.

It is bad. we fix it by activating the first work in the step 2.

I can't fully determine that it is workqueue's responsibility
or the user's responsibility.
If it is workqueue's responsibility, the patch needs go to -stable.
If it is user's responsibility. it is a nice cleanup, it can go to for-next.
I prefer it is workqueue's responsibility.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 kernel/workqueue.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index d0ca063..8783414 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3458,7 +3458,7 @@ void workqueue_set_max_active(struct workqueue_struct *wq, int max_active)
 
 		if (!(wq->flags & WQ_FREEZABLE) ||
 		    !(gcwq->flags & GCWQ_FREEZING))
-			get_cwq(gcwq->cpu, wq)->max_active = max_active;
+			cwq_set_max_active(get_cwq(gcwq->cpu, wq), max_active);
 
 		spin_unlock_irq(&gcwq->lock);
 	}
-- 
1.7.4.4

Re: [PATCH 2/2] workqueue: Keep activate-order equals to queue_work()-order

[Tejun Heo]

On Tue, Sep 18, 2012 at 04:36:53PM +0800, Lai Jiangshan wrote:
> The whole workqueue.c keeps activate-order equals to queue_work()-order
> in any given cwq except workqueue_set_max_active().
> 
> If this order is not kept, something may be not good:
> 
> first_work_fn() { release some resource; }
> second_work_fn() { wait and request the resource; use resource; }
> 
> 1. user queues the first work.	# ->max_active is low, is queued on ->delayed_works.
> 2. someone increases the >max_active via workqueue_set_max_active()
> 3. user queues the second work.	# queued on cwq->pool.
> 
> When the second work is launched to execute, it waits the first work
> to release the resource. But the first work is still in ->delayed_works,
> it waits the first work to finish and them it can be activated.
> 
> It is bad. we fix it by activating the first work in the step 2.
> 
> I can't fully determine that it is workqueue's responsibility
> or the user's responsibility.
> If it is workqueue's responsibility, the patch needs go to -stable.
> If it is user's responsibility. it is a nice cleanup, it can go to for-next.
> I prefer it is workqueue's responsibility.

Unless max_active == 1, workqueue doesn't give any guarantee on
execution order.  I don't think we need to care about this.

Thanks.

-- 
tejun

Re: [PATCH 2/2] workqueue: Keep activate-order equals to queue_work()-order

[Tejun Heo]

On Tue, Sep 18, 2012 at 10:05:19AM -0700, Tejun Heo wrote:
> On Tue, Sep 18, 2012 at 04:36:53PM +0800, Lai Jiangshan wrote:
> > The whole workqueue.c keeps activate-order equals to queue_work()-order
> > in any given cwq except workqueue_set_max_active().
> > 
> > If this order is not kept, something may be not good:
> > 
> > first_work_fn() { release some resource; }
> > second_work_fn() { wait and request the resource; use resource; }
> > 
> > 1. user queues the first work.	# ->max_active is low, is queued on ->delayed_works.
> > 2. someone increases the >max_active via workqueue_set_max_active()
> > 3. user queues the second work.	# queued on cwq->pool.
> > 
> > When the second work is launched to execute, it waits the first work
> > to release the resource. But the first work is still in ->delayed_works,
> > it waits the first work to finish and them it can be activated.
> > 
> > It is bad. we fix it by activating the first work in the step 2.
> > 
> > I can't fully determine that it is workqueue's responsibility
> > or the user's responsibility.
> > If it is workqueue's responsibility, the patch needs go to -stable.
> > If it is user's responsibility. it is a nice cleanup, it can go to for-next.
> > I prefer it is workqueue's responsibility.
> 
> Unless max_active == 1, workqueue doesn't give any guarantee on
> execution order.  I don't think we need to care about this.

That said, I kinda like the patches.  Can you please update the
description on the second patch to something along the line of "use
common set_max_active logic which immediately makes use of the newly
increased max_mactive if there are delayed work items and also happens
to keep activation ordering"?

Thanks.

-- 
tejun

Re: [PATCH 2/2] workqueue: Keep activate-order equals to queue_work()-order

[Lai Jiangshan]

On 09/19/2012 01:05 AM, Tejun Heo wrote:
> On Tue, Sep 18, 2012 at 04:36:53PM +0800, Lai Jiangshan wrote:
>> The whole workqueue.c keeps activate-order equals to queue_work()-order
>> in any given cwq except workqueue_set_max_active().
>>
>> If this order is not kept, something may be not good:
>>
>> first_work_fn() { release some resource; }
>> second_work_fn() { wait and request the resource; use resource; }
>>
>> 1. user queues the first work.	# ->max_active is low, is queued on ->delayed_works.
>> 2. someone increases the >max_active via workqueue_set_max_active()
>> 3. user queues the second work.	# queued on cwq->pool.
>>
>> When the second work is launched to execute, it waits the first work
>> to release the resource. But the first work is still in ->delayed_works,
>> it waits the first work to finish and them it can be activated.
>>
>> It is bad. we fix it by activating the first work in the step 2.
>>
>> I can't fully determine that it is workqueue's responsibility
>> or the user's responsibility.
>> If it is workqueue's responsibility, the patch needs go to -stable.
>> If it is user's responsibility. it is a nice cleanup, it can go to for-next.
>> I prefer it is workqueue's responsibility.
> 
> Unless max_active == 1, workqueue doesn't give any guarantee on
> execution order.  I don't think we need to care about this.
> 
> 

(disorder of execution is OK for current WQ. because we can launch new worker
to execute the next work if the previous one is waiting something)

But I concern activate-order, not execution order. A non-delayed work
may delay a delayed work for ever, and if a non-delayed work needs something
which will be produced by delayed one, the two work may wait each other.

{
a subsystem queues a work to release resource.
and them
a subsystem queues a work to use the resource.
}
Is this behavior acceptable?

If this is acceptable, and if the first queued work go to ->delayed_works,
and a userspace user issues workqueue_set_max_active() via sysctl
before the second queuing. thus the second queued work will be activated
immediately. since the second work is activated,
the first work can't be activated before the first work finished.
thus the two work may wait each other.


All of this is abnormal thought. and it is not a real problem, merging them
as cleanup is enough.

Thanks,
Lai

Re: [PATCH 2/2] workqueue: Keep activate-order equals to queue_work()-order

[Lai Jiangshan]

On 09/19/2012 01:08 AM, Tejun Heo wrote:
> On Tue, Sep 18, 2012 at 10:05:19AM -0700, Tejun Heo wrote:
>> On Tue, Sep 18, 2012 at 04:36:53PM +0800, Lai Jiangshan wrote:
>>> The whole workqueue.c keeps activate-order equals to queue_work()-order
>>> in any given cwq except workqueue_set_max_active().
>>>
>>> If this order is not kept, something may be not good:
>>>
>>> first_work_fn() { release some resource; }
>>> second_work_fn() { wait and request the resource; use resource; }
>>>
>>> 1. user queues the first work.	# ->max_active is low, is queued on ->delayed_works.
>>> 2. someone increases the >max_active via workqueue_set_max_active()
>>> 3. user queues the second work.	# queued on cwq->pool.
>>>
>>> When the second work is launched to execute, it waits the first work
>>> to release the resource. But the first work is still in ->delayed_works,
>>> it waits the first work to finish and them it can be activated.
>>>
>>> It is bad. we fix it by activating the first work in the step 2.
>>>
>>> I can't fully determine that it is workqueue's responsibility
>>> or the user's responsibility.
>>> If it is workqueue's responsibility, the patch needs go to -stable.
>>> If it is user's responsibility. it is a nice cleanup, it can go to for-next.
>>> I prefer it is workqueue's responsibility.
>>
>> Unless max_active == 1, workqueue doesn't give any guarantee on
>> execution order.  I don't think we need to care about this.
> 
> That said, I kinda like the patches.  Can you please update the
> description on the second patch to something along the line of "use
> common set_max_active logic which immediately makes use of the newly
> increased max_mactive if there are delayed work items and also happens
> to keep activation ordering"?
> 
> Thanks.
> 

Updated.
Thanks,
Lai

>From 314d43f087c85b11a29be0555f32deeb742bf18e Mon Sep 17 00:00:00 2001
From: Lai Jiangshan <laijs@cn.fujitsu.com>
Date: Tue, 18 Sep 2012 16:26:30 +0800
Subject: [PATCH] workqueue: use common cwq_set_max_active() for
 workqueue_set_max_active()

workqueue_set_max_active() may increase ->max_active without activating
delayed works. And it may cause the activation order doesn't equal to
to queue_work()-order.

To make things consist, we use common cwq_set_max_active() logic which
immediately makes use of the newly increased max_mactive if there are
delayed work items and also keep activation ordering.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 kernel/workqueue.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index d0ca063..8783414 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3458,7 +3458,7 @@ void workqueue_set_max_active(struct workqueue_struct *wq, int max_active)
 
 		if (!(wq->flags & WQ_FREEZABLE) ||
 		    !(gcwq->flags & GCWQ_FREEZING))
-			get_cwq(gcwq->cpu, wq)->max_active = max_active;
+			cwq_set_max_active(get_cwq(gcwq->cpu, wq), max_active);
 
 		spin_unlock_irq(&gcwq->lock);
 	}
-- 
1.7.4.4

Re: [PATCH 2/2] workqueue: Keep activate-order equals to queue_work()-order

[Tejun Heo]

Hello, Lai.

On Wed, Sep 19, 2012 at 05:57:12PM +0800, Lai Jiangshan wrote:
> (disorder of execution is OK for current WQ. because we can launch new worker
> to execute the next work if the previous one is waiting something)
> 
> But I concern activate-order, not execution order. A non-delayed work
> may delay a delayed work for ever, and if a non-delayed work needs something
> which will be produced by delayed one, the two work may wait each other.
> 
> {
> a subsystem queues a work to release resource.
> and them
> a subsystem queues a work to use the resource.
> }
> Is this behavior acceptable?

Even on workqueues with a rescuer, forward progress is not guaranteed
if there are more than one co-dependent work items.  workqueue doesn't
guarantee anything regarding activation or execution order and any
user which depends on that is broken.

In general, I think it's a bad idea to give that kind of guarantee and
encourage such usages which can lead to extremely subtle breakages
which cannot be detected in any automated way - we don't have any way
to mark dependencies among work items.

Thanks.

-- 
tejun

Re: [PATCH 2/2] workqueue: Keep activate-order equals to queue_work()-order

[Tejun Heo]

Hello, Lai.

On Wed, Sep 19, 2012 at 06:13:24PM +0800, Lai Jiangshan wrote:
> From 314d43f087c85b11a29be0555f32deeb742bf18e Mon Sep 17 00:00:00 2001
> From: Lai Jiangshan <laijs@cn.fujitsu.com>
> Date: Tue, 18 Sep 2012 16:26:30 +0800
> Subject: [PATCH] workqueue: use common cwq_set_max_active() for
>  workqueue_set_max_active()
> 
> workqueue_set_max_active() may increase ->max_active without activating
> delayed works. And it may cause the activation order doesn't equal to
> to queue_work()-order.
> 
> To make things consist, we use common cwq_set_max_active() logic which
> immediately makes use of the newly increased max_mactive if there are
> delayed work items and also keep activation ordering.
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>

Applied 1-2 to wq/for-3.7 with minor updates to comments and
descriptions.

Thanks for doing this!

-- 
tejun