Re: [PATCH 06/10 V2] workqueue: convert worker_idr to worker_ida

[Tejun Heo]

Hello,

On Tue, May 13, 2014 at 02:43:06PM +0800, Lai Jiangshan wrote:
> On 05/13/2014 05:40 AM, Tejun Heo wrote:
> > On Mon, May 12, 2014 at 02:56:18PM +0800, Lai Jiangshan wrote:
> >> @@ -1681,7 +1682,6 @@ static void worker_detach_from_pool(struct worker *worker,
> >>  	struct completion *detach_completion = NULL;
> >>  
> >>  	mutex_lock(&pool->manager_mutex);
> >> -	idr_remove(&pool->worker_idr, worker->id);
> >>  	list_del(&worker->node);
> >>  	if (list_empty(&pool->workers))
> >>  		detach_completion = pool->detach_completion;
> > 
> > Why are we moving ida removal to the caller here?  Does
> 
> ida							is for worker ID
> pool->workers list and worker_detach_from_pool()	are for attaching/detaching
> 
> moving ida removal to the caller removes the unneeded coupling.

This is completely arbitrary.  For example, if rescuers needed IDs to
be allocated and deallocated on attach/detach, ID allocation should be
included in the above two functions, right?  This makes sense only
because rescuers don't have IDs and we're gonna use the above
functions for the rescuers too.  There's nothing inherent in
decoupling a worker's attachment to its pool and its pool ID
allocation.  The design developed this way only because there's
certain specific usage for it.  The code is fine but it usually is a
lot more helpful for reviewing and later reference if you actually
explain why things are done in certain specific ways because in
isolation the above decoupling is completely arbitrary.

Thanks.

-- 
tejun
--
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/