Re: [PATCH 17/21] workqueue: simple reimplementation ofSINGLE_THREAD workqueue

[Andy Walls]

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@xxxxxxxxxx>
> 
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/