[PATCH 1/2] workqueue: remove unneeded test before wake up next worker

[PATCH 1/2] workqueue: remove unneeded test before wake up next worker

[Lai Jiangshan]

In this code:
	if ((worker->flags & WORKER_UNBOUND) && need_more_worker(pool))
		wake_up_worker(pool);

the first test is unneeded. Even the first test is removed, it doesn't affect
the wake-up logic when WORKER_UNBOUND. And it will not introduce any useless
wake-up when !WORKER_UNBOUND since the nr_running >= 1 except only one case.
It will introduce useless/redundant wake-up when cpu_intensive, but this
case is rare and next patch will also remove this redundant wake-up.

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

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index f8d54c1..6d11b9a 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2047,11 +2047,8 @@ __acquires(&pool->lock)
 	if (unlikely(cpu_intensive))
 		worker_set_flags(worker, WORKER_CPU_INTENSIVE, true);
 
-	/*
-	 * Unbound pool isn't concurrency managed and work items should be
-	 * executed ASAP.  Wake up another worker if necessary.
-	 */
-	if ((worker->flags & WORKER_UNBOUND) && need_more_worker(pool))
+	/* Wake up another worker if necessary. */
+	if (need_more_worker(pool))
 		wake_up_worker(pool);
 
 	/*
-- 
1.7.4.4

[PATCH 2/2] workqueue: remove the argument @wakeup from worker_set_flags()

[Lai Jiangshan]

worker_set_flags() doesn't necessarily wake next worker and the @wakeup
can be removed, the caller can use the following conbination instead
when needed:

	worker_set_flags();
	if (need_more_worker(pool))
 		wake_up_worker(pool);
	

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

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 6d11b9a..1a14f18 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -867,35 +867,22 @@ struct task_struct *wq_worker_sleeping(struct task_struct *task, int cpu)
  * worker_set_flags - set worker flags and adjust nr_running accordingly
  * @worker: self
  * @flags: flags to set
- * @wakeup: wakeup an idle worker if necessary
  *
- * Set @flags in @worker->flags and adjust nr_running accordingly.  If
- * nr_running becomes zero and @wakeup is %true, an idle worker is
- * woken up.
+ * Set @flags in @worker->flags and adjust nr_running accordingly.
  *
  * CONTEXT:
  * spin_lock_irq(pool->lock)
  */
-static inline void worker_set_flags(struct worker *worker, unsigned int flags,
-				    bool wakeup)
+static inline void worker_set_flags(struct worker *worker, unsigned int flags)
 {
 	struct worker_pool *pool = worker->pool;
 
 	WARN_ON_ONCE(worker->task != current);
 
-	/*
-	 * If transitioning into NOT_RUNNING, adjust nr_running and
-	 * wake up an idle worker as necessary if requested by
-	 * @wakeup.
-	 */
+	/* If transitioning into NOT_RUNNING, adjust nr_running. */
 	if ((flags & WORKER_NOT_RUNNING) &&
 	    !(worker->flags & WORKER_NOT_RUNNING)) {
-		if (wakeup) {
-			if (atomic_dec_and_test(&pool->nr_running) &&
-			    !list_empty(&pool->worklist))
-				wake_up_worker(pool);
-		} else
-			atomic_dec(&pool->nr_running);
+		atomic_dec(&pool->nr_running);
 	}
 
 	worker->flags |= flags;
@@ -2045,7 +2032,7 @@ __acquires(&pool->lock)
 	 * management.  They're the scheduler's responsibility.
 	 */
 	if (unlikely(cpu_intensive))
-		worker_set_flags(worker, WORKER_CPU_INTENSIVE, true);
+		worker_set_flags(worker, WORKER_CPU_INTENSIVE);
 
 	/* Wake up another worker if necessary. */
 	if (need_more_worker(pool))
@@ -2204,7 +2191,7 @@ recheck:
 		}
 	} while (keep_working(pool));
 
