Merge branch 'for-3.6-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq

Pull workqueue fixes from Tejun Heo:
"It's later than I'd like but well the timing just didn't work out this
time.

There are three bug fixes. One from before 3.6-rc1 and two from the
new CPU hotplug code. Kudos to Lai for discovering all of them and
providing fixes.

* Atomicity bug when clearing a flag and setting another. The two
operation should have been atomic but wasn't. This bug has existed
for a long time but is unlikely to have actually happened. Fix is
safe. Marked for -stable.

* If CPU hotplug cycles happen back-to-back before workers finish the
previous cycle, the states could get out of sync and it could get
stuck. Fixed by waiting for workers to complete before finishing
hotplug cycle.

* While CPU hotplug is in progress, idle workers could be depleted
which can then lead to deadlock. I think both happening together
is highly unlikely but still better to fix it and the fix isn't too
scary.

There's another workqueue related regression which reported a few days
ago:

https://bugzilla.kernel.org/show_bug.cgi?id=47301

It's a bit of head scratcher but there is a semi-reliable reproduce
case, so I'm hoping to resolve it soonish."

* 'for-3.6-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq:
workqueue: fix possible idle worker depletion across CPU hotplug
workqueue: restore POOL_MANAGING_WORKERS
workqueue: fix possible deadlock in idle worker rebinding
workqueue: move WORKER_REBIND clearing in rebind_workers() to the end of the function
workqueue: UNBOUND -> REBIND morphing in rebind_workers() should be atomic

Linus Torvalds 14 years ago 0bd1189e 274a2f5d

+89 -21

1 changed file

expand all

kernel

workqueue.c

+89 -21

kernel/workqueue.c

