On Mon, May 5, 2014 at 9:01 PM, Tejun Heo <t...@kernel.org> wrote: > On Sun, Apr 27, 2014 at 12:08:56PM +0800, Lai Jiangshan wrote: >> worker_idr is highly bound to managers and is always/only accessed in manager >> lock context. So we don't need pool->lock for it. >> >> Signed-off-by: Lai Jiangshan <la...@cn.fujitsu.com> > ... >> @@ -378,14 +367,14 @@ static void copy_workqueue_attrs(struct >> workqueue_attrs *to, >> * @wi: integer used for iteration >> * @pool: worker_pool to iterate workers of >> * >> - * This must be called with either @pool->manager_mutex or ->lock held. >> + * This must be called with either @pool->manager_mutex. > > Please drop "either" from the sentence.
Ahh, I will do it. > >> @@ -1725,13 +1714,7 @@ static struct worker *create_worker(struct >> worker_pool *pool) >> * ID is needed to determine kthread name. Allocate ID first >> * without installing the pointer. >> */ >> - idr_preload(GFP_KERNEL); >> - spin_lock_irq(&pool->lock); >> - >> - id = idr_alloc(&pool->worker_idr, NULL, 0, 0, GFP_NOWAIT); >> - >> - spin_unlock_irq(&pool->lock); >> - idr_preload_end(); >> + id = idr_alloc(&pool->worker_idr, NULL, 0, 0, GFP_KERNEL); >> if (id < 0) >> goto fail; >> >> @@ -1773,18 +1756,13 @@ static struct worker *create_worker(struct >> worker_pool *pool) >> worker->flags |= WORKER_UNBOUND; >> >> /* successful, commit the pointer to idr */ >> - spin_lock_irq(&pool->lock); >> idr_replace(&pool->worker_idr, worker, worker->id); >> - spin_unlock_irq(&pool->lock); > > W/ locking updated, we can simply assign the pointer on idr_alloc() > instead of doing split alloc/replace. I hope the patch simpler which just removes the lock around it. this idr_replace() will be removed in later patch, so it is OK when idr_replace() is temporary kept. And I don't want to add a fail path after the thread is allocated. > > I'm a bit on the fence about this patch. It does simplify the code a > bit but then we lose the ability to iterate workers without grabbing > the manager_mutex, which would come handy when, for example, > implementing better workqueue info reporting during oops which we'll > prolly need to add sooner or later. It can be done by idle_list + busy_hash if needed. Or add an additional list. Thanks Lai > > Thanks. > > -- > tejun > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/