-	worker_set_flags(worker, WORKER_PREP, false);
+	worker_set_flags(worker, WORKER_PREP);
 sleep:
 	/*
 	 * pool->lock is held and there's no work to process and no need to
-- 
1.7.4.4

Re: [PATCH 2/2] workqueue: remove the argument @wakeup from worker_set_flags()

[Tejun Heo]

On Wed, Jul 16, 2014 at 06:09:59PM +0800, Lai Jiangshan wrote:
> worker_set_flags() doesn't necessarily wake next worker and the @wakeup
> can be removed, the caller can use the following conbination instead
> when needed:
> 
> 	worker_set_flags();
> 	if (need_more_worker(pool))
>  		wake_up_worker(pool);

Hmmm, yeah, there were more places where worker_set_flags() was used
but it does seem excessive now.

> @@ -2045,7 +2032,7 @@ __acquires(&pool->lock)
>  	 * management.  They're the scheduler's responsibility.
>  	 */
>  	if (unlikely(cpu_intensive))
> -		worker_set_flags(worker, WORKER_CPU_INTENSIVE, true);
> +		worker_set_flags(worker, WORKER_CPU_INTENSIVE);

But let's do this separately.  Please drop the previous patch and
perform need_more_worker() test explicitly after setting
CPU_INTENSIVE.

Thanks.

-- 
tejun

Re: [PATCH 2/2] workqueue: remove the argument @wakeup from worker_set_flags()

[Tejun Heo]

On Fri, Jul 18, 2014 at 06:38:26PM -0400, Tejun Heo wrote:
> On Wed, Jul 16, 2014 at 06:09:59PM +0800, Lai Jiangshan wrote:
> > worker_set_flags() doesn't necessarily wake next worker and the @wakeup
> > can be removed, the caller can use the following conbination instead
> > when needed:
> > 
> > 	worker_set_flags();
> > 	if (need_more_worker(pool))
> >  		wake_up_worker(pool);
> 
> Hmmm, yeah, there were more places where worker_set_flags() was used
> but it does seem excessive now.
> 
> > @@ -2045,7 +2032,7 @@ __acquires(&pool->lock)
> >  	 * management.  They're the scheduler's responsibility.
> >  	 */
> >  	if (unlikely(cpu_intensive))
> > -		worker_set_flags(worker, WORKER_CPU_INTENSIVE, true);
> > +		worker_set_flags(worker, WORKER_CPU_INTENSIVE);
> 
> But let's do this separately.  Please drop the previous patch and
> perform need_more_worker() test explicitly after setting
> CPU_INTENSIVE.

So, we can do it together at need_more_workers() but let's please
explain how different cases would behave there.

Thanks.

-- 
tejun

Re: [PATCH 1/2] workqueue: remove unneeded test before wake up next worker

[Tejun Heo]

On Wed, Jul 16, 2014 at 06:09:58PM +0800, Lai Jiangshan wrote:
> In this code:
> 	if ((worker->flags & WORKER_UNBOUND) && need_more_worker(pool))
> 		wake_up_worker(pool);
> 
> the first test is unneeded. Even the first test is removed, it doesn't affect
> the wake-up logic when WORKER_UNBOUND. And it will not introduce any useless
> wake-up when !WORKER_UNBOUND since the nr_running >= 1 except only one case.
> It will introduce useless/redundant wake-up when cpu_intensive, but this
> case is rare and next patch will also remove this redundant wake-up.
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> ---
>  kernel/workqueue.c |    7 ++-----
>  1 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index f8d54c1..6d11b9a 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -2047,11 +2047,8 @@ __acquires(&pool->lock)
>  	if (unlikely(cpu_intensive))
>  		worker_set_flags(worker, WORKER_CPU_INTENSIVE, true);
>  
> -	/*
> -	 * Unbound pool isn't concurrency managed and work items should be
> -	 * executed ASAP.  Wake up another worker if necessary.
> -	 */
> -	if ((worker->flags & WORKER_UNBOUND) && need_more_worker(pool))
> +	/* Wake up another worker if necessary. */
> +	if (need_more_worker(pool))
>  		wake_up_worker(pool);

