[PATCH 1/2 wq/for-3.9] workqueue: fix is_chained_work() regression

[PATCH 1/2 wq/for-3.9] workqueue: fix is_chained_work() regression

[Tejun Heo]

c9e7cf273f ("workqueue: move busy_hash from global_cwq to
worker_pool") incorrectly converted is_chained_work() to use
get_gcwq() inside for_each_gcwq_cpu() while removing get_gcwq().

As cwq might not exist for all possible workqueue CPUs, @cwq can be
NULL and the following cwq deferences can lead to oops.

Fix it by using for_each_cwq_cpu() instead, which is the better one to
use anyway as we only need to check pools that the wq is associated
with.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 kernel/workqueue.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1167,7 +1167,7 @@ static bool is_chained_work(struct workq
 	unsigned long flags;
 	unsigned int cpu;
 
-	for_each_wq_cpu(cpu) {
+	for_each_cwq_cpu(cpu, wq) {
 		struct cpu_workqueue_struct *cwq = get_cwq(cpu, wq);
 		struct worker_pool *pool = cwq->pool;
 		struct worker *worker;

[PATCH 2/2 wq/for-3.9] workqueue: reimplement is_chained_work() using current_wq_worker()

[Tejun Heo]

is_chained_work() was added before current_wq_worker() and implemented
its own ham-fisted way of finding out whether %current is a workqueue
worker - it iterates through all possible workers.

Drop the custom implementation and reimplement using
current_wq_worker().

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 kernel/workqueue.c |   33 ++++++++-------------------------
 1 file changed, 8 insertions(+), 25 deletions(-)

--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1159,35 +1159,18 @@ static void insert_work(struct cpu_workq
 
 /*
  * 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;
 
-	for_each_cwq_cpu(cpu, wq) {
-		struct cpu_workqueue_struct *cwq = get_cwq(cpu, wq);
-		struct worker_pool *pool = cwq->pool;
-		struct worker *worker;
-		struct hlist_node *pos;
-		int i;
-
-		spin_lock_irqsave(&pool->lock, flags);
-		for_each_busy_worker(worker, i, pos, pool) {
-			if (worker->task != current)
-				continue;
-			spin_unlock_irqrestore(&pool->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(&pool->lock, flags);
-	}
-	return false;
+	worker = current_wq_worker();
+	/*
+	 * Return %true iff I'm a worker execuing a work item on @wq.  If
+	 * I'm @worker, it's safe to dereference it without locking.
+	 */
+	return worker && worker->current_cwq->wq == wq;
 }
 
 static void __queue_work(unsigned int cpu, struct workqueue_struct *wq,

Re: [PATCH 2/2 wq/for-3.9] workqueue: reimplement is_chained_work() using current_wq_worker()

[Lai Jiangshan]

Hi, tj

Thank you for adding this one.

Would you deffer "workqueue: rename cpu_workqueue to pool_workqueue" a 
little? I don't want to rebase my almost-ready work again(not a good 
reason... but please...)

I will answer your other emails soon and sent the patches.

Thanks,
Lai

On 14/02/13 11:27, Tejun Heo wrote:
> is_chained_work() was added before current_wq_worker() and implemented
> its own ham-fisted way of finding out whether %current is a workqueue
> worker - it iterates through all possible workers.
>
> Drop the custom implementation and reimplement using
> current_wq_worker().
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>   kernel/workqueue.c |   33 ++++++++-------------------------
>   1 file changed, 8 insertions(+), 25 deletions(-)
>
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -1159,35 +1159,18 @@ static void insert_work(struct cpu_workq
>
>   /*
>    * 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;
>
> -	for_each_cwq_cpu(cpu, wq) {
> -		struct cpu_workqueue_struct *cwq = get_cwq(cpu, wq);
> -		struct worker_pool *pool = cwq->pool;
> -		struct worker *worker;
> -		struct hlist_node *pos;
> -		int i;
> -
> -		spin_lock_irqsave(&pool->lock, flags);
> -		for_each_busy_worker(worker, i, pos, pool) {
> -			if (worker->task != current)
> -				continue;
> -			spin_unlock_irqrestore(&pool->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(&pool->lock, flags);
> -	}
> -	return false;
> +	worker = current_wq_worker();
> +	/*
> +	 * Return %true iff I'm a worker execuing a work item on @wq.  If
> +	 * I'm @worker, it's safe to dereference it without locking.
> +	 */
> +	return worker && worker->current_cwq->wq == wq;
>   }
>
>   static void __queue_work(unsigned int cpu, struct workqueue_struct *wq,
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 2/2 wq/for-3.9] workqueue: reimplement is_chained_work() using current_wq_worker()

[Tejun Heo]

Hey, Lai.

On Sun, Feb 17, 2013 at 01:06:40AM +0800, Lai Jiangshan wrote:
> Hi, tj
> 
> Thank you for adding this one.
> 
> Would you deffer "workqueue: rename cpu_workqueue to pool_workqueue"
> a little? I don't want to rebase my almost-ready work again(not a
> good reason... but please...)

Sorry about the late reply.  Was offline over the weekend.  I see that
you already updated your patches on top, so I suppose this doesn't
matter anymore?

Thanks!

-- 
tejun