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

Tejun Heo
2014-05-12T21:36:02Z

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