What does this buy us?  Sure, it may achieve about the same operation
but it's a lot more confusing.  need_more_worker() is specifically for
concurrency management.  Applying it to unmanaged workers could lead
to okay behavior but conflating the two to save one test on worker
flags doesn't seem like a good trade-off to me.

Thanks.

-- 
tejun

Re: [PATCH 1/2] workqueue: remove unneeded test before wake up next worker

[Tejun Heo]

On Fri, Jul 18, 2014 at 06:05:14PM -0400, Tejun Heo wrote:
> On Wed, Jul 16, 2014 at 06:09:58PM +0800, Lai Jiangshan wrote:
> > In this code:
> > 	if ((worker->flags & WORKER_UNBOUND) && need_more_worker(pool))
> > 		wake_up_worker(pool);
> > 
> > the first test is unneeded. Even the first test is removed, it doesn't affect
> > the wake-up logic when WORKER_UNBOUND. And it will not introduce any useless
> > wake-up when !WORKER_UNBOUND since the nr_running >= 1 except only one case.
> > It will introduce useless/redundant wake-up when cpu_intensive, but this
> > case is rare and next patch will also remove this redundant wake-up.
> > 
> > Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> > ---
> >  kernel/workqueue.c |    7 ++-----
> >  1 files changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index f8d54c1..6d11b9a 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -2047,11 +2047,8 @@ __acquires(&pool->lock)
> >  	if (unlikely(cpu_intensive))
> >  		worker_set_flags(worker, WORKER_CPU_INTENSIVE, true);
> >  
> > -	/*
> > -	 * Unbound pool isn't concurrency managed and work items should be
> > -	 * executed ASAP.  Wake up another worker if necessary.
> > -	 */
> > -	if ((worker->flags & WORKER_UNBOUND) && need_more_worker(pool))
> > +	/* Wake up another worker if necessary. */
> > +	if (need_more_worker(pool))
> >  		wake_up_worker(pool);
> 
> What does this buy us?  Sure, it may achieve about the same operation
> but it's a lot more confusing.  need_more_worker() is specifically for
> concurrency management.  Applying it to unmanaged workers could lead
> to okay behavior but conflating the two to save one test on worker
> flags doesn't seem like a good trade-off to me.

I take this back.  We do guarantee that need_more_worker() returns
%true for unbound pools and make use of that fact but I'd like it to
retain the comment about unbound pools.

Thanks.

-- 
tejun

[PATCH 1/2 V2] workqueue: remove unneeded test before wake up next worker

[Lai Jiangshan]

In this code:
	if ((worker->flags & WORKER_UNBOUND) && need_more_worker(pool))
		wake_up_worker(pool);

the first test is unneeded. Even the first test is removed, it doesn't affect
the wake-up logic when WORKER_UNBOUND. And it will not introduce any useless
wake-up when !WORKER_UNBOUND since the nr_running >= 1 except only one case.
It will introduce useless/redundant wake-up when cpu_intensive, but this
case is rare and next patch will also remove this redundant wake-up.

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

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index a791a8c..cd75689 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2048,10 +2048,13 @@ __acquires(&pool->lock)
 		worker_set_flags(worker, WORKER_CPU_INTENSIVE, true);
 
 	/*
-	 * Unbound pool isn't concurrency managed and work items should be
-	 * executed ASAP.  Wake up another worker if necessary.
+	 * Wake up another worker if necessary.  It is a no-op
+	 * when the current worker is concurrency managed since
+	 * pool->nr_running >= 1.  But it is required for non-concurrency
+	 * managed workers, mainly for unbound pool which requries
+	 * chain execution of currently pending work items ASAP.
 	 */
