Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Linux: Kernel

[PATCH 17/21] workqueue: simple reimplementation of SINGLE_THREAD workqueue

 

 

Linux kernel RSS feed   Index | Next | Previous | View Threaded


tj at kernel

Nov 16, 2009, 9:15 AM

Post #1 of 10 (1284 views)
Permalink
[PATCH 17/21] workqueue: simple reimplementation of SINGLE_THREAD workqueue

SINGLE_THREAD workqueues are used to reduce the number of worker
threads and ease synchronization. The first reason will be irrelevant
with concurrency managed workqueue implementation. Simplify
SINGLE_THREAD implementation by creating the workqueues the same but
making the worker grab mutex before actually executing works on the
workqueue. In the long run, most SINGLE_THREAD workqueues will be
replaced with generic ones.

Signed-off-by: Tejun Heo <tj [at] kernel>
---
kernel/workqueue.c | 151 ++++++++++++++++++----------------------------------
1 files changed, 52 insertions(+), 99 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 5392939..82b03a1 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -33,6 +33,7 @@
#include <linux/kallsyms.h>
#include <linux/debug_locks.h>
#include <linux/lockdep.h>
+#include <linux/mutex.h>

/*
* Structure fields follow one of the following exclusion rules.
@@ -71,6 +72,7 @@ struct workqueue_struct {
struct cpu_workqueue_struct *cpu_wq; /* I: cwq's */
struct list_head list; /* W: list of all workqueues */
const char *name; /* I: workqueue name */
+ struct mutex single_thread_mutex; /* for SINGLE_THREAD wq */
#ifdef CONFIG_LOCKDEP
struct lockdep_map lockdep_map;
#endif
@@ -190,34 +192,9 @@ static inline void debug_work_deactivate(struct work_struct *work) { }
static DEFINE_SPINLOCK(workqueue_lock);
static LIST_HEAD(workqueues);

-static int singlethread_cpu __read_mostly;
-static const struct cpumask *cpu_singlethread_map __read_mostly;
-/*
- * _cpu_down() first removes CPU from cpu_online_map, then CPU_DEAD
- * flushes cwq->worklist. This means that flush_workqueue/wait_on_work
- * which comes in between can't use for_each_online_cpu(). We could
- * use cpu_possible_map, the cpumask below is more a documentation
- * than optimization.
- */
-static cpumask_var_t cpu_populated_map __read_mostly;
-
-/* If it's single threaded, it isn't in the list of workqueues. */
-static inline bool is_wq_single_threaded(struct workqueue_struct *wq)
-{
- return wq->flags & WQ_SINGLE_THREAD;
-}
-
-static const struct cpumask *wq_cpu_map(struct workqueue_struct *wq)
-{
- return is_wq_single_threaded(wq)
- ? cpu_singlethread_map : cpu_populated_map;
-}
-
static struct cpu_workqueue_struct *get_cwq(unsigned int cpu,
struct workqueue_struct *wq)
{
- if (unlikely(is_wq_single_threaded(wq)))
- cpu = singlethread_cpu;
return per_cpu_ptr(wq->cpu_wq, cpu);
}

