In create_worker(), as pool->worker_ida now uses ida_simple_get()/ida_simple_put() and doesn't require external synchronization, it doesn't need manager_mutex. struct worker allocation and kthread allocation are not visible by any one before attached, so they don't need manager_mutex either. The above operations are before the attaching operation which attaches the worker to the pool. Between attaching and starting the worker, the worker is already attached to the pool, so the cpu hotplug will handle cpu-binding for the worker correctly and we don't need the manager_mutex after attaching. The conclusion is that only the attaching operation needs manager_mutex, so we narrow the protection section of manager_mutex in create_worker(). Some comments about manager_mutex are removed, because we will rename it to attach_mutex and add worker_attach_to_pool() later which will be self-explanatory. tj: Minor description updates.
Showing the changes to kernel/workqueue.c (1 of 1 file). Show all
Unified · Split
| @@ -1725,8 +1725,6 @@ static struct worker *create_worker(struct worker_pool *pool) · 1724 unmodified lines | ||
| 1725 | 1725 | int id = -1; |
| 1726 | 1726 | char id_buf[16]; |
| 1727 | 1727 | |
| 1728 | − lockdep_assert_held(&pool->manager_mutex); | |
| 1729 | − | |
| 1730 | 1728 | /* ID is needed to determine kthread name */ |
| 1731 | 1729 | id = ida_simple_get(&pool->worker_ida, 0, 0, GFP_KERNEL); |
| 1732 | 1730 | if (id < 0) |
| @@ -1755,6 +1753,8 @@ static struct worker *create_worker(struct worker_pool *pool) · 22 unmodified lines | ||
| 1755 | 1753 | /* prevent userland from meddling with cpumask of workqueue workers */ |
| 1756 | 1754 | worker->task->flags |= PF_NO_SETAFFINITY; |
| 1757 | 1755 | |
| 1756 | + mutex_lock(&pool->manager_mutex); | |
| 1757 | + | |
| 1758 | 1758 | /* |
| 1759 | 1759 | * set_cpus_allowed_ptr() will fail if the cpumask doesn't have any |
| 1760 | 1760 | * online CPUs. It'll be re-applied when any of the CPUs come up. |
| @@ -1762,7 +1762,7 @@ static struct worker *create_worker(struct worker_pool *pool) · 1 unmodified line | ||
| 1762 | 1762 | set_cpus_allowed_ptr(worker->task, pool->attrs->cpumask); |
| 1763 | 1763 | |
| 1764 | 1764 | /* |
| 1765 | − * The caller is responsible for ensuring %POOL_DISASSOCIATED | |
| 1765 | + * The pool->manager_mutex ensures %POOL_DISASSOCIATED | |
| 1766 | 1766 | * remains stable across this function. See the comments above the |
| 1767 | 1767 | * flag definition for details. |
| 1768 | 1768 | */ |
| @@ -1772,6 +1772,8 @@ static struct worker *create_worker(struct worker_pool *pool) · 3 unmodified lines | ||
| 1772 | 1772 | /* successful, attach the worker to the pool */ |
| 1773 | 1773 | list_add_tail(&worker->node, &pool->workers); |
| 1774 | 1774 | |
| 1775 | + mutex_unlock(&pool->manager_mutex); | |
| 1776 | + | |
| 1775 | 1777 | return worker; |
| 1776 | 1778 | |
| 1777 | 1779 | fail: |
| @@ -1809,8 +1811,6 @@ static int create_and_start_worker(struct worker_pool *pool) · 31 unmodified lines | ||
| 1809 | 1811 | { |
| 1810 | 1812 | struct worker *worker; |
| 1811 | 1813 | |
| 1812 | − mutex_lock(&pool->manager_mutex); | |
| 1813 | − | |
| 1814 | 1814 | worker = create_worker(pool); |
| 1815 | 1815 | if (worker) { |
| 1816 | 1816 | spin_lock_irq(&pool->lock); |
| @@ -1818,8 +1818,6 @@ static int create_and_start_worker(struct worker_pool *pool) · 1 unmodified line | ||
| 1818 | 1818 | spin_unlock_irq(&pool->lock); |
| 1819 | 1819 | } |
| 1820 | 1820 | |
| 1821 | − mutex_unlock(&pool->manager_mutex); | |
| 1822 | − | |
| 1823 | 1821 | return worker ? 0 : -ENOMEM; |
| 1824 | 1822 | } |
| 1825 | 1823 | |
| @@ -2019,8 +2017,6 @@ static bool manage_workers(struct worker *worker) · 193 unmodified lines | ||
| 2019 | 2017 | bool ret = false; |
| 2020 | 2018 | |
| 2021 | 2019 | /* |
| 2022 | − * Managership is governed by two mutexes - manager_arb and | |
| 2023 | − * manager_mutex. manager_arb handles arbitration of manager role. | |
| 2024 | 2020 | * Anyone who successfully grabs manager_arb wins the arbitration |
| 2025 | 2021 | * and becomes the manager. mutex_trylock() on pool->manager_arb |
| 2026 | 2022 | * failure while holding pool->lock reliably indicates that someone |
| @@ -2029,33 +2025,12 @@ static bool manage_workers(struct worker *worker) · 2 unmodified lines | ||
| 2029 | 2025 | * grabbing manager_arb is responsible for actually performing |
| 2030 | 2026 | * manager duties. If manager_arb is grabbed and released without |
| 2031 | 2027 | * actual management, the pool may stall indefinitely. |
| 2032 | − * | |
| 2033 | − * manager_mutex is used for exclusion of actual management | |
| 2034 | − * operations. The holder of manager_mutex can be sure that none | |
| 2035 | − * of management operations, including creation and destruction of | |
| 2036 | − * workers, won't take place until the mutex is released. Because | |
| 2037 | − * manager_mutex doesn't interfere with manager role arbitration, | |
| 2038 | − * it is guaranteed that the pool's management, while may be | |
| 2039 | − * delayed, won't be disturbed by someone else grabbing | |
| 2040 | − * manager_mutex. | |
| 2041 | 2028 | */ |
| 2042 | 2029 | if (!mutex_trylock(&pool->manager_arb)) |
| 2043 | 2030 | return ret; |
| 2044 | 2031 | |
| 2045 | − /* | |
| 2046 | − * With manager arbitration won, manager_mutex would be free in | |
| 2047 | − * most cases. trylock first without dropping @pool->lock. | |
| 2048 | − */ | |
| 2049 | − if (unlikely(!mutex_trylock(&pool->manager_mutex))) { | |
| 2050 | − spin_unlock_irq(&pool->lock); | |
| 2051 | − mutex_lock(&pool->manager_mutex); | |
| 2052 | − spin_lock_irq(&pool->lock); | |
| 2053 | − ret = true; | |
| 2054 | − } | |
| 2055 | − | |
| 2056 | 2032 | ret |= maybe_create_worker(pool); |
| 2057 | 2033 | |
| 2058 | − mutex_unlock(&pool->manager_mutex); | |
| 2059 | 2034 | mutex_unlock(&pool->manager_arb); |
| 2060 | 2035 | return ret; |
| 2061 | 2036 | } |