Re: [PATCH 04/12] workqueue: simplify is_chained_work()
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 04/12] workqueue: simplify is_chained_work()
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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 04/12] workqueue: simplify is_chained_work()
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
>> ---
>> 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(>lock, flags);
>> -for_each_busy_worker(worker, i, pos, gcwq) {
>> -if (worker->task != current)
>> -continue;
>> -spin_unlock_irqrestore(>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(>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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 04/12] workqueue: simplify is_chained_work()
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 la...@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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 04/12] workqueue: simplify is_chained_work()
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
> ---
> 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(>lock, flags);
> - for_each_busy_worker(worker, i, pos, gcwq) {
> - if (worker->task != current)
> - continue;
> - spin_unlock_irqrestore(>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(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 04/12] workqueue: simplify is_chained_work()
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
---
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(>lock, flags);
- for_each_busy_worker(worker, i, pos, gcwq) {
- if (worker->task != current)
- continue;
- spin_unlock_irqrestore(>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(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 04/12] workqueue: simplify is_chained_work()
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 la...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 04/12] workqueue: simplify is_chained_work()
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 la...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
