[PATCH 00/12] workqueue: simple cleanups

[PATCH 00/12] workqueue: simple cleanups

[Lai Jiangshan]

these all are different cleanups for workqueue.

depends:
Patch2 depends on Patch1
Patch3 depends on Patch1
Patch7 depends on Patch6

Patch7 need to be merged after Patch3 is merged.(not really depends)

Lai Jiangshan (12):
  workqueue: add WORKER_RESCUER
  workqueue: disallow set_cpus_allowed_ptr() from work item
  workqueue: remove WORKER_PREP from rescuer
  workqueue: simplify is_chained_work()
  workqueue: don't wake up other workers in rescuer
  workqueue: destroy_worker() can only destory idle worker not just
    created worker
  workqueue: remove WORKER_STARTED
  workqueue: fix comments of insert_work()
  workqueue: declare system_highpri_wq
  cpu-hotplug.txt: fix comments of work_on_cpu()
  workqueue: add WQ_CPU_INTENSIVE to system_long_wq
  workqueue: avoid work_on_cpu() to interfere system_wq

 Documentation/cpu-hotplug.txt |    2 +-
 include/linux/workqueue.h     |    4 ++
 kernel/sched/core.c           |    8 ++-
 kernel/workqueue.c            |   82 +++++++++++++++++------------------------
 4 files changed, 44 insertions(+), 52 deletions(-)

-- 
1.7.7.6

[PATCH 01/12] workqueue: add WORKER_RESCUER

[Lai Jiangshan]

rescuer thread must be a worker which is WORKER_NOT_RUNNING:
	If it is *not* WORKER_NOT_RUNNING, it will increase the nr_running
	and it disables the normal workers wrongly.

So rescuer thread must be WORKER_NOT_RUNNING.

Currently code implement it by always setting WORKER_PREP on rescuer thread,
but this kind of implement is ugly:
A)	It reuses WORKER_PREP which is used for a different meaning.
B)	It does not told us rescuer thread is WORKER_NOT_RUNNING.

So we add WORKER_RESCUER to fix these two sematic.

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

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 737ab01..ec882a6 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -73,11 +73,12 @@ enum {
 	WORKER_DIE		= 1 << 1,	/* die die die */
 	WORKER_IDLE		= 1 << 2,	/* is idle */
 	WORKER_PREP		= 1 << 3,	/* preparing to run works */
+	WORKER_RESCUER		= 1 << 4,	/* rescuer thread */
 	WORKER_CPU_INTENSIVE	= 1 << 6,	/* cpu intensive */
 	WORKER_UNBOUND		= 1 << 7,	/* worker is unbound */
 
 	WORKER_NOT_RUNNING	= WORKER_PREP | WORKER_UNBOUND |
-				  WORKER_CPU_INTENSIVE,
+				  WORKER_RESCUER | WORKER_CPU_INTENSIVE,
 
 	NR_WORKER_POOLS		= 2,		/* # worker pools per gcwq */
 
@@ -2405,6 +2406,7 @@ static int rescuer_thread(void *__wq)
 	bool is_unbound = wq->flags & WQ_UNBOUND;
 	unsigned int cpu;
 
+	rescuer->flags |= WORKER_RESCUER;
 	set_user_nice(current, RESCUER_NICE_LEVEL);
 repeat:
 	set_current_state(TASK_INTERRUPTIBLE);
-- 
1.7.7.6

Re: [PATCH 01/12] workqueue: add WORKER_RESCUER

[Tejun Heo]

On Thu, Sep 27, 2012 at 01:20:32AM +0800, Lai Jiangshan wrote:
> rescuer thread must be a worker which is WORKER_NOT_RUNNING:
> 	If it is *not* WORKER_NOT_RUNNING, it will increase the nr_running
> 	and it disables the normal workers wrongly.
> 
> So rescuer thread must be WORKER_NOT_RUNNING.
> 
> Currently code implement it by always setting WORKER_PREP on rescuer thread,
> but this kind of implement is ugly:
> A)	It reuses WORKER_PREP which is used for a different meaning.
> B)	It does not told us rescuer thread is WORKER_NOT_RUNNING.
> 
> So we add WORKER_RESCUER to fix these two sematic.

Ah, right, we always have WORKER_PREP set for rescuers.  So, this
doesn't actually change the behavior at all?  I'm not necessarily
against it but the commit message seems a bit misleading.

Thanks.

-- 
tejun

Re: [PATCH 01/12] workqueue: add WORKER_RESCUER

[Lai Jiangshan]