@@ -410,6 +387,8 @@ EXPORT_SYMBOL_GPL(queue_delayed_work_on);
static void process_one_work(struct cpu_workqueue_struct *cwq,
struct work_struct *work)
{
+ struct workqueue_struct *wq = cwq->wq;
+ bool single_thread = wq->flags & WQ_SINGLE_THREAD;
work_func_t f = work->func;
#ifdef CONFIG_LOCKDEP
/*
@@ -430,11 +409,18 @@ static void process_one_work(struct cpu_workqueue_struct *cwq,

BUG_ON(get_wq_data(work) != cwq);
work_clear_pending(work);
- lock_map_acquire(&cwq->wq->lockdep_map);
+ lock_map_acquire(&wq->lockdep_map);
lock_map_acquire(&lockdep_map);
- f(work);
+
+ if (unlikely(single_thread)) {
+ mutex_lock(&wq->single_thread_mutex);
+ f(work);
+ mutex_unlock(&wq->single_thread_mutex);
+ } else
+ f(work);
+
lock_map_release(&lockdep_map);
- lock_map_release(&cwq->wq->lockdep_map);
+ lock_map_release(&wq->lockdep_map);

if (unlikely(in_atomic() || lockdep_depth(current) > 0)) {
printk(KERN_ERR "BUG: workqueue leaked lock or atomic: "
@@ -569,14 +555,13 @@ static int flush_cpu_workqueue(struct cpu_workqueue_struct *cwq)
*/
void flush_workqueue(struct workqueue_struct *wq)
{
- const struct cpumask *cpu_map = wq_cpu_map(wq);
int cpu;

might_sleep();
lock_map_acquire(&wq->lockdep_map);
lock_map_release(&wq->lockdep_map);
- for_each_cpu(cpu, cpu_map)
- flush_cpu_workqueue(per_cpu_ptr(wq->cpu_wq, cpu));
+ for_each_possible_cpu(cpu)
+ flush_cpu_workqueue(get_cwq(cpu, wq));
}
EXPORT_SYMBOL_GPL(flush_workqueue);

@@ -694,7 +679,6 @@ static void wait_on_work(struct work_struct *work)
{
struct cpu_workqueue_struct *cwq;
struct workqueue_struct *wq;
- const struct cpumask *cpu_map;
int cpu;

might_sleep();
@@ -707,9 +691,8 @@ static void wait_on_work(struct work_struct *work)
return;

wq = cwq->wq;
- cpu_map = wq_cpu_map(wq);

- for_each_cpu(cpu, cpu_map)
+ for_each_possible_cpu(cpu)
wait_on_cpu_work(get_cwq(cpu, wq), work);
}

@@ -942,7 +925,7 @@ int current_is_keventd(void)

BUG_ON(!keventd_wq);

- cwq = per_cpu_ptr(keventd_wq->cpu_wq, cpu);
+ cwq = get_cwq(cpu, keventd_wq);
if (current == cwq->thread)
ret = 1;

@@ -950,26 +933,12 @@ int current_is_keventd(void)

}

