On 07/14/2014 12:05 PM, Lai Jiangshan wrote: > Simply unfold the code of start_worker() into create_worker() and > remove the original start_worker() and create_and_start_worker(). > > The only trade-off is the introduced overhead that the pool->lock > is released and re-grabbed after the newly worker is started. > The overhead is acceptable since the manager is slow path.
Hi, TJ Will you accept this trade-off and the patch? If so, I will rebase this patch without any dependence on other patch. Thanks, Lai > > And because this new locking behavior, the newly created worker > may grab the lock earlier than the manager and go to process > work items. In this case, the need_to_create_worker() may be > true as expected and the manager goes to restart without complaint. > > Signed-off-by: Lai Jiangshan <[email protected]> > --- > kernel/workqueue.c | 69 > ++++++++++------------------------------------------ > 1 files changed, 13 insertions(+), 56 deletions(-) > > diff --git a/kernel/workqueue.c b/kernel/workqueue.c > index 2b3c12c..5b68527 100644 > --- a/kernel/workqueue.c > +++ b/kernel/workqueue.c > @@ -1553,7 +1553,7 @@ static void worker_enter_idle(struct worker *worker) > (worker->hentry.next || worker->hentry.pprev))) > return; > > - /* can't use worker_set_flags(), also called from start_worker() */ > + /* can't use worker_set_flags(), also called from create_worker() */ > worker->flags |= WORKER_IDLE; > pool->nr_idle++; > worker->last_active = jiffies; > @@ -1674,8 +1674,7 @@ static void worker_detach_from_pool(struct worker > *worker, > * create_worker - create a new workqueue worker > * @pool: pool the new worker will belong to > * > - * Create a new worker which is attached to @pool. The new worker must be > - * started by start_worker(). > + * Create and start a new worker which is attached to @pool. > * > * CONTEXT: > * Might sleep. Does GFP_KERNEL allocations. > @@ -1720,6 +1719,13 @@ static struct worker *create_worker(struct worker_pool > *pool) > /* successful, attach the worker to the pool */ > worker_attach_to_pool(worker, pool); > > + /* start the newly created worker */ > + spin_lock_irq(&pool->lock); > + worker->pool->nr_workers++; > + worker_enter_idle(worker); > + wake_up_process(worker->task); > + spin_unlock_irq(&pool->lock); > + > return worker; > > fail: > @@ -1730,44 +1736,6 @@ fail: > } > > /** > - * start_worker - start a newly created worker > - * @worker: worker to start > - * > - * Make the pool aware of @worker and start it. > - * > - * CONTEXT: > - * spin_lock_irq(pool->lock). > - */ > -static void start_worker(struct worker *worker) > -{ > - worker->pool->nr_workers++; > - worker_enter_idle(worker); > - wake_up_process(worker->task); > -} > - > -/** > - * create_and_start_worker - create and start a worker for a pool > - * @pool: the target pool > - * > - * Grab the managership of @pool and create and start a new worker for it. > - * > - * Return: 0 on success. A negative error code otherwise. > - */ > -static int create_and_start_worker(struct worker_pool *pool) > -{ > - struct worker *worker; > - > - worker = create_worker(pool); > - if (worker) { > - spin_lock_irq(&pool->lock); > - start_worker(worker); > - spin_unlock_irq(&pool->lock); > - } > - > - return worker ? 0 : -ENOMEM; > -} > - > -/** > * destroy_worker - destroy a workqueue worker > * @worker: worker to be destroyed > * > @@ -1905,18 +1873,7 @@ restart: > mod_timer(&pool->mayday_timer, jiffies + MAYDAY_INITIAL_TIMEOUT); > > while (true) { > - struct worker *worker; > - > - worker = create_worker(pool); > - if (worker) { > - spin_lock_irq(&pool->lock); > - start_worker(worker); > - if (WARN_ON_ONCE(need_to_create_worker(pool))) > - goto restart; > - return true; > - } > - > - if (!need_to_create_worker(pool)) > + if (create_worker(pool) || !need_to_create_worker(pool)) > break; > > schedule_timeout_interruptible(CREATE_COOLDOWN); > @@ -3543,7 +3500,7 @@ static struct worker_pool *get_unbound_pool(const > struct workqueue_attrs *attrs) > goto fail; > > /* create and start the initial worker */ > - if (create_and_start_worker(pool) < 0) > + if (!create_worker(pool)) > goto fail; > > /* install */ > @@ -4617,7 +4574,7 @@ static int workqueue_cpu_up_callback(struct > notifier_block *nfb, > for_each_cpu_worker_pool(pool, cpu) { > if (pool->nr_workers) > continue; > - if (create_and_start_worker(pool) < 0) > + if (!create_worker(pool)) > return NOTIFY_BAD; > } > break; > @@ -4917,7 +4874,7 @@ static int __init init_workqueues(void) > > for_each_cpu_worker_pool(pool, cpu) { > pool->flags &= ~POOL_DISASSOCIATED; > - BUG_ON(create_and_start_worker(pool) < 0); > + BUG_ON(!create_worker(pool)); > } > } > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