On 09/27/2012 02:07 AM, Tejun Heo wrote:
> On Thu, Sep 27, 2012 at 01:20:32AM +0800, Lai Jiangshan wrote:
>> rescuer thread must be a worker which is WORKER_NOT_RUNNING:
>> 	If it is *not* WORKER_NOT_RUNNING, it will increase the nr_running
>> 	and it disables the normal workers wrongly.
>>
>> So rescuer thread must be WORKER_NOT_RUNNING.
>>
>> Currently code implement it by always setting WORKER_PREP on rescuer thread,
>> but this kind of implement is ugly:
>> A)	It reuses WORKER_PREP which is used for a different meaning.
>> B)	It does not told us rescuer thread is WORKER_NOT_RUNNING.
>>
>> So we add WORKER_RESCUER to fix these two sematic.
> 
> Ah, right, we always have WORKER_PREP set for rescuers.  So, this
> doesn't actually change the behavior at all?  

No, this doesn't change the behavior at all.

> I'm not necessarily
> against it but the commit message seems a bit misleading.
> 

I just try my best to say" we need to add WORKER_RESCUER to told us
rescuer is WORKER_NOT_RUNNING explicity, using WORKER_PREP only will
hide this info"

Thanks,
Lai

[PATCH 02/12] workqueue: disallow set_cpus_allowed_ptr() from work item

[Lai Jiangshan]

workers depend on local-wake-up, if a work function change its CPU, it will
corrupt workqueue, disallow this behavior.

When set_cpus_allowed_ptr() is called from workqueue.c in worker_thread(),
we clear the PF_WQ_WORKER before set_cpus_allowed_ptr() and set it back
after. (rescuer thread has no PF_WQ_WORKER, skip this behavior)

It prevents other/future BUGs like https://bugzilla.kernel.org/show_bug.cgi?id=47301.

CC: tangchen <tangchen@cn.fujitsu.com>
Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 kernel/sched/core.c |    8 +++++---
 kernel/workqueue.c  |   10 +++++++++-
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index d325c4b..355d3cc 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5187,9 +5187,11 @@ int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask)
 		goto out;
 	}
 
-	if (unlikely((p->flags & PF_THREAD_BOUND) && p != current)) {
-		ret = -EINVAL;
-		goto out;
+	if (unlikely(p->flags & PF_THREAD_BOUND)) {
+		if (WARN_ON_ONCE(p->flags & PF_WQ_WORKER) || p != current) {
+			ret = -EINVAL;
+			goto out;
+		}
 	}
 
 	do_set_cpus_allowed(p, new_mask);
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index ec882a6..c55884d 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1600,6 +1600,7 @@ __acquires(&gcwq->lock)
 	struct global_cwq *gcwq = worker->pool->gcwq;
 	struct task_struct *task = worker->task;
 
+	BUG_ON(task != current);
 	while (true) {
 		/*
 		 * The following call may fail, succeed or succeed
@@ -1607,9 +1608,16 @@ __acquires(&gcwq->lock)
 		 * it races with cpu hotunplug operation.  Verify
 		 * against GCWQ_DISASSOCIATED.
 		 */
-		if (!(gcwq->flags & GCWQ_DISASSOCIATED))
+		if (!(gcwq->flags & GCWQ_DISASSOCIATED)) {
+			if (!(worker->flags & WORKER_RESCUER))
+				task->flags &= ~PF_WQ_WORKER;
+
 			set_cpus_allowed_ptr(task, get_cpu_mask(gcwq->cpu));
 
+			if (!(worker->flags & WORKER_RESCUER))
+				task->flags |= PF_WQ_WORKER;
+		}
+
 		spin_lock_irq(&gcwq->lock);
 		if (gcwq->flags & GCWQ_DISASSOCIATED)
 			return false;
-- 
1.7.7.6

Re: [PATCH 02/12] workqueue: disallow set_cpus_allowed_ptr() from work item

[Tejun Heo]

On Thu, Sep 27, 2012 at 01:20:33AM +0800, Lai Jiangshan wrote:
> workers depend on local-wake-up, if a work function change its CPU, it will
> corrupt workqueue, disallow this behavior.
> 
> When set_cpus_allowed_ptr() is called from workqueue.c in worker_thread(),
> we clear the PF_WQ_WORKER before set_cpus_allowed_ptr() and set it back
> after. (rescuer thread has no PF_WQ_WORKER, skip this behavior)
> 
> It prevents other/future BUGs like https://bugzilla.kernel.org/show_bug.cgi?id=47301.

Hmmm... maybe.  We better clear up PF_THREAD_BOUND semantics first
tho.

I think the two possible choices are,

* Don't allow set_cpus_allowed_ptr() at all and make the caller
  responsible for turning it off if it explicitly wants to change
  cpus_allowed.

* Don't allow changing affinity from userland.

Peter, Thomas?

-- 
tejun

[PATCH 03/12] workqueue: remove WORKER_PREP from rescuer

[Lai Jiangshan]

There is no reason to use WORKER_PREP, remove it from rescuer.

And there is no reason to set it so early in alloc_worker(),
move "worker->flags = WORKER_PREP" to start_worker().

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

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index c55884d..e41c562 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1759,8 +1759,6 @@ static struct worker *alloc_worker(void)
 		INIT_LIST_HEAD(&worker->entry);
 		INIT_LIST_HEAD(&worker->scheduled);
 		INIT_WORK(&worker->rebind_work, busy_worker_rebind_fn);
-		/* on creation a worker is in !idle && prep state */
-		worker->flags = WORKER_PREP;
 	}
 	return worker;
 }