-static struct cpu_workqueue_struct *
-init_cpu_workqueue(struct workqueue_struct *wq, int cpu)
-{
- struct cpu_workqueue_struct *cwq = per_cpu_ptr(wq->cpu_wq, cpu);
-
- cwq->wq = wq;
- spin_lock_init(&cwq->lock);
- INIT_LIST_HEAD(&cwq->worklist);
- init_waitqueue_head(&cwq->more_work);
-
- return cwq;
-}
-
static int create_workqueue_thread(struct cpu_workqueue_struct *cwq, int cpu)
{
struct workqueue_struct *wq = cwq->wq;
- const char *fmt = is_wq_single_threaded(wq) ? "%s" : "%s/%d";
struct task_struct *p;

- p = kthread_create(worker_thread, cwq, fmt, wq->name, cpu);
+ p = kthread_create(worker_thread, cwq, "%s/%d", wq->name, cpu);
/*
* Nobody can add the work_struct to this cwq,
* if (caller is __create_workqueue)
@@ -1002,7 +971,6 @@ struct workqueue_struct *__create_workqueue_key(const char *name,
const char *lock_name)
{
struct workqueue_struct *wq;
- struct cpu_workqueue_struct *cwq;
int err = 0, cpu;

wq = kzalloc(sizeof(*wq), GFP_KERNEL);
@@ -1015,39 +983,40 @@ struct workqueue_struct *__create_workqueue_key(const char *name,

wq->flags = flags;
wq->name = name;
+ mutex_init(&wq->single_thread_mutex);
lockdep_init_map(&wq->lockdep_map, lock_name, key, 0);
INIT_LIST_HEAD(&wq->list);

- if (flags & WQ_SINGLE_THREAD) {
- cwq = init_cpu_workqueue(wq, singlethread_cpu);
- err = create_workqueue_thread(cwq, singlethread_cpu);
- start_workqueue_thread(cwq, -1);
- } else {
- cpu_maps_update_begin();
- /*
- * We must place this wq on list even if the code below fails.
- * cpu_down(cpu) can remove cpu from cpu_populated_map before
- * destroy_workqueue() takes the lock, in that case we leak
- * cwq[cpu]->thread.
- */
- spin_lock(&workqueue_lock);
- list_add(&wq->list, &workqueues);
- spin_unlock(&workqueue_lock);
- /*
- * We must initialize cwqs for each possible cpu even if we
- * are going to call destroy_workqueue() finally. Otherwise
- * cpu_up() can hit the uninitialized cwq once we drop the
- * lock.
- */
- for_each_possible_cpu(cpu) {
- cwq = init_cpu_workqueue(wq, cpu);
- if (err || !cpu_online(cpu))
- continue;
- err = create_workqueue_thread(cwq, cpu);
- start_workqueue_thread(cwq, cpu);
- }
- cpu_maps_update_done();
+ cpu_maps_update_begin();
+ /*
+ * We must place this wq on list even if the code below fails.
+ * cpu_down(cpu) can remove cpu from cpu_populated_map before
+ * destroy_workqueue() takes the lock, in that case we leak
+ * cwq[cpu]->thread.
+ */
+ spin_lock(&workqueue_lock);
+ list_add(&wq->list, &workqueues);
+ spin_unlock(&workqueue_lock);
+ /*
+ * We must initialize cwqs for each possible cpu even if we
+ * are going to call destroy_workqueue() finally. Otherwise
+ * cpu_up() can hit the uninitialized cwq once we drop the
+ * lock.
+ */
+ for_each_possible_cpu(cpu) {
+ struct cpu_workqueue_struct *cwq = get_cwq(cpu, wq);
+
+ cwq->wq = wq;
+ spin_lock_init(&cwq->lock);
+ INIT_LIST_HEAD(&cwq->worklist);
+ init_waitqueue_head(&cwq->more_work);
+
+ if (err || !cpu_online(cpu))
+ continue;
+ err = create_workqueue_thread(cwq, cpu);
+ start_workqueue_thread(cwq, cpu);
}
+ cpu_maps_update_done();

if (err) {
destroy_workqueue(wq);
@@ -1098,7 +1067,6 @@ static void cleanup_workqueue_thread(struct cpu_workqueue_struct *cwq)
*/
void destroy_workqueue(struct workqueue_struct *wq)
{
- const struct cpumask *cpu_map = wq_cpu_map(wq);
int cpu;

cpu_maps_update_begin();
@@ -1106,8 +1074,8 @@ void destroy_workqueue(struct workqueue_struct *wq)
list_del(&wq->list);
spin_unlock(&workqueue_lock);

- for_each_cpu(cpu, cpu_map)
- cleanup_workqueue_thread(per_cpu_ptr(wq->cpu_wq, cpu));
+ for_each_possible_cpu(cpu)
+ cleanup_workqueue_thread(get_cwq(cpu, wq));
cpu_maps_update_done();

free_percpu(wq->cpu_wq);
@@ -1126,13 +1094,9 @@ static int __devinit workqueue_cpu_callback(struct notifier_block *nfb,

action &= ~CPU_TASKS_FROZEN;

- switch (action) {
- case CPU_UP_PREPARE:
- cpumask_set_cpu(cpu, cpu_populated_map);
- }
undo:
list_for_each_entry(wq, &workqueues, list) {
- cwq = per_cpu_ptr(wq->cpu_wq, cpu);
+ cwq = get_cwq(cpu, wq);

switch (action) {
case CPU_UP_PREPARE:
@@ -1156,12 +1120,6 @@ undo:
}
}

- switch (action) {
- case CPU_UP_CANCELED:
- case CPU_POST_DEAD:
- cpumask_clear_cpu(cpu, cpu_populated_map);
- }
-
return ret;
}

@@ -1223,11 +1181,6 @@ void __init init_workqueues(void)
BUILD_BUG_ON(__alignof__(struct cpu_workqueue_struct) <
__alignof__(unsigned long long));

- alloc_cpumask_var(&cpu_populated_map, GFP_KERNEL);
-
- cpumask_copy(cpu_populated_map, cpu_online_mask);
- singlethread_cpu = cpumask_first(cpu_possible_mask);
- cpu_singlethread_map = cpumask_of(singlethread_cpu);
hotcpu_notifier(workqueue_cpu_callback, 0);
keventd_wq = create_workqueue("events");
BUG_ON(!keventd_wq);
--
1.6.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


awalls at radix

Nov 16, 2009, 4:47 PM

Post #2 of 10 (1244 views)
Permalink
Re: [PATCH 17/21] workqueue: simple reimplementation of SINGLE_THREAD workqueue [In reply to]

On Tue, 2009-11-17 at 02:15 +0900, Tejun Heo wrote:
> SINGLE_THREAD workqueues are used to reduce the number of worker
> threads and ease synchronization. The first reason will be irrelevant
> with concurrency managed workqueue implementation. Simplify
> SINGLE_THREAD implementation by creating the workqueues the same but
> making the worker grab mutex before actually executing works on the
> workqueue. In the long run, most SINGLE_THREAD workqueues will be
> replaced with generic ones.
>
> Signed-off-by: Tejun Heo <tj [at] kernel>
>
> ---
> kernel/workqueue.c | 151 ++++++++++++++++++----------------------------------
> 1 files changed, 52 insertions(+), 99 deletions(-)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 5392939..82b03a1 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -33,6 +33,7 @@
> #include <linux/kallsyms.h>
> #include <linux/debug_locks.h>
> #include <linux/lockdep.h>
> +#include <linux/mutex.h>
>
> /*
> * Structure fields follow one of the following exclusion rules.
> @@ -71,6 +72,7 @@ struct workqueue_struct {
> struct cpu_workqueue_struct *cpu_wq; /* I: cwq's */
> struct list_head list; /* W: list of all workqueues */
> const char *name; /* I: workqueue name */
> + struct mutex single_thread_mutex; /* for SINGLE_THREAD wq */
> #ifdef CONFIG_LOCKDEP
> struct lockdep_map lockdep_map;
> #endif


> @@ -410,6 +387,8 @@ EXPORT_SYMBOL_GPL(queue_delayed_work_on);
> static void process_one_work(struct cpu_workqueue_struct *cwq,
> struct work_struct *work)
> {
> + struct workqueue_struct *wq = cwq->wq;
> + bool single_thread = wq->flags & WQ_SINGLE_THREAD;
> work_func_t f = work->func;
> #ifdef CONFIG_LOCKDEP
> /*
> @@ -430,11 +409,18 @@ static void process_one_work(struct cpu_workqueue_struct *cwq,
>
> BUG_ON(get_wq_data(work) != cwq);
> work_clear_pending(work);
> - lock_map_acquire(&cwq->wq->lockdep_map);
> + lock_map_acquire(&wq->lockdep_map);
> lock_map_acquire(&lockdep_map);
> - f(work);
> +
> + if (unlikely(single_thread)) {
> + mutex_lock(&wq->single_thread_mutex);
> + f(work);
> + mutex_unlock(&wq->single_thread_mutex);
> + } else
> + f(work);
> +

An important property of the single threaded workqueue, upon which the
cx18 driver relies, is that work objects will be processed strictly in
the order in which they were queued. The cx18 driver has a pool of
"work orders" and multiple active work orders can be queued up on the
workqueue especially if multiple streams are active. If these work
orders were to be processed out of order, video artifacts would result
in video display applications.


With multiple work handling threads, I don't think the

mutex_lock(&wq->single_thread_mutex);
f(work);

here can guarantee work requests from the workqueue will always be
processed in the order they are received.

Am I missing something?

Regards,
Andy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


tj at kernel

Nov 16, 2009, 9:23 PM

Post #3 of 10 (1243 views)
Permalink
Re: [PATCH 17/21] workqueue: simple reimplementation of SINGLE_THREAD workqueue [In reply to]

Hello,

11/17/2009 09:47 AM, Andy Walls wrote:
> An important property of the single threaded workqueue, upon which the
> cx18 driver relies, is that work objects will be processed strictly in
> the order in which they were queued. The cx18 driver has a pool of
> "work orders" and multiple active work orders can be queued up on the
> workqueue especially if multiple streams are active. If these work
> orders were to be processed out of order, video artifacts would result
> in video display applications.

That's an interesting use of single thread workqueue. Most of single
thread workqueues seem to be made single thread just to save number of
threads. Some seem to depend on single thread of execution but I
never knew there are ones which depend on the exact execution order.
Do you think that usage is wide-spread? Implementing strict ordering
shouldn't be too difficult but I can't help but feeling that such
assumption is abuse of implementation detail.

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


awalls at radix

Nov 17, 2009, 4:05 AM

Post #4 of 10 (1239 views)
Permalink
Re: [PATCH 17/21] workqueue: simple reimplementation of SINGLE_THREAD workqueue [In reply to]

On Tue, 2009-11-17 at 14:23 +0900, Tejun Heo wrote:
> Hello,
>
> 11/17/2009 09:47 AM, Andy Walls wrote:
> > An important property of the single threaded workqueue, upon which the
> > cx18 driver relies, is that work objects will be processed strictly in
> > the order in which they were queued. The cx18 driver has a pool of
> > "work orders" and multiple active work orders can be queued up on the
> > workqueue especially if multiple streams are active. If these work
> > orders were to be processed out of order, video artifacts would result
> > in video display applications.
>
> That's an interesting use of single thread workqueue. Most of single
> thread workqueues seem to be made single thread just to save number of
> threads. Some seem to depend on single thread of execution but I
> never knew there are ones which depend on the exact execution order.
> Do you think that usage is wide-spread?

I doubt it.

Most that I have seen use the singlethreaded workqueue object with a
queue depth of essentially 1 for syncronization - as you have noted.


> Implementing strict ordering
> shouldn't be too difficult but I can't help but feeling that such
> assumption is abuse of implementation detail.

Hmmm, does not the "queue" in workqueue mean "FIFO"?

If not for strict ordering, why else would a driver absolutely need a
singlethreaded workqueue object? It seems to me the strict ording is
the driving requirement for a singlethreaded workqueue at all. Your
patch series indicates to me that the performance and synchronization
use cases are not driving requirements for a singlethreaded workqueue.

Thanks for your consideration.

Regards,
Andy

> Thanks.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


johannes at sipsolutions

Nov 17, 2009, 6:03 AM

Post #5 of 10 (1244 views)
Permalink
Re: [PATCH 17/21] workqueue: simple reimplementation of SINGLE_THREAD workqueue [In reply to]

On Tue, 2009-11-17 at 02:15 +0900, Tejun Heo wrote:
> SINGLE_THREAD workqueues are used to reduce the number of worker
> threads and ease synchronization.

Wireless (mac80211) also requires that the order in which different work
structs are queued up is identical to the processing order. At least
some code was written with that assumption in mind, and I think it's
actually required in a few places.

Also, that unlikely() here:

> + if (unlikely(single_thread)) {
> + mutex_lock(&wq->single_thread_mutex);
> + f(work);
> + mutex_unlock(&wq->single_thread_mutex);
> + } else
> + f(work);

seems wrong, there are many single-threaded workqueues after all.

johannes
Attachments: signature.asc (0.78 KB)


torvalds at linux-foundation

Nov 17, 2009, 7:05 AM

Post #6 of 10 (1236 views)
Permalink
Re: [PATCH 17/21] workqueue: simple reimplementation of SINGLE_THREAD workqueue [In reply to]

On Tue, 17 Nov 2009, Tejun Heo wrote:
>
> Do you think that usage is wide-spread? Implementing strict ordering
> shouldn't be too difficult but I can't help but feeling that such
> assumption is abuse of implementation detail.

I think it would be good if it were more than an implementation detail,
and was something documented and known.

The less random and timing-dependent our interfaces are, the better off we
are. Guaranteeing that a single-threaded workqueue is done in order seems
to me to be a GoodThing(tm), regardless of whether much code depends on
it.

Of course, if there is some fundamental reason why it wouldn't be the
case, that's another thing. But if you think uit should be easy, and since
there _are_ users, then it shouldn't be seen as an "implementation
detail". It's a feature.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


tj at kernel

Nov 17, 2009, 8:12 AM

Post #7 of 10 (1235 views)
Permalink
Re: [PATCH 17/21] workqueue: simple reimplementation of SINGLE_THREAD workqueue [In reply to]

Hello, Linus.

11/18/2009 12:05 AM, Linus Torvalds wrote:
>> Do you think that usage is wide-spread? Implementing strict ordering
>> shouldn't be too difficult but I can't help but feeling that such
>> assumption is abuse of implementation detail.
>
> I think it would be good if it were more than an implementation detail,
> and was something documented and known.
>
> The less random and timing-dependent our interfaces are, the better off we
> are. Guaranteeing that a single-threaded workqueue is done in order seems
> to me to be a GoodThing(tm), regardless of whether much code depends on
> it.
>
> Of course, if there is some fundamental reason why it wouldn't be the
> case, that's another thing. But if you think uit should be easy, and since
> there _are_ users, then it shouldn't be seen as an "implementation
> detail". It's a feature.

I might have been too early with the 'easy' part but I definitely can
give it a shot. What do you think about the scheduler notifier
implementation? It seems we'll end up with three callbacks. It can
either be three hlist_heads in the struct_task linking each ops or
single hilst_head links ops tables (like the current preempt
notifiers). Which one should I go with?

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


tj at kernel

Nov 17, 2009, 8:21 AM

Post #8 of 10 (1257 views)
Permalink
Re: [PATCH 17/21] workqueue: simple reimplementation of SINGLE_THREAD workqueue [In reply to]

Hello,

11/17/2009 09:05 PM, Andy Walls wrote:
>> Implementing strict ordering
>> shouldn't be too difficult but I can't help but feeling that such
>> assumption is abuse of implementation detail.
>
> Hmmm, does not the "queue" in workqueue mean "FIFO"?

I don't think it necessarily means strict execution ordering.

> If not for strict ordering, why else would a driver absolutely need a
> singlethreaded workqueue object? It seems to me the strict ording is
> the driving requirement for a singlethreaded workqueue at all. Your
> patch series indicates to me that the performance and synchronization
> use cases are not driving requirements for a singlethreaded workqueue.

I still think the biggest reason why single threaded workqueue is used
is just to reduce the number of threads hanging around. I tried to
audit single thread users some time ago. My impression was that many
of single thread work users did synchronization itself anyway while
smaller portion depended on single threadedness. I didn't notice the
strict ordering requirement but then again I wasn't looking for them.
It seems there are at least two cases depending on FIFO behavior, so
let's see if we can retain the behavior for single threaded
workqueues (maybe it should be renamed to ORDERED?).

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


tj at kernel

Nov 17, 2009, 8:24 AM

Post #9 of 10 (1232 views)
Permalink
Re: [PATCH 17/21] workqueue: simple reimplementation of SINGLE_THREAD workqueue [In reply to]

Hello,

11/17/2009 11:03 PM, Johannes Berg wrote:
> On Tue, 2009-11-17 at 02:15 +0900, Tejun Heo wrote:
>> SINGLE_THREAD workqueues are used to reduce the number of worker
>> threads and ease synchronization.
>
> Wireless (mac80211) also requires that the order in which different work
> structs are queued up is identical to the processing order. At least
> some code was written with that assumption in mind, and I think it's
> actually required in a few places.

Thanks for pointing it out.

> Also, that unlikely() here:
>
>> + if (unlikely(single_thread)) {
>> + mutex_lock(&wq->single_thread_mutex);
>> + f(work);
>> + mutex_unlock(&wq->single_thread_mutex);
>> + } else
>> + f(work);
>
> seems wrong, there are many single-threaded workqueues after all.

Well, most single threaded users which chose single threaded queue to
reduce the number of threads won't need to, so I'm expecting the
number of single threaded users to drop. I'll probably drop the
unlikely on the next round.

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


torvalds at linux-foundation

Nov 17, 2009, 11:01 AM

Post #10 of 10 (1231 views)
Permalink
Re: [PATCH 17/21] workqueue: simple reimplementation of SINGLE_THREAD workqueue [In reply to]

On Wed, 18 Nov 2009, Tejun Heo wrote:
>
> I might have been too early with the 'easy' part but I definitely can
> give it a shot. What do you think about the scheduler notifier
> implementation? It seems we'll end up with three callbacks. It can
> either be three hlist_heads in the struct_task linking each ops or
> single hilst_head links ops tables (like the current preempt
> notifiers). Which one should I go with?

I have to say that I don't know. Will this eventually be something common?
Is the cache footprint problem of 3 pointers that are usually empty worse
than the cache problem of following a chain where you don't use half the
entries? Who knows?

And when it actually _is_ used, is it going to be horrible to have a
possibly upredictable indirect branch (and on some architectures, _all_
indirect branches are unpredictable) in a really hot path?

In general, "notifiers" are always horrible. If there's only one or two
common cases, it's probably going to be better to hardcode those with
flags to be tested instead of following function pointers. So I just don't
know.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Linux kernel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.