-	if ((worker->flags & WORKER_UNBOUND) && need_more_worker(pool))
+	if (need_more_worker(pool))
 		wake_up_worker(pool);
 
 	/*
-- 
1.7.4.4

[PATCH 2/2 V2] workqueue: remove the argument @wakeup from worker_set_flags()

[Lai Jiangshan]

worker_set_flags() doesn't necessarily wake next worker and the @wakeup
can be removed, the caller can use the following conbination instead
when needed:

	worker_set_flags();
	if (need_more_worker(pool))
 		wake_up_worker(pool);

It reduces the machine-code and footprint for the process_one_work()
if worker_set_flags() is really inlined.

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

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index cd75689..40b1f00 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -867,35 +867,22 @@ struct task_struct *wq_worker_sleeping(struct task_struct *task, int cpu)
  * worker_set_flags - set worker flags and adjust nr_running accordingly
  * @worker: self
  * @flags: flags to set
- * @wakeup: wakeup an idle worker if necessary
  *
- * Set @flags in @worker->flags and adjust nr_running accordingly.  If
- * nr_running becomes zero and @wakeup is %true, an idle worker is
- * woken up.
+ * Set @flags in @worker->flags and adjust nr_running accordingly.
  *
  * CONTEXT:
  * spin_lock_irq(pool->lock)
  */
-static inline void worker_set_flags(struct worker *worker, unsigned int flags,
-				    bool wakeup)
+static inline void worker_set_flags(struct worker *worker, unsigned int flags)
 {
 	struct worker_pool *pool = worker->pool;
 
 	WARN_ON_ONCE(worker->task != current);
 
-	/*
-	 * If transitioning into NOT_RUNNING, adjust nr_running and
-	 * wake up an idle worker as necessary if requested by
-	 * @wakeup.
-	 */
+	/* If transitioning into NOT_RUNNING, adjust nr_running. */
 	if ((flags & WORKER_NOT_RUNNING) &&
 	    !(worker->flags & WORKER_NOT_RUNNING)) {
-		if (wakeup) {
-			if (atomic_dec_and_test(&pool->nr_running) &&
-			    !list_empty(&pool->worklist))
-				wake_up_worker(pool);
-		} else
-			atomic_dec(&pool->nr_running);
+		atomic_dec(&pool->nr_running);
 	}
 
 	worker->flags |= flags;
@@ -2043,9 +2030,11 @@ __acquires(&pool->lock)
 	/*
 	 * CPU intensive works don't participate in concurrency
 	 * management.  They're the scheduler's responsibility.
+	 * Another worker should be woken up after WORKER_CPU_INTENSIVE
+	 * is set, see next code.
 	 */
 	if (unlikely(cpu_intensive))
-		worker_set_flags(worker, WORKER_CPU_INTENSIVE, true);
+		worker_set_flags(worker, WORKER_CPU_INTENSIVE);
 
 	/*
 	 * Wake up another worker if necessary.  It is a no-op
@@ -2210,7 +2199,7 @@ recheck:
 		}
 	} while (keep_working(pool));
 
-	worker_set_flags(worker, WORKER_PREP, false);
+	worker_set_flags(worker, WORKER_PREP);
 sleep:
 	/*
 	 * pool->lock is held and there's no work to process and no need to
-- 
1.7.4.4

Re: [PATCH 2/2 V2] workqueue: remove the argument @wakeup from worker_set_flags()

[Tejun Heo]

On Tue, Jul 22, 2014 at 01:02:00PM +0800, Lai Jiangshan wrote:
> worker_set_flags() doesn't necessarily wake next worker and the @wakeup
> can be removed, the caller can use the following conbination instead
> when needed:
> 
> 	worker_set_flags();
> 	if (need_more_worker(pool))
>  		wake_up_worker(pool);
> 
> It reduces the machine-code and footprint for the process_one_work()
> if worker_set_flags() is really inlined.
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>

Applied 1-2 to wq/for-3.17 w/ descriptions and comments updated.

Thanks.

-- 
tejun