Re: [PATCH 2/6] workqueue: factor out worker_pool from global_cwq

[Namhyung Kim <namhyung@kernel.org>]

Hi, Tejun

Just nitpicks..


On Mon,  9 Jul 2012 11:41:51 -0700, Tejun Heo wrote:
> Move worklist and all worker management fields from global_cwq into
> the new struct worker_pool.  worker_pool points back to the containing
> gcwq.  worker and cpu_workqueue_struct are updated to point to
> worker_pool instead of gcwq too.
>
> This change is mechanical and doesn't introduce any functional
> difference other than rearranging of fields and an added level of
> indirection in some places.  This is to prepare for multiple pools per
> gcwq.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  include/trace/events/workqueue.h |    2 +-
>  kernel/workqueue.c               |  216 ++++++++++++++++++++-----------------
>  2 files changed, 118 insertions(+), 100 deletions(-)
>
> diff --git a/include/trace/events/workqueue.h b/include/trace/events/workqueue.h
> index 4018f50..f28d1b6 100644
> --- a/include/trace/events/workqueue.h
> +++ b/include/trace/events/workqueue.h
> @@ -54,7 +54,7 @@ TRACE_EVENT(workqueue_queue_work,
>  		__entry->function	= work->func;
>  		__entry->workqueue	= cwq->wq;
>  		__entry->req_cpu	= req_cpu;
> -		__entry->cpu		= cwq->gcwq->cpu;
> +		__entry->cpu		= cwq->pool->gcwq->cpu;
>  	),
>  
>  	TP_printk("work struct=%p function=%pf workqueue=%p req_cpu=%u cpu=%u",
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 27637c2..bc43a0c 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -115,6 +115,7 @@ enum {
>   */
>  
>  struct global_cwq;
> +struct worker_pool;
>  
>  /*
>   * The poor guys doing the actual heavy lifting.  All on-duty workers
> @@ -131,7 +132,7 @@ struct worker {
>  	struct cpu_workqueue_struct *current_cwq; /* L: current_work's cwq */
>  	struct list_head	scheduled;	/* L: scheduled works */
>  	struct task_struct	*task;		/* I: worker task */
> -	struct global_cwq	*gcwq;		/* I: the associated gcwq */
> +	struct worker_pool	*pool;		/* I: the associated pool */
>  	/* 64 bytes boundary on 64bit, 32 on 32bit */
>  	unsigned long		last_active;	/* L: last active timestamp */
>  	unsigned int		flags;		/* X: flags */
> @@ -139,6 +140,21 @@ struct worker {
>  	struct work_struct	rebind_work;	/* L: rebind worker to cpu */
>  };
>  
> +struct worker_pool {
> +	struct global_cwq	*gcwq;		/* I: the owning gcwq */
> +
> +	struct list_head	worklist;	/* L: list of pending works */
> +	int			nr_workers;	/* L: total number of workers */
> +	int			nr_idle;	/* L: currently idle ones */
> +
> +	struct list_head	idle_list;	/* X: list of idle workers */
> +	struct timer_list	idle_timer;	/* L: worker idle timeout */
> +	struct timer_list	mayday_timer;	/* L: SOS timer for dworkers */

What is 'dworkers'?


> +
> +	struct ida		worker_ida;	/* L: for worker IDs */
> +	struct worker		*first_idle;	/* L: first idle worker */
> +};
> +
>  /*
>   * Global per-cpu workqueue.  There's one and only one for each cpu
>   * and all works are queued and processed here regardless of their
> @@ -146,27 +162,18 @@ struct worker {
>   */
>  struct global_cwq {
>  	spinlock_t		lock;		/* the gcwq lock */
> -	struct list_head	worklist;	/* L: list of pending works */
>  	unsigned int		cpu;		/* I: the associated cpu */
>  	unsigned int		flags;		/* L: GCWQ_* flags */
>  
> -	int			nr_workers;	/* L: total number of workers */
> -	int			nr_idle;	/* L: currently idle ones */
> -
> -	/* workers are chained either in the idle_list or busy_hash */
> -	struct list_head	idle_list;	/* X: list of idle workers */
> +	/* workers are chained either in busy_head or pool idle_list */

s/busy_head/busy_hash/ ?

Thanks,
Namhyung


>  	struct hlist_head	busy_hash[BUSY_WORKER_HASH_SIZE];
>  						/* L: hash of busy workers */
>  
> -	struct timer_list	idle_timer;	/* L: worker idle timeout */
> -	struct timer_list	mayday_timer;	/* L: SOS timer for dworkers */
> -
> -	struct ida		worker_ida;	/* L: for worker IDs */
> +	struct worker_pool	pool;		/* the worker pools */
>  
>  	struct task_struct	*trustee;	/* L: for gcwq shutdown */
>  	unsigned int		trustee_state;	/* L: trustee state */
>  	wait_queue_head_t	trustee_wait;	/* trustee wait */
> -	struct worker		*first_idle;	/* L: first idle worker */
>  } ____cacheline_aligned_in_smp;
>  
>  /*