··· 66 66 67 67 /* pool flags */ 68 68 POOL_MANAGE_WORKERS = 1 << 0, /* need to manage workers */ 69 + POOL_MANAGING_WORKERS = 1 << 1, /* managing workers */ 69 70 70 71 /* worker flags */ 71 72 WORKER_STARTED = 1 << 0, /* started */ ··· 653 652 /* Do we have too many workers and should some go away? */ 654 653 static bool too_many_workers(struct worker_pool *pool) 655 654 { 656 - bool managing = mutex_is_locked(&pool->manager_mutex); 655 + bool managing = pool->flags & POOL_MANAGING_WORKERS; 657 656 int nr_idle = pool->nr_idle + managing; /* manager is considered idle */ 658 657 int nr_busy = pool->nr_workers - nr_idle; 659 658 ··· 1327 1326 1328 1327 /* we did our part, wait for rebind_workers() to finish up */ 1329 1328 wait_event(gcwq->rebind_hold, !(worker->flags & WORKER_REBIND)); 1329 + 1330 + /* 1331 + * rebind_workers() shouldn't finish until all workers passed the 1332 + * above WORKER_REBIND wait. Tell it when done. 1333 + */ 1334 + spin_lock_irq(&worker->pool->gcwq->lock); 1335 + if (!--worker->idle_rebind->cnt) 1336 + complete(&worker->idle_rebind->done); 1337 + spin_unlock_irq(&worker->pool->gcwq->lock); 1330 1338 } 1331 1339 1332 1340 /* ··· 1406 1396 /* set REBIND and kick idle ones, we'll wait for these later */ 1407 1397 for_each_worker_pool(pool, gcwq) { 1408 1398 list_for_each_entry(worker, &pool->idle_list, entry) { 1399 + unsigned long worker_flags = worker->flags; 1400 + 1409 1401 if (worker->flags & WORKER_REBIND) 1410 1402 continue; 1411 1403 1412 - /* morph UNBOUND to REBIND */ 1413 - worker->flags &= ~WORKER_UNBOUND; 1414 - worker->flags |= WORKER_REBIND; 1404 + /* morph UNBOUND to REBIND atomically */ 1405 + worker_flags &= ~WORKER_UNBOUND; 1406 + worker_flags |= WORKER_REBIND; 1407 + ACCESS_ONCE(worker->flags) = worker_flags; 1415 1408 1416 1409 idle_rebind.cnt++; 1417 1410 worker->idle_rebind = &idle_rebind; ··· 1432 1419 goto retry; 1433 1420 } 1434 1421 1435 - /* 1436 - * All idle workers are rebound and waiting for %WORKER_REBIND to 1437 - * be cleared inside idle_worker_rebind(). Clear and release. 1438 - * Clearing %WORKER_REBIND from this foreign context is safe 1439 - * because these workers are still guaranteed to be idle. 1440 - */ 1441 - for_each_worker_pool(pool, gcwq) 1442 - list_for_each_entry(worker, &pool->idle_list, entry) 1443 - worker->flags &= ~WORKER_REBIND; 1444 - 1445 - wake_up_all(&gcwq->rebind_hold); 1446 - 1447 - /* rebind busy workers */ 1422 + /* all idle workers are rebound, rebind busy workers */ 1448 1423 for_each_busy_worker(worker, i, pos, gcwq) { 1449 1424 struct work_struct *rebind_work = &worker->rebind_work; 1425 + unsigned long worker_flags = worker->flags; 1450 1426 1451 - /* morph UNBOUND to REBIND */ 1452 - worker->flags &= ~WORKER_UNBOUND; 1453 - worker->flags |= WORKER_REBIND; 1427 + /* morph UNBOUND to REBIND atomically */ 1428 + worker_flags &= ~WORKER_UNBOUND; 1429 + worker_flags |= WORKER_REBIND; 1430 + ACCESS_ONCE(worker->flags) = worker_flags; 1454 1431 1455 1432 if (test_and_set_bit(WORK_STRUCT_PENDING_BIT, 1456 1433 work_data_bits(rebind_work))) ··· 1451 1448 insert_work(get_cwq(gcwq->cpu, system_wq), rebind_work, 1452 1449 worker->scheduled.next, 1453 1450 work_color_to_flags(WORK_NO_COLOR)); 1451 + } 1452 + 1453 + /* 1454 + * All idle workers are rebound and waiting for %WORKER_REBIND to 1455 + * be cleared inside idle_worker_rebind(). Clear and release. 1456 + * Clearing %WORKER_REBIND from this foreign context is safe 1457 + * because these workers are still guaranteed to be idle. 1458 + * 1459 + * We need to make sure all idle workers passed WORKER_REBIND wait 1460 + * in idle_worker_rebind() before returning; otherwise, workers can 1461 + * get stuck at the wait if hotplug cycle repeats. 1462 + */ 1463 + idle_rebind.cnt = 1; 1464 + INIT_COMPLETION(idle_rebind.done); 1465 + 1466 + for_each_worker_pool(pool, gcwq) { 1467 + list_for_each_entry(worker, &pool->idle_list, entry) { 1468 + worker->flags &= ~WORKER_REBIND; 1469 + idle_rebind.cnt++; 1470 + } 1471 + } 1472 + 1473 + wake_up_all(&gcwq->rebind_hold); 1474 + 1475 + if (--idle_rebind.cnt) { 1476 + spin_unlock_irq(&gcwq->lock); 1477 + wait_for_completion(&idle_rebind.done); 1478 + spin_lock_irq(&gcwq->lock); 1454 1479 } 1455 1480 } 1456 1481 ··· 1825 1794 struct worker_pool *pool = worker->pool; 1826 1795 bool ret = false; 1827 1796 1828 - if (!mutex_trylock(&pool->manager_mutex)) 1797 + if (pool->flags & POOL_MANAGING_WORKERS) 1829 1798 return ret; 1799 + 1800 + pool->flags |= POOL_MANAGING_WORKERS; 1801 + 1802 + /* 1803 + * To simplify both worker management and CPU hotplug, hold off 1804 + * management while hotplug is in progress. CPU hotplug path can't 1805 + * grab %POOL_MANAGING_WORKERS to achieve this because that can 1806 + * lead to idle worker depletion (all become busy thinking someone 1807 + * else is managing) which in turn can result in deadlock under 1808 + * extreme circumstances. Use @pool->manager_mutex to synchronize 1809 + * manager against CPU hotplug. 1810 + * 1811 + * manager_mutex would always be free unless CPU hotplug is in 1812 + * progress. trylock first without dropping @gcwq->lock. 1813 + */ 1814 + if (unlikely(!mutex_trylock(&pool->manager_mutex))) { 1815 + spin_unlock_irq(&pool->gcwq->lock); 1816 + mutex_lock(&pool->manager_mutex); 1817 + /* 1818 + * CPU hotplug could have happened while we were waiting 1819 + * for manager_mutex. Hotplug itself can't handle us 1820 + * because manager isn't either on idle or busy list, and 1821 + * @gcwq's state and ours could have deviated. 1822 + * 1823 + * As hotplug is now excluded via manager_mutex, we can 1824 + * simply try to bind. It will succeed or fail depending 1825 + * on @gcwq's current state. Try it and adjust 1826 + * %WORKER_UNBOUND accordingly. 1827 + */ 1828 + if (worker_maybe_bind_and_lock(worker)) 1829 + worker->flags &= ~WORKER_UNBOUND; 1830 + else 1831 + worker->flags |= WORKER_UNBOUND; 1832 + 1833 + ret = true; 1834 + } 1830 1835 1831 1836 pool->flags &= ~POOL_MANAGE_WORKERS; 1832 1837 ··· 1873 1806 ret |= maybe_destroy_workers(pool); 1874 1807 ret |= maybe_create_worker(pool); 1875 1808 1809 + pool->flags &= ~POOL_MANAGING_WORKERS; 1876 1810 mutex_unlock(&pool->manager_mutex); 1877 1811 return ret; 1878 1812 }

Configure Feed

Configure Feed