@@ -1854,6 +1852,7 @@ fail:
 static void start_worker(struct worker *worker)
 {
 	worker->flags |= WORKER_STARTED;
+	worker->flags |= WORKER_PREP;
 	worker->pool->nr_workers++;
 	worker_enter_idle(worker);
 	wake_up_process(worker->task);
-- 
1.7.7.6

Re: [PATCH 03/12] workqueue: remove WORKER_PREP from rescuer

[Tejun Heo]

On Thu, Sep 27, 2012 at 01:20:34AM +0800, Lai Jiangshan wrote:
> There is no reason to use WORKER_PREP, remove it from rescuer.
> 
> And there is no reason to set it so early in alloc_worker(),
> move "worker->flags = WORKER_PREP" to start_worker().

Merge this with the patch to add RESCUER to NOT_RUNNING?

-- 
tejun

Re: [PATCH 03/12] workqueue: remove WORKER_PREP from rescuer

[Lai Jiangshan]

On 09/27/2012 02:24 AM, Tejun Heo wrote:
> On Thu, Sep 27, 2012 at 01:20:34AM +0800, Lai Jiangshan wrote:
>> There is no reason to use WORKER_PREP, remove it from rescuer.
>>
>> And there is no reason to set it so early in alloc_worker(),
>> move "worker->flags = WORKER_PREP" to start_worker().
> 
> Merge this with the patch to add RESCUER to NOT_RUNNING?
> 

Hi, Tejun:

Don't merge. the change log is not good.
I should list all the purposes of WORKER_PREP(I did it in my note),
and then we can claim that we don't need to add WORKER_PREP show early.

These two patches do different things.

Thanks,
Lai

Re: [PATCH 03/12] workqueue: remove WORKER_PREP from rescuer

[Tejun Heo]

Hello,

On Fri, Sep 28, 2012 at 06:04:01PM +0800, Lai Jiangshan wrote:
> On 09/27/2012 02:24 AM, Tejun Heo wrote:
> > On Thu, Sep 27, 2012 at 01:20:34AM +0800, Lai Jiangshan wrote:
> >> There is no reason to use WORKER_PREP, remove it from rescuer.
> >>
> >> And there is no reason to set it so early in alloc_worker(),
> >> move "worker->flags = WORKER_PREP" to start_worker().
> > 
> > Merge this with the patch to add RESCUER to NOT_RUNNING?
> 
> Don't merge. the change log is not good.
> I should list all the purposes of WORKER_PREP(I did it in my note),
> and then we can claim that we don't need to add WORKER_PREP show early.
> 
> These two patches do different things.

Hmmm?  You're replacing WORKER_PREP with RESCUER for NOT_RUNNING
marking.  I don't see why they need to be separate patches.  Just
explain what's going on in the commit message.

Thanks.

-- 
tejun

[PATCH 04/12] workqueue: simplify is_chained_work()

[Lai Jiangshan]

is_chained_work() is too complicated. we can simply found out
whether current task is worker by PF_WQ_WORKER or wq->rescuer.

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

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index e41c562..c718b94 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1182,34 +1182,22 @@ static void insert_work(struct cpu_workqueue_struct *cwq,
 
 /*
  * Test whether @work is being queued from another work executing on the
- * same workqueue.  This is rather expensive and should only be used from
- * cold paths.
+ * same workqueue.
  */
 static bool is_chained_work(struct workqueue_struct *wq)
 {
-	unsigned long flags;
-	unsigned int cpu;
+	struct worker *worker = NULL;
 
-	for_each_gcwq_cpu(cpu) {
-		struct global_cwq *gcwq = get_gcwq(cpu);
-		struct worker *worker;
-		struct hlist_node *pos;
-		int i;
+	if (wq->rescuer && current == wq->rescuer->task) /* rescuer_thread() */
+		worker = wq->rescuer;
+	else if (current->flags & PF_WQ_WORKER) /* worker_thread() */
+		worker = kthread_data(current);
 
-		spin_lock_irqsave(&gcwq->lock, flags);
-		for_each_busy_worker(worker, i, pos, gcwq) {
-			if (worker->task != current)
-				continue;
-			spin_unlock_irqrestore(&gcwq->lock, flags);
-			/*
-			 * I'm @worker, no locking necessary.  See if @work
-			 * is headed to the same workqueue.
-			 */
-			return worker->current_cwq->wq == wq;
-		}
-		spin_unlock_irqrestore(&gcwq->lock, flags);
-	}
-	return false;
+	/*
+	 * I'm @worker, no locking necessary.  See if @work
+	 * is headed to the same workqueue.
+	 */
+	return worker && worker->current_cwq->wq == wq;
 }
 
 static void __queue_work(unsigned int cpu, struct workqueue_struct *wq,
@@ -1231,7 +1219,7 @@ static void __queue_work(unsigned int cpu, struct workqueue_struct *wq,
 
 	debug_work_activate(work);
 
-	/* if dying, only works from the same workqueue are allowed */
+	/* if draining, only works from the same workqueue are allowed */
 	if (unlikely(wq->flags & WQ_DRAINING) &&
 	    WARN_ON_ONCE(!is_chained_work(wq)))
 		return;
-- 
1.7.7.6

Re: [PATCH 04/12] workqueue: simplify is_chained_work()

[Tejun Heo]

On Thu, Sep 27, 2012 at 01:20:35AM +0800, Lai Jiangshan wrote:
> is_chained_work() is too complicated. we can simply found out
> whether current task is worker by PF_WQ_WORKER or wq->rescuer.
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> ---
>  kernel/workqueue.c |   36 ++++++++++++------------------------
>  1 files changed, 12 insertions(+), 24 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index e41c562..c718b94 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -1182,34 +1182,22 @@ static void insert_work(struct cpu_workqueue_struct *cwq,
>  
>  /*
>   * Test whether @work is being queued from another work executing on the
> - * same workqueue.  This is rather expensive and should only be used from
> - * cold paths.
> + * same workqueue.
>   */
>  static bool is_chained_work(struct workqueue_struct *wq)
>  {
> -	unsigned long flags;
> -	unsigned int cpu;
> +	struct worker *worker = NULL;
>  
> -	for_each_gcwq_cpu(cpu) {
> -		struct global_cwq *gcwq = get_gcwq(cpu);
> -		struct worker *worker;
> -		struct hlist_node *pos;
> -		int i;
> +	if (wq->rescuer && current == wq->rescuer->task) /* rescuer_thread() */
> +		worker = wq->rescuer;
> +	else if (current->flags & PF_WQ_WORKER) /* worker_thread() */
> +		worker = kthread_data(current);
>  
> -		spin_lock_irqsave(&gcwq->lock, flags);
> -		for_each_busy_worker(worker, i, pos, gcwq) {
> -			if (worker->task != current)
> -				continue;
> -			spin_unlock_irqrestore(&gcwq->lock, flags);
> -			/*
> -			 * I'm @worker, no locking necessary.  See if @work
> -			 * is headed to the same workqueue.
> -			 */
> -			return worker->current_cwq->wq == wq;
> -		}
> -		spin_unlock_irqrestore(&gcwq->lock, flags);
> -	}
> -	return false;
> +	/*
> +	 * I'm @worker, no locking necessary.  See if @work
> +	 * is headed to the same workqueue.
> +	 */
> +	return worker && worker->current_cwq->wq == wq;

How about,

	if (wq->rescuer && current == wq->rescuer->task)
		worker = wq->rescuer;
	else if (current->flags & PF_WQ_WORKER)
		worker = kthread_data(current);
	else
		return NULL;

	return worker->curent_cwq->wq == wq;

Thanks.

-- 
tejun

Re: [PATCH 04/12] workqueue: simplify is_chained_work()

[Lai Jiangshan]

On 09/27/2012 02:28 AM, Tejun Heo wrote:
> On Thu, Sep 27, 2012 at 01:20:35AM +0800, Lai Jiangshan wrote:
>> is_chained_work() is too complicated. we can simply found out
>> whether current task is worker by PF_WQ_WORKER or wq->rescuer.
>>
>> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
>> ---
>>  kernel/workqueue.c |   36 ++++++++++++------------------------
>>  1 files changed, 12 insertions(+), 24 deletions(-)
>>
>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> index e41c562..c718b94 100644
>> --- a/kernel/workqueue.c
>> +++ b/kernel/workqueue.c
>> @@ -1182,34 +1182,22 @@ static void insert_work(struct cpu_workqueue_struct *cwq,
>>  
>>  /*
>>   * Test whether @work is being queued from another work executing on the
>> - * same workqueue.  This is rather expensive and should only be used from
>> - * cold paths.
>> + * same workqueue.
>>   */
>>  static bool is_chained_work(struct workqueue_struct *wq)
>>  {
>> -	unsigned long flags;
>> -	unsigned int cpu;
>> +	struct worker *worker = NULL;
>>  
>> -	for_each_gcwq_cpu(cpu) {
>> -		struct global_cwq *gcwq = get_gcwq(cpu);
>> -		struct worker *worker;
>> -		struct hlist_node *pos;
>> -		int i;
>> +	if (wq->rescuer && current == wq->rescuer->task) /* rescuer_thread() */
>> +		worker = wq->rescuer;
>> +	else if (current->flags & PF_WQ_WORKER) /* worker_thread() */
>> +		worker = kthread_data(current);
>>  
>> -		spin_lock_irqsave(&gcwq->lock, flags);
>> -		for_each_busy_worker(worker, i, pos, gcwq) {
>> -			if (worker->task != current)
>> -				continue;
>> -			spin_unlock_irqrestore(&gcwq->lock, flags);
>> -			/*
>> -			 * I'm @worker, no locking necessary.  See if @work
>> -			 * is headed to the same workqueue.
>> -			 */
>> -			return worker->current_cwq->wq == wq;
>> -		}
>> -		spin_unlock_irqrestore(&gcwq->lock, flags);
>> -	}
>> -	return false;
>> +	/*
>> +	 * I'm @worker, no locking necessary.  See if @work
>> +	 * is headed to the same workqueue.
>> +	 */
>> +	return worker && worker->current_cwq->wq == wq;

	if current is a worker and ...

> 
> How about,
> 
> 	if (wq->rescuer && current == wq->rescuer->task)
> 		worker = wq->rescuer;
> 	else if (current->flags & PF_WQ_WORKER)
> 		worker = kthread_data(current);
> 	else
> 		return NULL;
> 
> 	return worker->curent_cwq->wq == wq;
> 

Hi, Tejun

Your code is good, but I don't think I need to resend(and use your code).

Main reason: I think the readability of your is the same as mine,
and your add two lines.

Tiny reason: my code uses only one return. (I don't always keep one return,
but I try to keep it if it is still clean)

Is there any other reason to change it?

Thanks,
Lai.

Re: [PATCH 04/12] workqueue: simplify is_chained_work()

[Tejun Heo]

Hello,

On Fri, Sep 28, 2012 at 05:52:02PM +0800, Lai Jiangshan wrote:
> Main reason: I think the readability of your is the same as mine,
> and your add two lines.
> 
> Tiny reason: my code uses only one return. (I don't always keep one return,
> but I try to keep it if it is still clean)
> 
> Is there any other reason to change it?

I don't like that the same condition is tested twice but technical
advantages on issues like this are usually too small to be used as the
sole basis to choose one over another and it usually comes down to
preferences.  For little things without clear technical advantage,
following maintainer's taste tends to lead to higher consistency.  So,
yeah, I'd appreciate if you change it on the next posting or will
probably just change it while applying it.

Thanks.

-- 
tejun

[PATCH 05/12] workqueue: don't wake up other workers in rescuer

[Lai Jiangshan]

rescuer is NOT_RUNNING, so there is no sense when it wakes up other workers,
if there are available normal workers, they are already woken up when needed.

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

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index c718b94..6c339bf 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2438,14 +2438,6 @@ repeat:
 
 		process_scheduled_works(rescuer);
 
-		/*
-		 * Leave this gcwq.  If keep_working() is %true, notify a
-		 * regular worker; otherwise, we end up with 0 concurrency
-		 * and stalling the execution.
-		 */
-		if (keep_working(pool))
-			wake_up_worker(pool);
-
 		spin_unlock_irq(&gcwq->lock);
 	}
 
-- 
1.7.7.6

Re: [PATCH 05/12] workqueue: don't wake up other workers in rescuer

[Tejun Heo]

(cc'ing Ray Jui)

On Thu, Sep 27, 2012 at 01:20:36AM +0800, Lai Jiangshan wrote:
> rescuer is NOT_RUNNING, so there is no sense when it wakes up other workers,
> if there are available normal workers, they are already woken up when needed.
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> ---
>  kernel/workqueue.c |    8 --------
>  1 files changed, 0 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index c718b94..6c339bf 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -2438,14 +2438,6 @@ repeat:
>  
>  		process_scheduled_works(rescuer);
>  
> -		/*
> -		 * Leave this gcwq.  If keep_working() is %true, notify a
> -		 * regular worker; otherwise, we end up with 0 concurrency
> -		 * and stalling the execution.
> -		 */
> -		if (keep_working(pool))
> -			wake_up_worker(pool);
> -

This was added by 7576958a9d5a4a6 ("workqueue: wake up a worker when a
rescuer is leaving a gcwq") to fix a bug reported by Ray Jui.

  http://thread.gmane.org/gmane.linux.kernel/1098131

I'm fairly sure it was a valid bug report.  I don't think the
depletion comes from concurrency management.  It's just the lack of
chaining which could lead to stall.  What am I missing here?

Thanks.

-- 
tejun

Re: [PATCH 05/12] workqueue: don't wake up other workers in rescuer

[Lai Jiangshan]

On 09/27/2012 02:34 AM, Tejun Heo wrote:
> (cc'ing Ray Jui)
> 
> On Thu, Sep 27, 2012 at 01:20:36AM +0800, Lai Jiangshan wrote:
>> rescuer is NOT_RUNNING, so there is no sense when it wakes up other workers,
>> if there are available normal workers, they are already woken up when needed.
>>
>> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
>> ---
>>  kernel/workqueue.c |    8 --------
>>  1 files changed, 0 insertions(+), 8 deletions(-)
>>
>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> index c718b94..6c339bf 100644
>> --- a/kernel/workqueue.c
>> +++ b/kernel/workqueue.c
>> @@ -2438,14 +2438,6 @@ repeat:
>>  
>>  		process_scheduled_works(rescuer);
>>  
>> -		/*
>> -		 * Leave this gcwq.  If keep_working() is %true, notify a
>> -		 * regular worker; otherwise, we end up with 0 concurrency
>> -		 * and stalling the execution.
>> -		 */
>> -		if (keep_working(pool))
>> -			wake_up_worker(pool);
>> -
> 
> This was added by 7576958a9d5a4a6 ("workqueue: wake up a worker when a
> rescuer is leaving a gcwq") to fix a bug reported by Ray Jui.
> 
>   http://thread.gmane.org/gmane.linux.kernel/1098131
> 
> I'm fairly sure it was a valid bug report.  I don't think the
> depletion comes from concurrency management.  It's just the lack of
> chaining which could lead to stall.  What am I missing here?
> 

"http://thread.gmane.org/gmane.linux.kernel/1098131" does not find out the root cause.
I find out the root cause with hard searching from the code, I will describe it later.

Thanks,
Lai

[PATCH 06/12] workqueue: destroy_worker() can only destory idle worker not just created worker

[Lai Jiangshan]

After we reimpletment hotplug code, we will not destroy just newly
created worker, all created worker will enter idle soon.
And destroy_worker() is not used to destroy just newly created worker,
it always destory idle worker, so we need to fix destroy_worker()
and update the comments.

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

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 6c339bf..fe3b1d3 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1756,8 +1756,7 @@ static struct worker *alloc_worker(void)
  * @pool: pool the new worker will belong to
  *
  * Create a new worker which is bound to @pool.  The returned worker
- * can be started by calling start_worker() or destroyed using
- * destroy_worker().
+ * can be started by calling start_worker();
  *
  * CONTEXT:
  * Might sleep.  Does GFP_KERNEL allocations.
@@ -1847,7 +1846,7 @@ static void start_worker(struct worker *worker)
 }
 
 /**
- * destroy_worker - destroy a workqueue worker
+ * destroy_worker - destroy a idle workqueue worker
  * @worker: worker to be destroyed
  *
  * Destroy @worker and adjust @gcwq stats accordingly.
@@ -1864,11 +1863,12 @@ static void destroy_worker(struct worker *worker)
 	/* sanity check frenzy */
 	BUG_ON(worker->current_work);
 	BUG_ON(!list_empty(&worker->scheduled));
+	BUG_ON(!(worker->flags & WORKER_STARTED));
+	BUG_ON(!(worker->flags & WORKER_IDLE));
+	BUG_ON(list_empty(&worker->entry));
 
-	if (worker->flags & WORKER_STARTED)
-		pool->nr_workers--;
-	if (worker->flags & WORKER_IDLE)
-		pool->nr_idle--;
+	pool->nr_workers--;
+	pool->nr_idle--;
 
 	list_del_init(&worker->entry);
 	worker->flags |= WORKER_DIE;
-- 
1.7.7.6

Re: [PATCH 06/12] workqueue: destroy_worker() can only destory idle worker not just created worker

[Tejun Heo]

On Thu, Sep 27, 2012 at 01:20:37AM +0800, Lai Jiangshan wrote:
> After we reimpletment hotplug code, we will not destroy just newly
> created worker, all created worker will enter idle soon.
> And destroy_worker() is not used to destroy just newly created worker,
> it always destory idle worker, so we need to fix destroy_worker()
> and update the comments.
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> ---
>  kernel/workqueue.c |   14 +++++++-------
>  1 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 6c339bf..fe3b1d3 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -1756,8 +1756,7 @@ static struct worker *alloc_worker(void)
>   * @pool: pool the new worker will belong to
>   *
>   * Create a new worker which is bound to @pool.  The returned worker
> - * can be started by calling start_worker() or destroyed using
> - * destroy_worker().
> + * can be started by calling start_worker();
>   *
>   * CONTEXT:
>   * Might sleep.  Does GFP_KERNEL allocations.
> @@ -1847,7 +1846,7 @@ static void start_worker(struct worker *worker)
>  }
>  
>  /**
> - * destroy_worker - destroy a workqueue worker
> + * destroy_worker - destroy a idle workqueue worker
>   * @worker: worker to be destroyed
>   *
>   * Destroy @worker and adjust @gcwq stats accordingly.
> @@ -1864,11 +1863,12 @@ static void destroy_worker(struct worker *worker)
>  	/* sanity check frenzy */
>  	BUG_ON(worker->current_work);
>  	BUG_ON(!list_empty(&worker->scheduled));
> +	BUG_ON(!(worker->flags & WORKER_STARTED));
> +	BUG_ON(!(worker->flags & WORKER_IDLE));
> +	BUG_ON(list_empty(&worker->entry));

Let's go for WARN_ON() at least for the new ones.  Or let's convert
all BUG()s to WARN_ON[_ONCE]() beforehand.

Thanks.

-- 
tejun

[PATCH 07/12] workqueue: remove WORKER_STARTED

[Lai Jiangshan]

All newly created worker will enter idle soon,
WORKER_STARTED is not used any more, remove it.

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

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index fe3b1d3..d37f446 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -69,7 +69,6 @@ enum {
 	POOL_MANAGING_WORKERS   = 1 << 1,       /* managing workers */
 
 	/* worker flags */
-	WORKER_STARTED		= 1 << 0,	/* started */
 	WORKER_DIE		= 1 << 1,	/* die die die */
 	WORKER_IDLE		= 1 << 2,	/* is idle */
 	WORKER_PREP		= 1 << 3,	/* preparing to run works */
@@ -1838,7 +1837,6 @@ fail:
  */
 static void start_worker(struct worker *worker)
 {
-	worker->flags |= WORKER_STARTED;
 	worker->flags |= WORKER_PREP;
 	worker->pool->nr_workers++;
 	worker_enter_idle(worker);
@@ -1863,7 +1861,6 @@ static void destroy_worker(struct worker *worker)
 	/* sanity check frenzy */
 	BUG_ON(worker->current_work);
 	BUG_ON(!list_empty(&worker->scheduled));
-	BUG_ON(!(worker->flags & WORKER_STARTED));
 	BUG_ON(!(worker->flags & WORKER_IDLE));
 	BUG_ON(list_empty(&worker->entry));
 
-- 
1.7.7.6

Re: [PATCH 07/12] workqueue: remove WORKER_STARTED

[Tejun Heo]

On Thu, Sep 27, 2012 at 01:20:38AM +0800, Lai Jiangshan wrote:
> All newly created worker will enter idle soon,
> WORKER_STARTED is not used any more, remove it.

Please merge this with the previous patch.

-- 
tejun

Re: [PATCH 07/12] workqueue: remove WORKER_STARTED

[Lai Jiangshan]

On 09/27/2012 02:36 AM, Tejun Heo wrote:
> On Thu, Sep 27, 2012 at 01:20:38AM +0800, Lai Jiangshan wrote:
>> All newly created worker will enter idle soon,
>> WORKER_STARTED is not used any more, remove it.
> 
> Please merge this with the previous patch.
> 

OK, I will do it.

Thanks,
Lai

[PATCH 08/12] workqueue: fix comments of insert_work()

[Lai Jiangshan]

This comment is so important to understand the CMWQ, fix it and make
new reviewer who read workqueue.c at the first time.

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 d37f446..89fd1b2 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1169,7 +1169,7 @@ static void insert_work(struct cpu_workqueue_struct *cwq,
 	list_add_tail(&work->entry, head);
 
 	/*
-	 * Ensure either worker_sched_deactivated() sees the above
+	 * Ensure either wq_worker_sleeping() sees the above
 	 * list_add_tail() or we see zero nr_running to avoid workers
 	 * lying around lazily while there are works to be processed.
 	 */
-- 
1.7.7.6

[PATCH 09/12] workqueue: declare system_highpri_wq

[Lai Jiangshan]

system_highpri_wq is missed in workqueue.h, add it back.
also add a short comment for it.

CC: Joonsoo Kim <js1304@gmail.com>
Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 include/linux/workqueue.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 2b58905..68b1d2a 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -294,6 +294,9 @@ enum {
  * short queue flush time.  Don't queue works which can run for too
  * long.
  *
+ * system_highpri_wq is similar to system_wq but services for urgent works
+ * and works will be processed in high priority workers.
+ *
  * system_long_wq is similar to system_wq but may host long running
  * works.  Queue flushing might take relatively long.
  *
@@ -306,6 +309,7 @@ enum {
  * freezable.
  */
 extern struct workqueue_struct *system_wq;
+extern struct workqueue_struct *system_highpri_wq;
 extern struct workqueue_struct *system_long_wq;
 extern struct workqueue_struct *system_unbound_wq;
 extern struct workqueue_struct *system_freezable_wq;
-- 
1.7.7.6

[PATCH 10/12] cpu-hotplug.txt: fix comments of work_on_cpu()

[Lai Jiangshan]

This is a tiny fix of the comments of work_on_cpu() which is changed
back to use workqueue and it can be run at some time.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 Documentation/cpu-hotplug.txt |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Documentation/cpu-hotplug.txt b/Documentation/cpu-hotplug.txt
index 66ef8f3..d19c5fd 100644
--- a/Documentation/cpu-hotplug.txt
+++ b/Documentation/cpu-hotplug.txt
@@ -317,7 +317,7 @@ Q: I need to ensure that a particular cpu is not removed when there is some
    work specific to this cpu is in progress.
 A: There are two ways.  If your code can be run in interrupt context, use
    smp_call_function_single(), otherwise use work_on_cpu().  Note that
-   work_on_cpu() is slow, and can fail due to out of memory:
+   work_on_cpu() is slow:
 
 	int my_func_on_cpu(int cpu)
 	{
-- 
1.7.7.6

[RFC PATCH 11/12] workqueue: add WQ_CPU_INTENSIVE to system_long_wq

[Lai Jiangshan]

works in system_long_wq will be running long.
add WQ_CPU_INTENSIVE to system_long_wq to avoid these kinds of works occupy
the running wokers which delay the normal works.

if system_long_wq is designed for only sleep-long works, not running-long works,
this patch makes no sense.

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 89fd1b2..ccb1d60 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3850,7 +3850,7 @@ static int __init init_workqueues(void)
 
 	system_wq = alloc_workqueue("events", 0, 0);
 	system_highpri_wq = alloc_workqueue("events_highpri", WQ_HIGHPRI, 0);
-	system_long_wq = alloc_workqueue("events_long", 0, 0);
+	system_long_wq = alloc_workqueue("events_long", WQ_CPU_INTENSIVE, 0);
 	system_unbound_wq = alloc_workqueue("events_unbound", WQ_UNBOUND,
 					    WQ_UNBOUND_MAX_ACTIVE);
 	system_freezable_wq = alloc_workqueue("events_freezable",
-- 
1.7.7.6

Re: [RFC PATCH 11/12] workqueue: add WQ_CPU_INTENSIVE to system_long_wq

[Tejun Heo]

On Thu, Sep 27, 2012 at 01:20:42AM +0800, Lai Jiangshan wrote:
> works in system_long_wq will be running long.
> add WQ_CPU_INTENSIVE to system_long_wq to avoid these kinds of works occupy
> the running wokers which delay the normal works.
> 
> if system_long_wq is designed for only sleep-long works, not running-long works,
> this patch makes no sense.

There "long" doesn't mean it's gonna consume a lot of CPU cycles, so
it should contribute to concurrency management.  The plan is to remove
system_long_wq once we got rid of flushing of system_wq via
flush_scheduled_work() and directly through flush_workqueue().  I
think we're pretty close, so let's not add more usage of it.

Thanks.

-- 
tejun

Re: [RFC PATCH 11/12] workqueue: add WQ_CPU_INTENSIVE to system_long_wq

[Lai Jiangshan]

Hi, Tejun

On 09/27/2012 02:38 AM, Tejun Heo wrote:
> On Thu, Sep 27, 2012 at 01:20:42AM +0800, Lai Jiangshan wrote:
>> works in system_long_wq will be running long.
>> add WQ_CPU_INTENSIVE to system_long_wq to avoid these kinds of works occupy
>> the running wokers which delay the normal works.
>>
>> if system_long_wq is designed for only sleep-long works, not running-long works,
>> this patch makes no sense.
> 
> There "long" doesn't mean it's gonna consume a lot of CPU cycles, so
> it should contribute to concurrency management.  The plan is to remove
> system_long_wq once we got rid of flushing of system_wq via
> flush_scheduled_work() and directly through flush_workqueue().  I
> think we're pretty close, so let's not add more usage of it.
> 

OK.

But does we need a stand-alone workqueue for work_on_cpu() as it is original
introduced? (2d3854a3)

Thanks,
Lai

Re: [RFC PATCH 11/12] workqueue: add WQ_CPU_INTENSIVE to system_long_wq

[Tejun Heo]

Hello, Lai.

On Fri, Sep 28, 2012 at 04:06:48PM +0800, Lai Jiangshan wrote:
> But does we need a stand-alone workqueue for work_on_cpu() as it is original
> introduced? (2d3854a3)

Given how work_on_cpu() is used currently, I don't think we need that.
What we need to do is removing the remaining flush_scheduled_work()
users and trigger WARN if somebody tries to flush one of the system
workqueues.

Thanks.

-- 
tejun

[PATCH 12/12] workqueue: avoid work_on_cpu() to interfere system_wq

[Lai Jiangshan]

We can't expect how long the work of work_on_cpu() will run.
So move it to system_long_wq to avoid it interfere system_wq.

Note:
The initial implement(2d3854a3) of work_on_cpu() uses its own workqueue.

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 ccb1d60..c14d94c 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3638,7 +3638,7 @@ long work_on_cpu(unsigned int cpu, long (*fn)(void *), void *arg)
 	struct work_for_cpu wfc = { .fn = fn, .arg = arg };
 
 	INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
-	schedule_work_on(cpu, &wfc.work);
+	queue_work_on(cpu, system_long_wq, &wfc.work);
 	flush_work(&wfc.work);
 	return wfc.ret;
 }
-- 
1.7.7.6