[PATCH 1/2 tj/for-3.10] workqueue: add wq->freezing and remove POOL_FREEZING

[PATCH 1/2 tj/for-3.10] workqueue: add wq->freezing and remove POOL_FREEZING

[Lai Jiangshan]

freezing is nothing related to pools, but POOL_FREEZING adds a connection,
and causes freeze_workqueues_begin() and thaw_workqueues() complicated.

Since freezing is workqueue instance attribute, so we introduce wq->freezing
instead and remove POOL_FREEZING.

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

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 04a8b98..6b7e5a4 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -66,7 +66,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 */
@@ -241,6 +240,7 @@ struct workqueue_struct {
 
 	int			nr_drainers;	/* WQ: drain in progress */
 	int			saved_max_active; /* WQ: saved pwq max_active */
+	bool			freezing;	/* WQ: the wq is freezing */
 
 #ifdef CONFIG_SYSFS
 	struct wq_device	*wq_dev;	/* I: for sysfs interface */
@@ -3493,9 +3493,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);
 
@@ -3571,7 +3568,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->freezing */
 	lockdep_assert_held(&wq->mutex);
 
 	/* fast exit for non-freezable wqs */
@@ -3580,7 +3577,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->freezing) {
 		pwq->max_active = wq->saved_max_active;
 
 		while (!list_empty(&pwq->delayed_works) &&
@@ -3795,6 +3792,7 @@ struct workqueue_struct *__alloc_workqueue_key(const char *fmt,
 	mutex_lock(&wq_pool_mutex);
 
 	mutex_lock(&wq->mutex);
+	wq->freezing = workqueue_freezing;
 	for_each_pwq(pwq, wq)
 		pwq_adjust_max_active(pwq);
 	mutex_unlock(&wq->mutex);
@@ -4282,26 +4280,18 @@ 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) {
 		mutex_lock(&wq->mutex);
+		WARN_ON_ONCE(wq->freezing);
+		wq->freezing = true;
 		for_each_pwq(pwq, wq)
 			pwq_adjust_max_active(pwq);
 		mutex_unlock(&wq->mutex);
@@ -4369,25 +4359,16 @@ 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) {
 		mutex_lock(&wq->mutex);
+		wq->freezing = false;
 		for_each_pwq(pwq, wq)
 			pwq_adjust_max_active(pwq);
 		mutex_unlock(&wq->mutex);
-- 
1.7.7.6

[PATCH 2/2 tj/for-3.10] workqueue: modify wq->freezing only when freezable

[Lai Jiangshan]

simplify pwq_adjust_max_active().
make freeze_workqueues_begin() and thaw_workqueues() fast skip non-freezable wq.

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

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 6b7e5a4..0a38852 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3566,18 +3566,15 @@ 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;
 
 	/* for @wq->saved_max_active and @wq->freezing */
 	lockdep_assert_held(&wq->mutex);
-
-	/* fast exit for non-freezable wqs */
-	if (!freezable && pwq->max_active == wq->saved_max_active)
-		return;
+	if (WARN_ON_ONCE(!(wq->flags & WQ_FREEZABLE) && wq->freezing))
+		wq->freezing = false;
 
 	spin_lock_irq(&pwq->pool->lock);
 
-	if (!freezable || !wq->freezing) {
+	if (!wq->freezing) {
 		pwq->max_active = wq->saved_max_active;
 
 		while (!list_empty(&pwq->delayed_works) &&
@@ -3792,7 +3789,8 @@ struct workqueue_struct *__alloc_workqueue_key(const char *fmt,
 	mutex_lock(&wq_pool_mutex);
 
 	mutex_lock(&wq->mutex);
-	wq->freezing = workqueue_freezing;
+	if (wq->flags & WQ_FREEZABLE)
+		wq->freezing = workqueue_freezing;
 	for_each_pwq(pwq, wq)
 		pwq_adjust_max_active(pwq);
 	mutex_unlock(&wq->mutex);
@@ -4289,6 +4287,8 @@ void freeze_workqueues_begin(void)
 	workqueue_freezing = true;
 
 	list_for_each_entry(wq, &workqueues, list) {
+		if (!(wq->flags & WQ_FREEZABLE))
+			continue;
 		mutex_lock(&wq->mutex);
 		WARN_ON_ONCE(wq->freezing);
 		wq->freezing = true;
@@ -4367,6 +4367,8 @@ void thaw_workqueues(void)
 
 	/* restore max_active and repopulate worklist */
 	list_for_each_entry(wq, &workqueues, list) {
+		if (!(wq->flags & WQ_FREEZABLE))
+			continue;
 		mutex_lock(&wq->mutex);
 		wq->freezing = false;
 		for_each_pwq(pwq, wq)
-- 
1.7.7.6

Re: [PATCH 1/2 tj/for-3.10] workqueue: add wq->freezing and remove POOL_FREEZING

[Tejun Heo]

On Sun, Mar 31, 2013 at 12:29:14AM +0800, Lai Jiangshan wrote:
> freezing is nothing related to pools, but POOL_FREEZING adds a connection,
> and causes freeze_workqueues_begin() and thaw_workqueues() complicated.
> 
> Since freezing is workqueue instance attribute, so we introduce wq->freezing
> instead and remove POOL_FREEZING.
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> ---
>  kernel/workqueue.c |   33 +++++++--------------------------
>  1 files changed, 7 insertions(+), 26 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 04a8b98..6b7e5a4 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -66,7 +66,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 */
> @@ -241,6 +240,7 @@ struct workqueue_struct {
>  
>  	int			nr_drainers;	/* WQ: drain in progress */
>  	int			saved_max_active; /* WQ: saved pwq max_active */
> +	bool			freezing;	/* WQ: the wq is freezing */

Why not use another internal flag?  There already are __WQ_DRAINING
and __WQ_ORDERED.  Can't we just add __WQ_FREEZING?

Thanks.

-- 
tejun

Re: [PATCH 1/2 tj/for-3.10] workqueue: add wq->freezing and remove POOL_FREEZING

[Lai Jiangshan]

On 04/02/2013 02:44 AM, Tejun Heo wrote:
> On Sun, Mar 31, 2013 at 12:29:14AM +0800, Lai Jiangshan wrote:
>> freezing is nothing related to pools, but POOL_FREEZING adds a connection,
>> and causes freeze_workqueues_begin() and thaw_workqueues() complicated.
>>
>> Since freezing is workqueue instance attribute, so we introduce wq->freezing
>> instead and remove POOL_FREEZING.
>>
>> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
>> ---
>>  kernel/workqueue.c |   33 +++++++--------------------------
>>  1 files changed, 7 insertions(+), 26 deletions(-)
>>
>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> index 04a8b98..6b7e5a4 100644
>> --- a/kernel/workqueue.c
>> +++ b/kernel/workqueue.c
>> @@ -66,7 +66,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 */
>> @@ -241,6 +240,7 @@ struct workqueue_struct {
>>  
>>  	int			nr_drainers;	/* WQ: drain in progress */
>>  	int			saved_max_active; /* WQ: saved pwq max_active */
>> +	bool			freezing;	/* WQ: the wq is freezing */
> 
> Why not use another internal flag?  There already are __WQ_DRAINING
> and __WQ_ORDERED.  Can't we just add __WQ_FREEZING?
> 
> Thanks.
> 



->flags is hot and almost read-only(except __WQ_DRAINING).
__WQ_DRAINING bit is accessed for every queue_work(), so we add it to hot ->flags.

__WQ_ORDERED is read-only.

->freezing is cold and non-read-only.
I don't think we need to add __WQ_FREEZING to ->flags.

Thanks,
Lai

Re: [PATCH 1/2 tj/for-3.10] workqueue: add wq->freezing and remove POOL_FREEZING

[Tejun Heo]

Hello, Lai.

On Tue, Apr 02, 2013 at 10:17:33AM +0800, Lai Jiangshan wrote:
> ->flags is hot and almost read-only(except __WQ_DRAINING).
> __WQ_DRAINING bit is accessed for every queue_work(), so we add it to hot ->flags.
> 
> __WQ_ORDERED is read-only.
> 
> ->freezing is cold and non-read-only.
> I don't think we need to add __WQ_FREEZING to ->flags.

Hmm... I'm not following.  If you have multiple hot flags which may be
used from different CPUs, yeah, you may want to put them in separate
cachelines but cold flags don't really matter either way so why put it
in a separate field?  Can you please elaborate?

Thanks.

-- 
tejun