Re: [PATCH 05/10 V2] workqueue: separate iteration role from worker_idr

[Tejun Heo <tj@kernel.org>]

On Mon, May 12, 2014 at 02:56:17PM +0800, Lai Jiangshan wrote:
> worker_idr has the iteration(iterating for attached workers) and worker ID
                              ^
Please put a space before an opening parenthses.  It becomes a lot
easier on the eyes.

> duties. These two duties are not necessary tied together. We can separate
                                   necessarily

> them and use a list for tracking attached workers and iteration
> 
> After separation, we can add the rescuer workers to the list for iteration
> in future. worker_idr can't add rescuer workers due to rescuer workers
> can't allocate id from worker_idr.

Explaining how that'd be beneficial and justifies this change would be
nice.

...
>  /**
> - * for_each_pool_worker - iterate through all workers of a worker_pool
> - * @worker: iteration cursor
> - * @wi: integer used for iteration
> - * @pool: worker_pool to iterate workers of
> - *
> - * This must be called with @pool->manager_mutex.
> - *
> - * The if/else clause exists only for the lockdep assertion and can be
> - * ignored.
> - */
> -#define for_each_pool_worker(worker, wi, pool)				\
> -	idr_for_each_entry(&(pool)->worker_idr, (worker), (wi))		\
> -		if (({ lockdep_assert_held(&pool->manager_mutex); false; })) { } \
> -		else

Hmmm.... don't we still want the lockdep protection?

> @@ -1772,6 +1759,8 @@ static struct worker *create_worker(struct worker_pool *pool)
>  
>  	/* successful, commit the pointer to idr */

	/* successful, commit it to idr and link on the workers list */

>  	idr_replace(&pool->worker_idr, worker, worker->id);
> +	/* successful, attach the worker to the pool */

and lose the above line.

> diff --git a/kernel/workqueue_internal.h b/kernel/workqueue_internal.h
> index 7e2204d..c44abc3 100644
> --- a/kernel/workqueue_internal.h
> +++ b/kernel/workqueue_internal.h
> @@ -37,6 +37,7 @@ struct worker {
>  	struct task_struct	*task;		/* I: worker task */
>  	struct worker_pool	*pool;		/* I: the associated pool */
>  						/* L: for rescuers */
> +	struct list_head	node;		/* M: attached to the pool */

						/* M: anchored at pool->workers */

Thanks.

-- 
tejun