[PATCH 1/2 V3] workqueue: substitute POOL_FREEZING with __WQ_FREEZING

[PATCH 1/2 V3] workqueue: substitute POOL_FREEZING with __WQ_FREEZING

[Lai Jiangshan]

Only workqueues have freezable or freezing attribution/state, not worker pools.
But POOL_FREEZING adds a suspicious state and makes reviewers confused.

And it causes freeze_workqueues_begin() and thaw_workqueues() much complicated,
they need to travel all the pools besides wqs.

Since freezable is workqueue instance's attribution, and freezing
is workqueue instance's state, so we introduce __WQ_FREEZING
to wq->flags instead and remove POOL_FREEZING.

It is different from POOL_FREEZING, POOL_FREEZING is simply set
all over the world(all pools), while __WQ_FREEZING is only set for freezable wq.
freeze_workqueues_begin()/thaw_workqueues() skip to handle non-freezable wqs
and don't touch the non-freezable wqs' flags.

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

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 1b22c42..6bf353e 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -322,6 +322,7 @@ enum {
 
 	__WQ_DRAINING		= 1 << 16, /* internal: workqueue is draining */
 	__WQ_ORDERED		= 1 << 17, /* internal: workqueue is ordered */
+	__WQ_FREEZING		= 1 << 18, /* internel: workqueue is freezing */
 
 	WQ_MAX_ACTIVE		= 512,	  /* I like 512, better ideas? */
 	WQ_MAX_UNBOUND_PER_CPU	= 4,	  /* 4 * #cpus for unbound wq */
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index c3f076f..beca98b 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -70,7 +70,6 @@ enum {
 	 */
 	POOL_MANAGE_WORKERS	= 1 << 0,	/* need to manage workers */
 	POOL_DISASSOCIATED	= 1 << 2,	/* cpu can't serve workers */
-	POOL_FREEZING		= 1 << 3,	/* freeze in progress */
 
 	/* worker flags */
 	WORKER_STARTED		= 1 << 0,	/* started */
@@ -3662,9 +3661,6 @@ static struct worker_pool *get_unbound_pool(const struct workqueue_attrs *attrs)
 	if (!pool || init_worker_pool(pool) < 0)
 		goto fail;
 
-	if (workqueue_freezing)
-		pool->flags |= POOL_FREEZING;
-
 	lockdep_set_subclass(&pool->lock, 1);	/* see put_pwq() */
 	copy_workqueue_attrs(pool->attrs, attrs);
 
@@ -3762,7 +3758,7 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
 	struct workqueue_struct *wq = pwq->wq;
 	bool freezable = wq->flags & WQ_FREEZABLE;
 
-	/* for @wq->saved_max_active */
+	/* for @wq->saved_max_active and @wq->flags */
 	lockdep_assert_held(&wq->mutex);
 
 	/* fast exit for non-freezable wqs */
@@ -3771,7 +3767,7 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
 
 	spin_lock_irq(&pwq->pool->lock);
 
-	if (!freezable || !(pwq->pool->flags & POOL_FREEZING)) {
+	if (!freezable || !(wq->flags & __WQ_FREEZING)) {
 		pwq->max_active = wq->saved_max_active;
 
 		while (!list_empty(&pwq->delayed_works) &&
@@ -4277,6 +4273,8 @@ struct workqueue_struct *__alloc_workqueue_key(const char *fmt,
 	mutex_lock(&wq_pool_mutex);
 
 	mutex_lock(&wq->mutex);
+	if ((wq->flags & WQ_FREEZABLE) && workqueue_freezing)
+		wq->flags |= __WQ_FREEZING;
 	for_each_pwq(pwq, wq)
 		pwq_adjust_max_active(pwq);
 	mutex_unlock(&wq->mutex);
@@ -4883,26 +4881,20 @@ EXPORT_SYMBOL_GPL(work_on_cpu);
  */
 void freeze_workqueues_begin(void)
 {
-	struct worker_pool *pool;
 	struct workqueue_struct *wq;
 	struct pool_workqueue *pwq;
-	int pi;
 
 	mutex_lock(&wq_pool_mutex);
 
 	WARN_ON_ONCE(workqueue_freezing);
 	workqueue_freezing = true;
 
-	/* set FREEZING */
-	for_each_pool(pool, pi) {
-		spin_lock_irq(&pool->lock);
-		WARN_ON_ONCE(pool->flags & POOL_FREEZING);
-		pool->flags |= POOL_FREEZING;
-		spin_unlock_irq(&pool->lock);
-	}
-
 	list_for_each_entry(wq, &workqueues, list) {
+		if (!(wq->flags & WQ_FREEZABLE))
+			continue;
 		mutex_lock(&wq->mutex);
+		WARN_ON_ONCE(wq->flags & __WQ_FREEZING);
+		wq->flags |= __WQ_FREEZING;
 		for_each_pwq(pwq, wq)
 			pwq_adjust_max_active(pwq);
 		mutex_unlock(&wq->mutex);
@@ -4970,25 +4962,19 @@ void thaw_workqueues(void)
 {
 	struct workqueue_struct *wq;
 	struct pool_workqueue *pwq;
-	struct worker_pool *pool;
-	int pi;
 
 	mutex_lock(&wq_pool_mutex);
 
 	if (!workqueue_freezing)
 		goto out_unlock;
 
-	/* clear FREEZING */
-	for_each_pool(pool, pi) {
-		spin_lock_irq(&pool->lock);
-		WARN_ON_ONCE(!(pool->flags & POOL_FREEZING));
-		pool->flags &= ~POOL_FREEZING;
-		spin_unlock_irq(&pool->lock);
-	}
-
 	/* restore max_active and repopulate worklist */
 	list_for_each_entry(wq, &workqueues, list) {
+		if (!(wq->flags & WQ_FREEZABLE))
+			continue;
 		mutex_lock(&wq->mutex);
+		WARN_ON_ONCE(!(wq->flags & __WQ_FREEZING));
+		wq->flags &= ~__WQ_FREEZING;
 		for_each_pwq(pwq, wq)
 			pwq_adjust_max_active(pwq);
 		mutex_unlock(&wq->mutex);
-- 
1.7.4.4

[PATCH 2/2 V3] workqueue: simple refactor pwq_adjust_max_active()

[Lai Jiangshan]

Caculate the value(max_active) for pwq->max_active earlier.

Fast exit for max_active is not changed.
	"if (!freezable && pwq->max_active == wq->saved_max_active)" is hardly
	hit after __WQ_FREEZING was introduced. So we change the exit-condition
	to a more possible condition.

Reduce wake_up_worker() rate.

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

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index beca98b..59bad6e 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -198,7 +198,7 @@ struct pool_workqueue {
 	int			nr_in_flight[WORK_NR_COLORS];
 						/* L: nr of in_flight works */
 	int			nr_active;	/* L: nr of active works */
-	int			max_active;	/* L: max active works */
+	int			max_active;	/* L&WQ: max active works */
 	struct list_head	delayed_works;	/* L: delayed works */
 	struct list_head	pwqs_node;	/* WR: node on wq->pwqs */
 	struct list_head	mayday_node;	/* MD: node on wq->maydays */
@@ -3756,20 +3756,23 @@ static void pwq_unbound_release_workfn(struct work_struct *work)
 static void pwq_adjust_max_active(struct pool_workqueue *pwq)
 {
 	struct workqueue_struct *wq = pwq->wq;
-	bool freezable = wq->flags & WQ_FREEZABLE;
+	int max_active = wq->saved_max_active;
 
 	/* for @wq->saved_max_active and @wq->flags */
 	lockdep_assert_held(&wq->mutex);
 
-	/* fast exit for non-freezable wqs */
-	if (!freezable && pwq->max_active == wq->saved_max_active)
+	if (wq->flags & __WQ_FREEZING) {
+		if (!WARN_ON_ONCE(!(wq->flags & WQ_FREEZABLE)))
+			max_active = 0;
+	}
+
+	/* fast exit for max_active is not changed */
+	if (pwq->max_active == max_active)
 		return;
 
 	spin_lock_irq(&pwq->pool->lock);
-
-	if (!freezable || !(wq->flags & __WQ_FREEZING)) {
-		pwq->max_active = wq->saved_max_active;
-
+	pwq->max_active = max_active;
+	if (pwq->nr_active < pwq->max_active) {
 		while (!list_empty(&pwq->delayed_works) &&
 		       pwq->nr_active < pwq->max_active)
 			pwq_activate_first_delayed(pwq);
@@ -3779,8 +3782,6 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
 		 * max_active is bumped.  It's a slow path.  Do it always.
 		 */
 		wake_up_worker(pwq->pool);
-	} else {
-		pwq->max_active = 0;
 	}
 
 	spin_unlock_irq(&pwq->pool->lock);
-- 
1.7.4.4

Re: [PATCH 1/2 V3] workqueue: substitute POOL_FREEZING with __WQ_FREEZING

[Tejun Heo]

On Mon, Apr 21, 2014 at 07:59:20PM +0800, Lai Jiangshan wrote:
> Only workqueues have freezable or freezing attribution/state, not worker pools.
> But POOL_FREEZING adds a suspicious state and makes reviewers confused.
> 
> And it causes freeze_workqueues_begin() and thaw_workqueues() much complicated,
> they need to travel all the pools besides wqs.
> 
> Since freezable is workqueue instance's attribution, and freezing
> is workqueue instance's state, so we introduce __WQ_FREEZING
> to wq->flags instead and remove POOL_FREEZING.
> 
> It is different from POOL_FREEZING, POOL_FREEZING is simply set
> all over the world(all pools), while __WQ_FREEZING is only set for freezable wq.
> freeze_workqueues_begin()/thaw_workqueues() skip to handle non-freezable wqs
> and don't touch the non-freezable wqs' flags.

I was about to apply the patch and have updated the patch description.

  While freezing takes place globally, its execution is per-workqueue;
  however, the current implementation makes use of the per-worker_pool
  POOL_FREEZING flag.  While it's not broken, the flag makes the code
  more confusing and complicates freeze_workqueues_begin() and
  thaw_workqueues() by requiring them to walk through all pools.

  Since freezable is a workqueue's attribute, and freezing is a
  workqueue's state, let's introduce __WQ_FREEZING to wq->flags instead
  and remove POOL_FREEZING.

  It is different from POOL_FREEZING in that __WQ_FREEZING is only set
  for freezable workqueues while POOL_FREEZING is set globally over all
  pools.  freeze_workqueues_begin() and thaw_workqueues() now skip
  non-freezable workqueues.

But looking at the patch, why do we need __WQ_FREEZING at all?  We
should be able to test workqueue_freezing in pwq_adjust_max_active(),
right?  The only requirement there would be that
pwq_adjust_max_active(0 is invoked at least once after
workqueue_freezing is changed, which is already guaranteed.

Thanks.

-- 
tejun

Re: [PATCH 1/2 V3] workqueue: substitute POOL_FREEZING with __WQ_FREEZING

[Lai Jiangshan]

On 04/22/2014 06:20 AM, Tejun Heo wrote:
> On Mon, Apr 21, 2014 at 07:59:20PM +0800, Lai Jiangshan wrote:
>> Only workqueues have freezable or freezing attribution/state, not worker pools.
>> But POOL_FREEZING adds a suspicious state and makes reviewers confused.
>>
>> And it causes freeze_workqueues_begin() and thaw_workqueues() much complicated,
>> they need to travel all the pools besides wqs.
>>
>> Since freezable is workqueue instance's attribution, and freezing
>> is workqueue instance's state, so we introduce __WQ_FREEZING
>> to wq->flags instead and remove POOL_FREEZING.
>>
>> It is different from POOL_FREEZING, POOL_FREEZING is simply set
>> all over the world(all pools), while __WQ_FREEZING is only set for freezable wq.
>> freeze_workqueues_begin()/thaw_workqueues() skip to handle non-freezable wqs
>> and don't touch the non-freezable wqs' flags.
> 
> I was about to apply the patch and have updated the patch description.
> 
>   While freezing takes place globally, its execution is per-workqueue;
>   however, the current implementation makes use of the per-worker_pool
>   POOL_FREEZING flag.  While it's not broken, the flag makes the code
>   more confusing and complicates freeze_workqueues_begin() and
>   thaw_workqueues() by requiring them to walk through all pools.
> 
>   Since freezable is a workqueue's attribute, and freezing is a
>   workqueue's state, let's introduce __WQ_FREEZING to wq->flags instead
>   and remove POOL_FREEZING.
> 
>   It is different from POOL_FREEZING in that __WQ_FREEZING is only set
>   for freezable workqueues while POOL_FREEZING is set globally over all
>   pools.  freeze_workqueues_begin() and thaw_workqueues() now skip
>   non-freezable workqueues.
> 
> But looking at the patch, why do we need __WQ_FREEZING at all?  We
> should be able to test workqueue_freezing in pwq_adjust_max_active(),
> right?  The only requirement there would be that

Testing workqueue_freezing requires wq_pool_mutex held.
Although almost-all pwq_adjust_max_active() are called with wq_pool_mutex held,
except workqueue_set_max_active(). But I hope pwq_adjust_max_active()
don't require the heavy wq_pool_mutex.

> pwq_adjust_max_active() is invoked at least once after
> workqueue_freezing is changed, which is already guaranteed.
> 
> Thanks.
> 

Re: [PATCH 1/2 V3] workqueue: substitute POOL_FREEZING with __WQ_FREEZING

[Tejun Heo]

Hello,

On Tue, Apr 22, 2014 at 09:47:47AM +0800, Lai Jiangshan wrote:
> Testing workqueue_freezing requires wq_pool_mutex held.
> Although almost-all pwq_adjust_max_active() are called with wq_pool_mutex held,
> except workqueue_set_max_active(). But I hope pwq_adjust_max_active()
> don't require the heavy wq_pool_mutex.

No it doesn't require wq_pool_mutex to be held.  All it requires is
that the changed state is visible on the subsequent
pwq_adjust_max_active() invocatino which is already trivially
guaranteed.

Thanks.

-- 
tejun

Re: [PATCH 1/2 V3] workqueue: substitute POOL_FREEZING with __WQ_FREEZING

[Lai Jiangshan]

On 04/23/2014 04:46 AM, Tejun Heo wrote:
> Hello,
> 
> On Tue, Apr 22, 2014 at 09:47:47AM +0800, Lai Jiangshan wrote:
>> Testing workqueue_freezing requires wq_pool_mutex held.
>> Although almost-all pwq_adjust_max_active() are called with wq_pool_mutex held,
>> except workqueue_set_max_active(). But I hope pwq_adjust_max_active()
>> don't require the heavy wq_pool_mutex.
> 
> No it doesn't require wq_pool_mutex to be held.  All it requires is
> that the changed state is visible on the subsequent
> pwq_adjust_max_active() invocatino which is already trivially
> guaranteed.
> 

Good! I understood! Could you respin the patch? I'm afraid
I can't explain it well in the comments.

For me, I always prefer locks for non-performance critical path,
locks help review, I believe your comment will do so.

Thanks,
Lai