Hello,
This is v2. Changes from v1
(http://lkml.kernel.org/r/[email protected]):
- wq->max_active now uses WRITE/READ_ONCE() as suggested by Lai.
- __queue_work() is updated to alwyas delay the work item if there already
are inactive work items on the pwq. This prevents work item reordering
inside the pwq when max_active is increased thus maintaining execution
order for ordered workqueues. This issue was noticed by Lai.
- In 0008-workqueue-Introduce-struct-wq_node_nr_active.patch, Lai pointed
out that pwq_tryinc_nr_active() incorrectly dropped pwq->max_active check.
Restored. As the next patch replaces the max_active enforcement mechanism,
this doesn't change the end result.
- 0010-workqueue-Reimplement-ordered-workqueue-using-shared.patch was broken
and could reorder work items in ordered workqueues leading to severe perf
regressions and hangs with certain workloads. Dropped.
A pool_workqueue (pwq) represents the connection between a workqueue and a
worker_pool. One of the roles that a pwq plays is enforcement of the
max_active concurrency limit. Before 636b927eba5b ("workqueue: Make unbound
workqueues to use per-cpu pool_workqueues"), there was one pwq per each CPU
for per-cpu workqueues and per each NUMA node for unbound workqueues, which
was a natural result of per-cpu workqueues being served by per-cpu pools and
unbound by per-NUMA pools.
In terms of max_active enforcement, this was, while not perfect, workable.
For per-cpu workqueues, it was fine. For unbound, it wasn't great in that
NUMA machines would get max_active that's multiplied by the number of nodes
but didn't cause huge problems because NUMA machines are relatively rare and
the node count is usually pretty low.
However, cache layouts are more complex now and sharing a worker pool across
a whole node didn't really work well for unbound workqueues. Thus, a series
of commits culminating on 8639ecebc9b1 ("workqueue: Make unbound workqueues
to use per-cpu pool_workqueues") implemented more flexible affinity
mechanism for unbound workqueues which enables using e.g. last-level-cache
aligned pools. In the process, 636b927eba5b ("workqueue: Make unbound
workqueues to use per-cpu pool_workqueues") made unbound workqueues use
per-cpu pwqs like per-cpu workqueues.
While the change was necessary to enable more flexible affinity scopes, this
came with the side effect of blowing up the effective max_active for unbound
workqueues. Before, the effective max_active for unbound workqueues was
multiplied by the number of nodes. After, by the number of CPUs.
636b927eba5b ("workqueue: Make unbound workqueues to use per-cpu
pool_workqueues") claims that this should generally be okay. It is okay for
users which self-regulates concurrency level which are the vast majority;
however, there are enough use cases which actually depend on max_active to
prevent the level of concurrency from going bonkers including several IO
handling workqueues that can issue a work item for each in-flight IO. With
targeted benchmarks, the misbehavior can easily be exposed as reported in
http://lkml.kernel.org/r/dbu6wiwu3sdhmhikb2w6lns7b27gbobfavhjj57kwi2quafgwl@htjcc5oikcr3.
Unfortunately, there is no way to express what these use cases need using
per-cpu max_active. A CPU may issue most of in-flight IOs, so we don't want
to set max_active too low but as soon as we increase max_active a bit, we
can end up with unreasonable number of in-flight work items when many CPUs
issue IOs at the same time. ie. The acceptable lowest max_active is higher
than the acceptable highest max_active.
Ideally, max_active for an unbound workqueue should be system-wide so that
the users can regulate the total level of concurrency regardless of node and
cache layout. The reasons workqueue hasn't implemented that yet are:
- One max_active enforcement decouples from pool boundaires, chaining
execution after a work item finishes requires inter-pool operations which
would require lock dancing, which is nasty.
- Sharing a single nr_active count across the whole system can be pretty
expensive on NUMA machines.
- Per-pwq enforcement had been more or less okay while we were using
per-node pools.
It looks like we no longer can avoid decoupling max_active enforcement from
pool boundaries. This patchset implements system-wide nr_active mechanism
with the following design characteristics:
- To avoid sharing a single counter across multiple nodes, the configured
max_active is split across nodes according to the proportion of online
CPUs per node. e.g. A node with twice more online CPUs will get twice
higher portion of max_active.
- Workqueue used to be able to process a chain of interdependent work items
which is as long as max_active. We can't do this anymore as max_active is
distributed across the nodes. Instead, a new parameter min_active is
introduced which determines the minimum level of concurrency within a node
regardless of how max_active distribution comes out to be.
It is set to the smaller of max_active and WQ_DFL_MIN_ACTIVE which is 8.
This can lead to higher effective max_weight than configured and also
deadlocks if a workqueue was depending on being able to handle chains of
interdependent work items that are longer than 8.
I believe these should be fine given that the number of CPUs in each NUMA
node is usually higher than 8 and work item chain longer than 8 is pretty
unlikely. However, if these assumptions turn out to be wrong, we'll need
to add an interface to adjust min_active.
- Each unbound wq has an array of struct wq_node_nr_active which tracks
per-node nr_active. When its pwq wants to run a work item, it has to
obtain the matching node's nr_active. If over the node's max_active, the
pwq is queued on wq_node_nr_active->pending_pwqs. As work items finish,
the completion path round-robins the pending pwqs activating the first
inactive work item of each, which involves some pool lock dancing and
kicking other pools. It's not the simplest code but doesn't look too bad.
This patchset includes the following patches:
0001-workqueue-Move-pwq-max_active-to-wq-max_active.patch
0002-workqueue-Factor-out-pwq_is_empty.patch
0003-workqueue-Replace-pwq_activate_inactive_work-with-__.patch
0004-workqueue-Move-nr_active-handling-into-helpers.patch
0005-workqueue-Make-wq_adjust_max_active-round-robin-pwqs.patch
0006-workqueue-Add-first_possible_node-and-node_nr_cpus.patch
0007-workqueue-Move-pwq_dec_nr_in_flight-to-the-end-of-wo.patch
0008-workqueue-Introduce-struct-wq_node_nr_active.patch
0009-workqueue-Implement-system-wide-nr_active-enforcemen.patch
This pachset is also available in the following git branch.
https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git unbound-system-wide-max_active-v2
diffstat follows.
include/linux/workqueue.h | 35 ++
kernel/workqueue.c | 644 ++++++++++++++++++++++++++++++++++++++--------
2 files changed, 576 insertions(+), 103 deletions(-)
--
tejun
"!pwq->nr_active && list_empty(&pwq->inactive_works)" test is repeated
multiple times. Let's factor it out into pwq_is_empty().
Signed-off-by: Tejun Heo <[email protected]>
---
kernel/workqueue.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 8bf1d143e911..09f0ec086726 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1456,6 +1456,11 @@ static void put_pwq_unlocked(struct pool_workqueue *pwq)
}
}
+static bool pwq_is_empty(struct pool_workqueue *pwq)
+{
+ return !pwq->nr_active && list_empty(&pwq->inactive_works);
+}
+
static void pwq_activate_inactive_work(struct work_struct *work)
{
struct pool_workqueue *pwq = get_work_pwq(work);
@@ -3325,7 +3330,7 @@ void drain_workqueue(struct workqueue_struct *wq)
bool drained;
raw_spin_lock_irq(&pwq->pool->lock);
- drained = !pwq->nr_active && list_empty(&pwq->inactive_works);
+ drained = pwq_is_empty(pwq);
raw_spin_unlock_irq(&pwq->pool->lock);
if (drained)
@@ -4768,7 +4773,7 @@ static bool pwq_busy(struct pool_workqueue *pwq)
if ((pwq != pwq->wq->dfl_pwq) && (pwq->refcnt > 1))
return true;
- if (pwq->nr_active || !list_empty(&pwq->inactive_works))
+ if (!pwq_is_empty(pwq))
return true;
return false;
@@ -5205,7 +5210,7 @@ void show_one_workqueue(struct workqueue_struct *wq)
unsigned long flags;
for_each_pwq(pwq, wq) {
- if (pwq->nr_active || !list_empty(&pwq->inactive_works)) {
+ if (!pwq_is_empty(pwq)) {
idle = false;
break;
}
@@ -5217,7 +5222,7 @@ void show_one_workqueue(struct workqueue_struct *wq)
for_each_pwq(pwq, wq) {
raw_spin_lock_irqsave(&pwq->pool->lock, flags);
- if (pwq->nr_active || !list_empty(&pwq->inactive_works)) {
+ if (!pwq_is_empty(pwq)) {
/*
* Defer printing to avoid deadlocks in console
* drivers that queue work while holding locks
--
2.43.0
max_active is a workqueue-wide setting and the configured value is stored in
wq->saved_max_active; however, the effective value was stored in
pwq->max_active. While this is harmless, it makes max_active update process
more complicated and gets in the way of the planned max_active semantic
updates for unbound workqueues.
This patches moves pwq->max_active to wq->max_active. This simplifies the
code and makes freezing and noop max_active updates cheaper too. No
user-visible behavior change is intended.
As wq->max_active is updated while holding wq mutex but read without any
locking, it now uses WRITE/READ_ONCE(). A new locking locking rule WO is
added for it.
v2: - wq->max_active now uses WRITE/READ_ONCE() as suggested by Lai.
- __queue_work() is updated to alwyas delay the work item if there
already are inactive work items on the pwq. This prevents work item
reordering inside the pwq when max_active is increased thus
maintaining execution order for ordered workqueues. This issue was
noticed by Lai.
Signed-off-by: Tejun Heo <[email protected]>
Cc: Lai Jiangshan <[email protected]>
---
kernel/workqueue.c | 134 ++++++++++++++++++++++-----------------------
1 file changed, 66 insertions(+), 68 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 76e60faed892..8bf1d143e911 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -143,6 +143,9 @@ enum {
*
* WR: wq->mutex protected for writes. RCU protected for reads.
*
+ * WO: wq->mutex protected for writes. Updated with WRITE_ONCE() and can be read
+ * with READ_ONCE() without locking.
+ *
* MD: wq_mayday_lock protected.
*
* WD: Used internally by the watchdog.
@@ -250,7 +253,6 @@ struct pool_workqueue {
* is marked with WORK_STRUCT_INACTIVE iff it is in pwq->inactive_works.
*/
int nr_active; /* L: nr of active works */
- int max_active; /* L: max active works */
struct list_head inactive_works; /* L: inactive works */
struct list_head pwqs_node; /* WR: node on wq->pwqs */
struct list_head mayday_node; /* MD: node on wq->maydays */
@@ -298,7 +300,8 @@ struct workqueue_struct {
struct worker *rescuer; /* MD: rescue worker */
int nr_drainers; /* WQ: drain in progress */
- int saved_max_active; /* WQ: saved pwq max_active */
+ int max_active; /* WO: max active works */
+ int saved_max_active; /* WQ: saved max_active */
struct workqueue_attrs *unbound_attrs; /* PW: only for unbound wqs */
struct pool_workqueue *dfl_pwq; /* PW: only for unbound wqs */
@@ -1492,7 +1495,7 @@ static void pwq_dec_nr_in_flight(struct pool_workqueue *pwq, unsigned long work_
pwq->nr_active--;
if (!list_empty(&pwq->inactive_works)) {
/* one down, submit an inactive one */
- if (pwq->nr_active < pwq->max_active)
+ if (pwq->nr_active < READ_ONCE(pwq->wq->max_active))
pwq_activate_first_inactive(pwq);
}
}
@@ -1793,7 +1796,13 @@ static void __queue_work(int cpu, struct workqueue_struct *wq,
pwq->nr_in_flight[pwq->work_color]++;
work_flags = work_color_to_flags(pwq->work_color);
- if (likely(pwq->nr_active < pwq->max_active)) {
+ /*
+ * Limit the number of concurrently active work items to max_active.
+ * @work must also queue behind existing inactive work items to maintain
+ * ordering when max_active changes. See wq_adjust_max_active().
+ */
+ if (list_empty(&pwq->inactive_works) &&
+ pwq->nr_active < READ_ONCE(pwq->wq->max_active)) {
if (list_empty(&pool->worklist))
pool->watchdog_ts = jiffies;
@@ -4142,50 +4151,6 @@ static void pwq_release_workfn(struct kthread_work *work)
}
}
-/**
- * pwq_adjust_max_active - update a pwq's max_active to the current setting
- * @pwq: target pool_workqueue
- *
- * If @pwq isn't freezing, set @pwq->max_active to the associated
- * workqueue's saved_max_active and activate inactive work items
- * accordingly. If @pwq is freezing, clear @pwq->max_active to zero.
- */
-static void pwq_adjust_max_active(struct pool_workqueue *pwq)
-{
- struct workqueue_struct *wq = pwq->wq;
- bool freezable = wq->flags & WQ_FREEZABLE;
- unsigned long flags;
-
- /* for @wq->saved_max_active */
- lockdep_assert_held(&wq->mutex);
-
- /* fast exit for non-freezable wqs */
- if (!freezable && pwq->max_active == wq->saved_max_active)
- return;
-
- /* this function can be called during early boot w/ irq disabled */
- raw_spin_lock_irqsave(&pwq->pool->lock, flags);
-
- /*
- * During [un]freezing, the caller is responsible for ensuring that
- * this function is called at least once after @workqueue_freezing
- * is updated and visible.
- */
- if (!freezable || !workqueue_freezing) {
- pwq->max_active = wq->saved_max_active;
-
- while (!list_empty(&pwq->inactive_works) &&
- pwq->nr_active < pwq->max_active)
- pwq_activate_first_inactive(pwq);
-
- kick_pool(pwq->pool);
- } else {
- pwq->max_active = 0;
- }
-
- raw_spin_unlock_irqrestore(&pwq->pool->lock, flags);
-}
-
/* initialize newly allocated @pwq which is associated with @wq and @pool */
static void init_pwq(struct pool_workqueue *pwq, struct workqueue_struct *wq,
struct worker_pool *pool)
@@ -4218,9 +4183,6 @@ static void link_pwq(struct pool_workqueue *pwq)
/* set the matching work_color */
pwq->work_color = wq->work_color;
- /* sync max_active to the current setting */
- pwq_adjust_max_active(pwq);
-
/* link in @pwq */
list_add_rcu(&pwq->pwqs_node, &wq->pwqs);
}
@@ -4658,6 +4620,52 @@ static int init_rescuer(struct workqueue_struct *wq)
return 0;
}
+/**
+ * wq_adjust_max_active - update a wq's max_active to the current setting
+ * @wq: target workqueue
+ *
+ * If @wq isn't freezing, set @wq->max_active to the saved_max_active and
+ * activate inactive work items accordingly. If @wq is freezing, clear
+ * @wq->max_active to zero.
+ */
+static void wq_adjust_max_active(struct workqueue_struct *wq)
+{
+ struct pool_workqueue *pwq;
+
+ lockdep_assert_held(&wq->mutex);
+
+ if ((wq->flags & WQ_FREEZABLE) && workqueue_freezing) {
+ WRITE_ONCE(wq->max_active, 0);
+ return;
+ }
+
+ if (wq->max_active == wq->saved_max_active)
+ return;
+
+ /*
+ * Update @wq->max_active and then kick inactive work items if more
+ * active work items are allowed. This doesn't break work item ordering
+ * because new work items are always queued behind existing inactive
+ * work items if there are any.
+ */
+ WRITE_ONCE(wq->max_active, wq->saved_max_active);
+
+ for_each_pwq(pwq, wq) {
+ unsigned long flags;
+
+ /* this function can be called during early boot w/ irq disabled */
+ raw_spin_lock_irqsave(&pwq->pool->lock, flags);
+
+ while (!list_empty(&pwq->inactive_works) &&
+ pwq->nr_active < wq->max_active)
+ pwq_activate_first_inactive(pwq);
+
+ kick_pool(pwq->pool);
+
+ raw_spin_unlock_irqrestore(&pwq->pool->lock, flags);
+ }
+}
+
__printf(1, 4)
struct workqueue_struct *alloc_workqueue(const char *fmt,
unsigned int flags,
@@ -4665,7 +4673,6 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
{
va_list args;
struct workqueue_struct *wq;
- struct pool_workqueue *pwq;
/*
* Unbound && max_active == 1 used to imply ordered, which is no longer
@@ -4700,6 +4707,7 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
/* init wq */
wq->flags = flags;
+ wq->max_active = max_active;
wq->saved_max_active = max_active;
mutex_init(&wq->mutex);
atomic_set(&wq->nr_pwqs_to_flush, 0);
@@ -4728,8 +4736,7 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
mutex_lock(&wq_pool_mutex);
mutex_lock(&wq->mutex);
- for_each_pwq(pwq, wq)
- pwq_adjust_max_active(pwq);
+ wq_adjust_max_active(wq);
mutex_unlock(&wq->mutex);
list_add_tail_rcu(&wq->list, &workqueues);
@@ -4867,8 +4874,6 @@ EXPORT_SYMBOL_GPL(destroy_workqueue);
*/
void workqueue_set_max_active(struct workqueue_struct *wq, int max_active)
{
- struct pool_workqueue *pwq;
-
/* disallow meddling with max_active for ordered workqueues */
if (WARN_ON(wq->flags & __WQ_ORDERED_EXPLICIT))
return;
@@ -4879,10 +4884,7 @@ void workqueue_set_max_active(struct workqueue_struct *wq, int max_active)
wq->flags &= ~__WQ_ORDERED;
wq->saved_max_active = max_active;
-
- for_each_pwq(pwq, wq)
- pwq_adjust_max_active(pwq);
-
+ wq_adjust_max_active(wq);
mutex_unlock(&wq->mutex);
}
EXPORT_SYMBOL_GPL(workqueue_set_max_active);
@@ -5128,8 +5130,8 @@ static void show_pwq(struct pool_workqueue *pwq)
pr_info(" pwq %d:", pool->id);
pr_cont_pool_info(pool);
- pr_cont(" active=%d/%d refcnt=%d%s\n",
- pwq->nr_active, pwq->max_active, pwq->refcnt,
+ pr_cont(" active=%d refcnt=%d%s\n",
+ pwq->nr_active, pwq->refcnt,
!list_empty(&pwq->mayday_node) ? " MAYDAY" : "");
hash_for_each(pool->busy_hash, bkt, worker, hentry) {
@@ -5677,7 +5679,6 @@ EXPORT_SYMBOL_GPL(work_on_cpu_safe_key);
void freeze_workqueues_begin(void)
{
struct workqueue_struct *wq;
- struct pool_workqueue *pwq;
mutex_lock(&wq_pool_mutex);
@@ -5686,8 +5687,7 @@ void freeze_workqueues_begin(void)
list_for_each_entry(wq, &workqueues, list) {
mutex_lock(&wq->mutex);
- for_each_pwq(pwq, wq)
- pwq_adjust_max_active(pwq);
+ wq_adjust_max_active(wq);
mutex_unlock(&wq->mutex);
}
@@ -5752,7 +5752,6 @@ bool freeze_workqueues_busy(void)
void thaw_workqueues(void)
{
struct workqueue_struct *wq;
- struct pool_workqueue *pwq;
mutex_lock(&wq_pool_mutex);
@@ -5764,8 +5763,7 @@ void thaw_workqueues(void)
/* restore max_active and repopulate worklist */
list_for_each_entry(wq, &workqueues, list) {
mutex_lock(&wq->mutex);
- for_each_pwq(pwq, wq)
- pwq_adjust_max_active(pwq);
+ wq_adjust_max_active(wq);
mutex_unlock(&wq->mutex);
}
--
2.43.0
__queue_work(), pwq_dec_nr_in_flight() and wq_adjust_max_active() were
open-coding nr_active handling, which is fine given that the operations are
trivial. However, the planned unbound nr_active update will make them more
complicated, so let's move them into helpers.
- pwq_tryinc_nr_active() is added. It increments nr_active if under
max_active limit and return a boolean indicating whether inc was
successful. Note that the function is structured to accommodate future
changes. __queue_work() is updated to use the new helper.
- pwq_activate_first_inactive() is updated to use pwq_tryinc_nr_active() and
thus no longer assumes that nr_active is under max_active and returns a
boolean to indicate whether a work item has been activated.
- wq_adjust_max_active() no longer tests directly whether a work item can be
activated. Instead, it's updated to use the return value of
pwq_activate_first_inactive() to tell whether a work item has been
activated.
- nr_active decrement and activating the first inactive work item is
factored into pwq_dec_nr_active().
v2: wq->max_active now uses WRITE/READ_ONCE() as suggested by Lai.
Signed-off-by: Tejun Heo <[email protected]>
Cc: Lai Jiangshan <[email protected]>
---
kernel/workqueue.c | 81 +++++++++++++++++++++++++++++++++++-----------
1 file changed, 63 insertions(+), 18 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 2cd275b34df2..7eb1925087fd 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1493,12 +1493,66 @@ static bool pwq_activate_work(struct pool_workqueue *pwq,
return true;
}
-static void pwq_activate_first_inactive(struct pool_workqueue *pwq)
+/**
+ * pwq_tryinc_nr_active - Try to increment nr_active for a pwq
+ * @pwq: pool_workqueue of interest
+ *
+ * Try to increment nr_active for @pwq. Returns %true if an nr_active count is
+ * successfully obtained. %false otherwise.
+ */
+static bool pwq_tryinc_nr_active(struct pool_workqueue *pwq)
{
- struct work_struct *work = list_first_entry(&pwq->inactive_works,
- struct work_struct, entry);
+ struct workqueue_struct *wq = pwq->wq;
+ struct worker_pool *pool = pwq->pool;
+ bool obtained;
- pwq_activate_work(pwq, work);
+ lockdep_assert_held(&pool->lock);
+
+ obtained = pwq->nr_active < READ_ONCE(wq->max_active);
+
+ if (obtained)
+ pwq->nr_active++;
+ return obtained;
+}
+
+/**
+ * pwq_activate_first_inactive - Activate the first inactive work item on a pwq
+ * @pwq: pool_workqueue of interest
+ *
+ * Activate the first inactive work item of @pwq if available and allowed by
+ * max_active limit.
+ *
+ * Returns %true if an inactive work item has been activated. %false if no
+ * inactive work item is found or max_active limit is reached.
+ */
+static bool pwq_activate_first_inactive(struct pool_workqueue *pwq)
+{
+ struct work_struct *work =
+ list_first_entry_or_null(&pwq->inactive_works,
+ struct work_struct, entry);
+
+ if (work && pwq_tryinc_nr_active(pwq)) {
+ __pwq_activate_work(pwq, work);
+ return true;
+ } else {
+ return false;
+ }
+}
+
+/**
+ * pwq_dec_nr_active - Retire an active count
+ * @pwq: pool_workqueue of interest
+ *
+ * Decrement @pwq's nr_active and try to activate the first inactive work item.
+ */
+static void pwq_dec_nr_active(struct pool_workqueue *pwq)
+{
+ struct worker_pool *pool = pwq->pool;
+
+ lockdep_assert_held(&pool->lock);
+
+ pwq->nr_active--;
+ pwq_activate_first_inactive(pwq);
}
/**
@@ -1516,14 +1570,8 @@ static void pwq_dec_nr_in_flight(struct pool_workqueue *pwq, unsigned long work_
{
int color = get_work_color(work_data);
- if (!(work_data & WORK_STRUCT_INACTIVE)) {
- pwq->nr_active--;
- if (!list_empty(&pwq->inactive_works)) {
- /* one down, submit an inactive one */
- if (pwq->nr_active < READ_ONCE(pwq->wq->max_active))
- pwq_activate_first_inactive(pwq);
- }
- }
+ if (!(work_data & WORK_STRUCT_INACTIVE))
+ pwq_dec_nr_active(pwq);
pwq->nr_in_flight[color]--;
@@ -1825,13 +1873,11 @@ static void __queue_work(int cpu, struct workqueue_struct *wq,
* @work must also queue behind existing inactive work items to maintain
* ordering when max_active changes. See wq_adjust_max_active().
*/
- if (list_empty(&pwq->inactive_works) &&
- pwq->nr_active < READ_ONCE(pwq->wq->max_active)) {
+ if (list_empty(&pwq->inactive_works) && pwq_tryinc_nr_active(pwq)) {
if (list_empty(&pool->worklist))
pool->watchdog_ts = jiffies;
trace_workqueue_activate_work(work);
- pwq->nr_active++;
insert_work(pwq, work, &pool->worklist, work_flags);
kick_pool(pool);
} else {
@@ -4680,9 +4726,8 @@ static void wq_adjust_max_active(struct workqueue_struct *wq)
/* this function can be called during early boot w/ irq disabled */
raw_spin_lock_irqsave(&pwq->pool->lock, flags);
- while (!list_empty(&pwq->inactive_works) &&
- pwq->nr_active < wq->max_active)
- pwq_activate_first_inactive(pwq);
+ while (pwq_activate_first_inactive(pwq))
+ ;
kick_pool(pwq->pool);
--
2.43.0
The planned shared nr_active handling for unbound workqueues will make
pwq_dec_nr_active() sometimes drop the pool lock temporarily to acquire
other pool locks, which is necessary as retirement of an nr_active count
from one pool may need kick off an inactive work item in another pool.
This patch moves pwq_dec_nr_in_flight() call in try_to_grab_pending() to the
end of work item handling so that work item state changes stay atomic.
process_one_work() which is the other user of pwq_dec_nr_in_flight() already
calls it at the end of work item handling. Comments are added to both call
sites and pwq_dec_nr_in_flight().
This shouldn't cause any behavior changes.
Signed-off-by: Tejun Heo <[email protected]>
---
kernel/workqueue.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 3f45baa96d51..522ef63088de 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1566,6 +1566,11 @@ static void pwq_dec_nr_active(struct pool_workqueue *pwq)
* A work either has completed or is removed from pending queue,
* decrement nr_in_flight of its pwq and handle workqueue flushing.
*
+ * NOTE:
+ * For unbound workqueues, this function may temporarily drop @pwq->pool->lock
+ * and thus should be called after all other state updates for the in-flight
+ * work item is complete.
+ *
* CONTEXT:
* raw_spin_lock_irq(pool->lock).
*/
@@ -1690,11 +1695,13 @@ static int try_to_grab_pending(struct work_struct *work, bool is_dwork,
pwq_activate_work(pwq, work);
list_del_init(&work->entry);
- pwq_dec_nr_in_flight(pwq, *work_data_bits(work));
/* work->data points to pwq iff queued, point to pool */
set_work_pool_and_keep_pending(work, pool->id);
+ /* must be the last step, see the function comment */
+ pwq_dec_nr_in_flight(pwq, *work_data_bits(work));
+
raw_spin_unlock(&pool->lock);
rcu_read_unlock();
return 1;
@@ -2759,6 +2766,8 @@ __acquires(&pool->lock)
worker->current_func = NULL;
worker->current_pwq = NULL;
worker->current_color = INT_MAX;
+
+ /* must be the last step, see the function comment */
pwq_dec_nr_in_flight(pwq, work_data);
}
--
2.43.0
Currently, for both percpu and unbound workqueues, max_active applies
per-cpu, which is a recent change for unbound workqueues. The change for
unbound workqueues was a significant departure from the previous behavior of
per-node application. It made some use cases create undesirable number of
concurrent work items and left no good way of fixing them. To address the
problem, workqueue is implementing a NUMA node segmented global nr_active
mechanism, which will be explained further in the next patch.
As a preparation, this patch introduces struct wq_node_nr_active. It's a
data structured allocated for each workqueue and NUMA node pair and
currently only tracks the workqueue's number of active work items on the
node. This is split out from the next patch to make it easier to understand
and review.
Note that there is an extra wq_node_nr_active allocated for the invalid node
nr_node_ids which is used to track nr_active for pools which don't have NUMA
node associated such as the default fallback system-wide pool.
This doesn't cause any behavior changes visible to userland yet. The next
patch will expand to implement the control mechanism on top.
v2: - wq->max_active now uses WRITE/READ_ONCE() as suggested by Lai.
- Lai pointed out that pwq_tryinc_nr_active() incorrectly dropped
pwq->max_active check. Restored. As the next patch replaces the
max_active enforcement mechanism, this doesn't change the end result.
Signed-off-by: Tejun Heo <[email protected]>
Cc: Lai Jiangshan <[email protected]>
---
kernel/workqueue.c | 125 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 123 insertions(+), 2 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 522ef63088de..71ac3a3633ac 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -281,6 +281,16 @@ struct wq_flusher {
struct wq_device;
+/*
+ * Unlike in a per-cpu workqueue where max_active limits its concurrency level
+ * on each CPU, in an unbound workqueue, max_active applies to the whole system.
+ * As sharing a single nr_active across multiple sockets can be very expensive,
+ * the counting and enforcement is per NUMA node.
+ */
+struct wq_node_nr_active {
+ atomic_t count; /* per-node nr_active count */
+};
+
/*
* The externally visible workqueue. It relays the issued work items to
* the appropriate worker_pool through its pool_workqueues.
@@ -327,6 +337,7 @@ struct workqueue_struct {
/* hot fields used during command issue, aligned to cacheline */
unsigned int flags ____cacheline_aligned; /* WQ: WQ_* flags */
struct pool_workqueue __percpu __rcu **cpu_pwq; /* I: per-cpu pwqs */
+ struct wq_node_nr_active **node_nr_active; /* I: per-node nr_active */
};
static struct kmem_cache *pwq_cache;
@@ -1407,6 +1418,31 @@ work_func_t wq_worker_last_func(struct task_struct *task)
return worker->last_func;
}
+/**
+ * wq_node_nr_active - Determine wq_node_nr_active to use
+ * @wq: workqueue of interest
+ * @node: NUMA node, can be %NUMA_NO_NODE
+ *
+ * Determine wq_node_nr_active to use for @wq on @node. Returns:
+ *
+ * - %NULL for per-cpu workqueues as they don't need to use shared nr_active.
+ *
+ * - node_nr_active[nr_node_ids] if @node is %NUMA_NO_NODE.
+ *
+ * - Otherwise, node_nr_active[@node].
+ */
+static struct wq_node_nr_active *wq_node_nr_active(struct workqueue_struct *wq,
+ int node)
+{
+ if (!(wq->flags & WQ_UNBOUND))
+ return NULL;
+
+ if (node == NUMA_NO_NODE)
+ node = nr_node_ids;
+
+ return wq->node_nr_active[node];
+}
+
/**
* get_pwq - get an extra reference on the specified pool_workqueue
* @pwq: pool_workqueue to get
@@ -1485,12 +1521,17 @@ static bool pwq_activate_work(struct pool_workqueue *pwq,
struct work_struct *work)
{
struct worker_pool *pool = pwq->pool;
+ struct wq_node_nr_active *nna;
lockdep_assert_held(&pool->lock);
if (!(*work_data_bits(work) & WORK_STRUCT_INACTIVE))
return false;
+ nna = wq_node_nr_active(pwq->wq, pool->node);
+ if (nna)
+ atomic_inc(&nna->count);
+
pwq->nr_active++;
__pwq_activate_work(pwq, work);
return true;
@@ -1507,14 +1548,18 @@ static bool pwq_tryinc_nr_active(struct pool_workqueue *pwq)
{
struct workqueue_struct *wq = pwq->wq;
struct worker_pool *pool = pwq->pool;
+ struct wq_node_nr_active *nna = wq_node_nr_active(wq, pool->node);
bool obtained;
lockdep_assert_held(&pool->lock);
obtained = pwq->nr_active < READ_ONCE(wq->max_active);
- if (obtained)
+ if (obtained) {
pwq->nr_active++;
+ if (nna)
+ atomic_inc(&nna->count);
+ }
return obtained;
}
@@ -1551,10 +1596,26 @@ static bool pwq_activate_first_inactive(struct pool_workqueue *pwq)
static void pwq_dec_nr_active(struct pool_workqueue *pwq)
{
struct worker_pool *pool = pwq->pool;
+ struct wq_node_nr_active *nna = wq_node_nr_active(pwq->wq, pool->node);
lockdep_assert_held(&pool->lock);
+ /*
+ * @pwq->nr_active should be decremented for both percpu and unbound
+ * workqueues.
+ */
pwq->nr_active--;
+
+ /*
+ * For a percpu workqueue, it's simple. Just need to kick the first
+ * inactive work item on @pwq itself.
+ */
+ if (!nna) {
+ pwq_activate_first_inactive(pwq);
+ return;
+ }
+
+ atomic_dec(&nna->count);
pwq_activate_first_inactive(pwq);
}
@@ -4018,12 +4079,64 @@ static void wq_free_lockdep(struct workqueue_struct *wq)
}
#endif
+static void free_node_nr_active(struct wq_node_nr_active **ptr_ar)
+{
+ int i;
+
+ if (!ptr_ar)
+ return;
+ for (i = 0; i < nr_node_ids + 1; i++)
+ kfree(ptr_ar[i]);
+ kfree(ptr_ar);
+}
+
+static void init_node_nr_active(struct wq_node_nr_active *nna)
+{
+ atomic_set(&nna->count, 0);
+}
+
+/*
+ * Each node's nr_active counter will be accessed mostly from its own node and
+ * should be allocated in the node.
+ */
+static struct wq_node_nr_active **alloc_node_nr_active(void)
+{
+ struct wq_node_nr_active **nna_ar, *nna;
+ int node;
+
+ nna_ar = kcalloc(nr_node_ids + 1, sizeof(*nna_ar), GFP_KERNEL);
+ if (!nna_ar)
+ return NULL;
+
+ for_each_node(node) {
+ nna = kzalloc_node(sizeof(*nna), GFP_KERNEL, node);
+ if (!nna)
+ goto err_free;
+ init_node_nr_active(nna);
+ nna_ar[node] = nna;
+ }
+
+ /* [nr_node_ids] is used as the fallback */
+ nna = kzalloc_node(sizeof(*nna), GFP_KERNEL, NUMA_NO_NODE);
+ if (!nna)
+ goto err_free;
+ init_node_nr_active(nna);
+ nna_ar[nr_node_ids] = nna;
+
+ return nna_ar;
+
+err_free:
+ free_node_nr_active(nna_ar);
+ return NULL;
+}
+
static void rcu_free_wq(struct rcu_head *rcu)
{
struct workqueue_struct *wq =
container_of(rcu, struct workqueue_struct, rcu);
wq_free_lockdep(wq);
+ free_node_nr_active(wq->node_nr_active);
free_percpu(wq->cpu_pwq);
free_workqueue_attrs(wq->unbound_attrs);
kfree(wq);
@@ -4807,8 +4920,14 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
wq_init_lockdep(wq);
INIT_LIST_HEAD(&wq->list);
+ if (flags & WQ_UNBOUND) {
+ wq->node_nr_active = alloc_node_nr_active();
+ if (!wq->node_nr_active)
+ goto err_unreg_lockdep;
+ }
+
if (alloc_and_link_pwqs(wq) < 0)
- goto err_unreg_lockdep;
+ goto err_free_node_nr_active;
if (wq_online && init_rescuer(wq) < 0)
goto err_destroy;
@@ -4833,6 +4952,8 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
return wq;
+err_free_node_nr_active:
+ free_node_nr_active(wq->node_nr_active);
err_unreg_lockdep:
wq_unregister_lockdep(wq);
wq_free_lockdep(wq);
--
2.43.0
A pool_workqueue (pwq) represents the connection between a workqueue and a
worker_pool. One of the roles that a pwq plays is enforcement of the
max_active concurrency limit. Before 636b927eba5b ("workqueue: Make unbound
workqueues to use per-cpu pool_workqueues"), there was one pwq per each CPU
for per-cpu workqueues and per each NUMA node for unbound workqueues, which
was a natural result of per-cpu workqueues being served by per-cpu pools and
unbound by per-NUMA pools.
In terms of max_active enforcement, this was, while not perfect, workable.
For per-cpu workqueues, it was fine. For unbound, it wasn't great in that
NUMA machines would get max_active that's multiplied by the number of nodes
but didn't cause huge problems because NUMA machines are relatively rare and
the node count is usually pretty low.
However, cache layouts are more complex now and sharing a worker pool across
a whole node didn't really work well for unbound workqueues. Thus, a series
of commits culminating on 8639ecebc9b1 ("workqueue: Make unbound workqueues
to use per-cpu pool_workqueues") implemented more flexible affinity
mechanism for unbound workqueues which enables using e.g. last-level-cache
aligned pools. In the process, 636b927eba5b ("workqueue: Make unbound
workqueues to use per-cpu pool_workqueues") made unbound workqueues use
per-cpu pwqs like per-cpu workqueues.
While the change was necessary to enable more flexible affinity scopes, this
came with the side effect of blowing up the effective max_active for unbound
workqueues. Before, the effective max_active for unbound workqueues was
multiplied by the number of nodes. After, by the number of CPUs.
636b927eba5b ("workqueue: Make unbound workqueues to use per-cpu
pool_workqueues") claims that this should generally be okay. It is okay for
users which self-regulates concurrency level which are the vast majority;
however, there are enough use cases which actually depend on max_active to
prevent the level of concurrency from going bonkers including several IO
handling workqueues that can issue a work item for each in-flight IO. With
targeted benchmarks, the misbehavior can easily be exposed as reported in
http://lkml.kernel.org/r/dbu6wiwu3sdhmhikb2w6lns7b27gbobfavhjj57kwi2quafgwl@htjcc5oikcr3.
Unfortunately, there is no way to express what these use cases need using
per-cpu max_active. A CPU may issue most of in-flight IOs, so we don't want
to set max_active too low but as soon as we increase max_active a bit, we
can end up with unreasonable number of in-flight work items when many CPUs
issue IOs at the same time. ie. The acceptable lowest max_active is higher
than the acceptable highest max_active.
Ideally, max_active for an unbound workqueue should be system-wide so that
the users can regulate the total level of concurrency regardless of node and
cache layout. The reasons workqueue hasn't implemented that yet are:
- One max_active enforcement decouples from pool boundaires, chaining
execution after a work item finishes requires inter-pool operations which
would require lock dancing, which is nasty.
- Sharing a single nr_active count across the whole system can be pretty
expensive on NUMA machines.
- Per-pwq enforcement had been more or less okay while we were using
per-node pools.
It looks like we no longer can avoid decoupling max_active enforcement from
pool boundaries. This patch implements system-wide nr_active mechanism with
the following design characteristics:
- To avoid sharing a single counter across multiple nodes, the configured
max_active is split across nodes according to the proportion of online
CPUs per node. e.g. A node with twice more online CPUs will get twice
higher portion of max_active.
- Workqueue used to be able to process a chain of interdependent work items
which is as long as max_active. We can't do this anymore as max_active is
distributed across the nodes. Instead, a new parameter min_active is
introduced which determines the minimum level of concurrency within a node
regardless of how max_active distribution comes out to be.
It is set to the smaller of max_active and WQ_DFL_MIN_ACTIVE which is 8.
This can lead to higher effective max_weight than configured and also
deadlocks if a workqueue was depending on being able to handle chains of
interdependent work items that are longer than 8.
I believe these should be fine given that the number of CPUs in each NUMA
node is usually higher than 8 and work item chain longer than 8 is pretty
unlikely. However, if these assumptions turn out to be wrong, we'll need
to add an interface to adjust min_active.
- Each unbound wq has an array of struct wq_node_nr_active which tracks
per-node nr_active. When its pwq wants to run a work item, it has to
obtain the matching node's nr_active. If over the node's max_active, the
pwq is queued on wq_node_nr_active->pending_pwqs. As work items finish,
the completion path round-robins the pending pwqs activating the first
inactive work item of each, which involves some pool lock dancing and
kicking other pools. It's not the simplest code but doesn't look too bad.
v2: wq->min/max_active now uses WRITE/READ_ONCE() as suggested by Lai.
Signed-off-by: Tejun Heo <[email protected]>
Reported-by: Naohiro Aota <[email protected]>
Link: http://lkml.kernel.org/r/dbu6wiwu3sdhmhikb2w6lns7b27gbobfavhjj57kwi2quafgwl@htjcc5oikcr3
Fixes: 636b927eba5b ("workqueue: Make unbound workqueues to use per-cpu pool_workqueues")
Cc: Lai Jiangshan <[email protected]>
---
include/linux/workqueue.h | 35 ++++-
kernel/workqueue.c | 284 ++++++++++++++++++++++++++++++++++----
2 files changed, 289 insertions(+), 30 deletions(-)
diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 2cc0a9606175..515e7958c6c1 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -391,6 +391,13 @@ enum {
WQ_MAX_ACTIVE = 512, /* I like 512, better ideas? */
WQ_UNBOUND_MAX_ACTIVE = WQ_MAX_ACTIVE,
WQ_DFL_ACTIVE = WQ_MAX_ACTIVE / 2,
+
+ /*
+ * Per-node default cap on min_active. Unless explicitly set, min_active
+ * is set to min(max_active, WQ_DFL_MIN_ACTIVE). For more details, see
+ * workqueue_struct->min_active definition.
+ */
+ WQ_DFL_MIN_ACTIVE = 8,
};
/*
@@ -433,11 +440,33 @@ extern struct workqueue_struct *system_freezable_power_efficient_wq;
* alloc_workqueue - allocate a workqueue
* @fmt: printf format for the name of the workqueue
* @flags: WQ_* flags
- * @max_active: max in-flight work items per CPU, 0 for default
+ * @max_active: max in-flight work items, 0 for default
* remaining args: args for @fmt
*
- * Allocate a workqueue with the specified parameters. For detailed
- * information on WQ_* flags, please refer to
+ * For a per-cpu workqueue, @max_active limits the number of in-flight work
+ * items for each CPU. e.g. @max_active of 1 indicates that each CPU can be
+ * executing at most one work item for the workqueue.
+ *
+ * For unbound workqueues, @max_active limits the number of in-flight work items
+ * for the whole system. e.g. @max_active of 16 indicates that that there can be
+ * at most 16 work items executing for the workqueue in the whole system.
+ *
+ * As sharing the same active counter for an unbound workqueue across multiple
+ * NUMA nodes can be expensive, @max_active is distributed to each NUMA node
+ * according to the proportion of the number of online CPUs and enforced
+ * independently.
+ *
+ * Depending on online CPU distribution, a node may end up with per-node
+ * max_active which is significantly lower than @max_active, which can lead to
+ * deadlocks if the per-node concurrency limit is lower than the maximum number
+ * of interdependent work items for the workqueue.
+ *
+ * To guarantee forward progress regardless of online CPU distribution, the
+ * concurrency limit on every node is guaranteed to be equal to or greater than
+ * min_active which is set to min(@max_active, %WQ_DFL_MIN_ACTIVE). This means
+ * that the sum of per-node max_active's may be larger than @max_active.
+ *
+ * For detailed information on %WQ_* flags, please refer to
* Documentation/core-api/workqueue.rst.
*
* RETURNS:
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 71ac3a3633ac..68d4c78046cd 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -123,6 +123,9 @@ enum {
*
* L: pool->lock protected. Access with pool->lock held.
*
+ * LN: pool->lock and wq_node_nr_active->lock protected for writes. Either for
+ * reads.
+ *
* K: Only modified by worker while holding pool->lock. Can be safely read by
* self, while holding pool->lock or from IRQ context if %current is the
* kworker.
@@ -244,17 +247,18 @@ struct pool_workqueue {
* pwq->inactive_works instead of pool->worklist and marked with
* WORK_STRUCT_INACTIVE.
*
- * All work items marked with WORK_STRUCT_INACTIVE do not participate
- * in pwq->nr_active and all work items in pwq->inactive_works are
- * marked with WORK_STRUCT_INACTIVE. But not all WORK_STRUCT_INACTIVE
- * work items are in pwq->inactive_works. Some of them are ready to
- * run in pool->worklist or worker->scheduled. Those work itmes are
- * only struct wq_barrier which is used for flush_work() and should
- * not participate in pwq->nr_active. For non-barrier work item, it
- * is marked with WORK_STRUCT_INACTIVE iff it is in pwq->inactive_works.
+ * All work items marked with WORK_STRUCT_INACTIVE do not participate in
+ * nr_active and all work items in pwq->inactive_works are marked with
+ * WORK_STRUCT_INACTIVE. But not all WORK_STRUCT_INACTIVE work items are
+ * in pwq->inactive_works. Some of them are ready to run in
+ * pool->worklist or worker->scheduled. Those work itmes are only struct
+ * wq_barrier which is used for flush_work() and should not participate
+ * in nr_active. For non-barrier work item, it is marked with
+ * WORK_STRUCT_INACTIVE iff it is in pwq->inactive_works.
*/
int nr_active; /* L: nr of active works */
struct list_head inactive_works; /* L: inactive works */
+ struct list_head pending_node; /* LN: node on wq_node_nr_active->pending_pwqs */
struct list_head pwqs_node; /* WR: node on wq->pwqs */
struct list_head mayday_node; /* MD: node on wq->maydays */
@@ -286,9 +290,18 @@ struct wq_device;
* on each CPU, in an unbound workqueue, max_active applies to the whole system.
* As sharing a single nr_active across multiple sockets can be very expensive,
* the counting and enforcement is per NUMA node.
+ *
+ * The following struct is used to enforce per-node max_active. When a pwq wants
+ * to start executing a work item, it should increment ->count using
+ * tryinc_node_nr_active(). If acquisition fails due to the count already being
+ * over wq_node_max_active(), the pwq is queued on ->pending_pwqs. As in-flight
+ * work items finish and decrement ->count, node_activate_pending_pwq()
+ * activates the pending pwqs in round-robin order.
*/
struct wq_node_nr_active {
atomic_t count; /* per-node nr_active count */
+ raw_spinlock_t lock; /* nests inside pool locks */
+ struct list_head pending_pwqs; /* LN: pwqs with inactive works */
};
/*
@@ -311,8 +324,12 @@ struct workqueue_struct {
struct worker *rescuer; /* MD: rescue worker */
int nr_drainers; /* WQ: drain in progress */
+
+ /* See alloc_workqueue() function comment for info on min/max_active */
int max_active; /* WO: max active works */
+ int min_active; /* WO: min active works */
int saved_max_active; /* WQ: saved max_active */
+ int saved_min_active; /* WQ: saved min_active */
struct workqueue_attrs *unbound_attrs; /* PW: only for unbound wqs */
struct pool_workqueue *dfl_pwq; /* PW: only for unbound wqs */
@@ -1443,6 +1460,33 @@ static struct wq_node_nr_active *wq_node_nr_active(struct workqueue_struct *wq,
return wq->node_nr_active[node];
}
+/**
+ * wq_node_max_active - Determine max_active to use
+ * @wq: workqueue of interest
+ * @node: NUMA node, can be %NUMA_NO_NODE
+ *
+ * Determine the max_active @wq should use on @node. @wq must be unbound.
+ * max_active is distributed among nodes according to the proportions of numbers
+ * of online cpus. The result is always between @wq->min_active and max_active.
+ */
+static int wq_node_max_active(struct workqueue_struct *wq, int node)
+{
+ int min_active = READ_ONCE(wq->min_active);
+ int max_active = READ_ONCE(wq->max_active);
+ int node_max_active;
+
+ if (node == NUMA_NO_NODE)
+ return min_active;
+
+ node_max_active = DIV_ROUND_UP(max_active * node_nr_cpus[node],
+ num_online_cpus());
+ /*
+ * We aren't synchronizing against CPU hotplug operations and can get
+ * stray numbers. Clamp between min and max.
+ */
+ return clamp(node_max_active, min_active, max_active);
+}
+
/**
* get_pwq - get an extra reference on the specified pool_workqueue
* @pwq: pool_workqueue to get
@@ -1537,35 +1581,99 @@ static bool pwq_activate_work(struct pool_workqueue *pwq,
return true;
}
+static bool tryinc_node_nr_active(struct wq_node_nr_active *nna, int max)
+{
+ while (true) {
+ int old, tmp;
+
+ old = atomic_read(&nna->count);
+ if (old >= max)
+ return false;
+ tmp = atomic_cmpxchg_relaxed(&nna->count, old, old + 1);
+ if (tmp == old)
+ return true;
+ }
+}
+
/**
* pwq_tryinc_nr_active - Try to increment nr_active for a pwq
* @pwq: pool_workqueue of interest
+ * @fill: max_active may have increased, try to increase concurrency level
*
* Try to increment nr_active for @pwq. Returns %true if an nr_active count is
* successfully obtained. %false otherwise.
*/
-static bool pwq_tryinc_nr_active(struct pool_workqueue *pwq)
+static bool pwq_tryinc_nr_active(struct pool_workqueue *pwq, bool fill)
{
struct workqueue_struct *wq = pwq->wq;
struct worker_pool *pool = pwq->pool;
struct wq_node_nr_active *nna = wq_node_nr_active(wq, pool->node);
- bool obtained;
+ bool obtained = false;
+ int node_max_active;
lockdep_assert_held(&pool->lock);
- obtained = pwq->nr_active < READ_ONCE(wq->max_active);
+ if (!nna) {
+ /* per-cpu workqueue, pwq->nr_active is sufficient */
+ obtained = pwq->nr_active < READ_ONCE(wq->max_active);
+ goto out;
+ }
+
+ /*
+ * Unbound workqueue uses per-node shared nr_active $nna. If @pwq is
+ * already waiting on $nna, pwq_dec_nr_active() will maintain the
+ * concurrency level. Don't jump the line.
+ *
+ * We need to ignore the pending test after max_active has increased as
+ * pwq_dec_nr_active() can only maintain the concurrency level but not
+ * increase it. This is indicated by @fill.
+ */
+ if (!list_empty(&pwq->pending_node) && likely(!fill))
+ goto out;
+
+ node_max_active = wq_node_max_active(wq, pool->node);
+
+ obtained = tryinc_node_nr_active(nna, node_max_active);
+ if (obtained)
+ goto out;
- if (obtained) {
+ /*
+ * Lockless acquisition failed. Lock, add ourself to $nna->pending_pwqs
+ * and try again. The smp_mb() is paired with the implied memory barrier
+ * of atomic_dec_return() in pwq_dec_nr_active() to ensure that either
+ * we see the decremented $nna->count or they see non-empty
+ * $nna->pending_pwqs.
+ */
+ raw_spin_lock(&nna->lock);
+
+ if (list_empty(&pwq->pending_node))
+ list_add_tail(&pwq->pending_node, &nna->pending_pwqs);
+ else if (likely(!fill))
+ goto out_unlock;
+
+ smp_mb();
+
+ obtained = tryinc_node_nr_active(nna, node_max_active);
+
+ /*
+ * If @fill, @pwq might have already been pending. Being spuriously
+ * pending in cold paths doesn't affect anything. Let's leave it be.
+ */
+ if (obtained && likely(!fill))
+ list_del_init(&pwq->pending_node);
+
+out_unlock:
+ raw_spin_unlock(&nna->lock);
+out:
+ if (obtained)
pwq->nr_active++;
- if (nna)
- atomic_inc(&nna->count);
- }
return obtained;
}
/**
* pwq_activate_first_inactive - Activate the first inactive work item on a pwq
* @pwq: pool_workqueue of interest
+ * @fill: max_active may have increased, try to increase concurrency level
*
* Activate the first inactive work item of @pwq if available and allowed by
* max_active limit.
@@ -1573,13 +1681,13 @@ static bool pwq_tryinc_nr_active(struct pool_workqueue *pwq)
* Returns %true if an inactive work item has been activated. %false if no
* inactive work item is found or max_active limit is reached.
*/
-static bool pwq_activate_first_inactive(struct pool_workqueue *pwq)
+static bool pwq_activate_first_inactive(struct pool_workqueue *pwq, bool fill)
{
struct work_struct *work =
list_first_entry_or_null(&pwq->inactive_works,
struct work_struct, entry);
- if (work && pwq_tryinc_nr_active(pwq)) {
+ if (work && pwq_tryinc_nr_active(pwq, fill)) {
__pwq_activate_work(pwq, work);
return true;
} else {
@@ -1587,11 +1695,95 @@ static bool pwq_activate_first_inactive(struct pool_workqueue *pwq)
}
}
+/**
+ * node_activate_pending_pwq - Activate a pending pwq on a wq_node_nr_active
+ * @nna: wq_node_nr_active to activate a pending pwq for
+ * @caller_pool: worker_pool the caller is locking
+ *
+ * Activate a pwq in @nna->pending_pwqs. Called with @caller_pool locked.
+ * @caller_pool may be unlocked and relocked to lock other worker_pools.
+ */
+static void node_activate_pending_pwq(struct wq_node_nr_active *nna,
+ struct worker_pool *caller_pool)
+{
+ struct worker_pool *locked_pool = caller_pool;
+ struct pool_workqueue *pwq;
+ struct work_struct *work;
+ int node_max_active;
+
+ lockdep_assert_held(&caller_pool->lock);
+
+ raw_spin_lock(&nna->lock);
+retry:
+ pwq = list_first_entry_or_null(&nna->pending_pwqs,
+ struct pool_workqueue, pending_node);
+ if (!pwq)
+ goto out_unlock;
+
+ /*
+ * If @pwq is for a different pool than @locked_pool, we need to lock
+ * @pwq->pool->lock. Let's trylock first. If unsuccessful, do the unlock
+ * / lock dance. For that, we also need to release @nna->lock as it's
+ * nested inside pool locks.
+ */
+ if (pwq->pool != locked_pool) {
+ raw_spin_unlock(&locked_pool->lock);
+ locked_pool = pwq->pool;
+ if (!raw_spin_trylock(&locked_pool->lock)) {
+ raw_spin_unlock(&nna->lock);
+ raw_spin_lock(&locked_pool->lock);
+ raw_spin_lock(&nna->lock);
+ goto retry;
+ }
+ }
+
+ /*
+ * $pwq may not have any inactive work items due to e.g. cancellations.
+ * Drop it from pending_pwqs and see if there's another one.
+ */
+ work = list_first_entry_or_null(&pwq->inactive_works,
+ struct work_struct, entry);
+ if (!work) {
+ list_del_init(&pwq->pending_node);
+ goto retry;
+ }
+
+ /*
+ * Acquire an nr_active count and activate the inactive work item. If
+ * $pwq still has inactive work items, rotate it to the end of the
+ * pending_pwqs so that we round-robin through them. This means that
+ * inactive work items are not activated in queueing order which is fine
+ * given that there has never been any ordering across different pwqs.
+ */
+ node_max_active = wq_node_max_active(pwq->wq, pwq->pool->node);
+ if (likely(tryinc_node_nr_active(nna, node_max_active))) {
+ pwq->nr_active++;
+ __pwq_activate_work(pwq, work);
+
+ if (list_empty(&pwq->inactive_works))
+ list_del_init(&pwq->pending_node);
+ else
+ list_move_tail(&pwq->pending_node, &nna->pending_pwqs);
+
+ /* if activating a foreign pool, make sure it's running */
+ if (pwq->pool != caller_pool)
+ kick_pool(pwq->pool);
+ }
+
+out_unlock:
+ raw_spin_unlock(&nna->lock);
+ if (locked_pool != caller_pool) {
+ raw_spin_unlock(&locked_pool->lock);
+ raw_spin_lock(&caller_pool->lock);
+ }
+}
+
/**
* pwq_dec_nr_active - Retire an active count
* @pwq: pool_workqueue of interest
*
* Decrement @pwq's nr_active and try to activate the first inactive work item.
+ * For unbound workqueues, this function may temporarily drop @pwq->pool->lock.
*/
static void pwq_dec_nr_active(struct pool_workqueue *pwq)
{
@@ -1611,12 +1803,30 @@ static void pwq_dec_nr_active(struct pool_workqueue *pwq)
* inactive work item on @pwq itself.
*/
if (!nna) {
- pwq_activate_first_inactive(pwq);
+ pwq_activate_first_inactive(pwq, false);
return;
}
- atomic_dec(&nna->count);
- pwq_activate_first_inactive(pwq);
+ /*
+ * If @pwq is for an unbound workqueue, it's more complicated because
+ * multiple pwqs and pools may be sharing the nr_active count. When a
+ * pwq needs to wait for an nr_active count, it puts itself on
+ * $nna->pending_pwqs. The following atomic_dec_return()'s implied
+ * memory barrier is paired with smp_mb() in pwq_tryinc_nr_active() to
+ * guarantee that either we see non-empty pending_pwqs or they see
+ * decremented $nna->count.
+ *
+ * wq_node_max_active() may change as CPUs come online/offline and
+ * @pwq->wq's max_active gets updated. However, it is guaranteed to be
+ * euqal to or larger than @pwq->wq->min_active which is above zero
+ * unless freezing. This maintains the forward progress guarantee.
+ */
+ if (atomic_dec_return(&nna->count) >=
+ wq_node_max_active(pwq->wq, pool->node))
+ return;
+
+ if (!list_empty(&nna->pending_pwqs))
+ node_activate_pending_pwq(nna, pool);
}
/**
@@ -1944,7 +2154,7 @@ static void __queue_work(int cpu, struct workqueue_struct *wq,
* @work must also queue behind existing inactive work items to maintain
* ordering when max_active changes. See wq_adjust_max_active().
*/
- if (list_empty(&pwq->inactive_works) && pwq_tryinc_nr_active(pwq)) {
+ if (list_empty(&pwq->inactive_works) && pwq_tryinc_nr_active(pwq, false)) {
if (list_empty(&pool->worklist))
pool->watchdog_ts = jiffies;
@@ -3179,7 +3389,7 @@ static void insert_wq_barrier(struct pool_workqueue *pwq,
barr->task = current;
- /* The barrier work item does not participate in pwq->nr_active. */
+ /* The barrier work item does not participate in nr_active. */
work_flags |= WORK_STRUCT_INACTIVE;
/*
@@ -4093,6 +4303,8 @@ static void free_node_nr_active(struct wq_node_nr_active **ptr_ar)
static void init_node_nr_active(struct wq_node_nr_active *nna)
{
atomic_set(&nna->count, 0);
+ raw_spin_lock_init(&nna->lock);
+ INIT_LIST_HEAD(&nna->pending_pwqs);
}
/*
@@ -4334,6 +4546,15 @@ static void pwq_release_workfn(struct kthread_work *work)
mutex_unlock(&wq_pool_mutex);
}
+ if (!list_empty(&pwq->pending_node)) {
+ struct wq_node_nr_active *nna =
+ wq_node_nr_active(pwq->wq, pwq->pool->node);
+
+ raw_spin_lock_irq(&nna->lock);
+ list_del_init(&pwq->pending_node);
+ raw_spin_unlock_irq(&nna->lock);
+ }
+
call_rcu(&pwq->rcu, rcu_free_pwq);
/*
@@ -4359,6 +4580,7 @@ static void init_pwq(struct pool_workqueue *pwq, struct workqueue_struct *wq,
pwq->flush_color = -1;
pwq->refcnt = 1;
INIT_LIST_HEAD(&pwq->inactive_works);
+ INIT_LIST_HEAD(&pwq->pending_node);
INIT_LIST_HEAD(&pwq->pwqs_node);
INIT_LIST_HEAD(&pwq->mayday_node);
kthread_init_work(&pwq->release_work, pwq_release_workfn);
@@ -4834,16 +5056,18 @@ static void wq_adjust_max_active(struct workqueue_struct *wq)
return;
}
- if (wq->max_active == wq->saved_max_active)
+ if (wq->max_active == wq->saved_max_active &&
+ wq->min_active == wq->saved_min_active)
return;
/*
- * Update @wq->max_active and then kick inactive work items if more
+ * Update @wq->max/min_active and then kick inactive work items if more
* active work items are allowed. This doesn't break work item ordering
* because new work items are always queued behind existing inactive
* work items if there are any.
*/
WRITE_ONCE(wq->max_active, wq->saved_max_active);
+ WRITE_ONCE(wq->min_active, wq->saved_min_active);
/*
* Round-robin through pwq's activating the first inactive work item
@@ -4858,7 +5082,7 @@ static void wq_adjust_max_active(struct workqueue_struct *wq)
/* can be called during early boot w/ irq disabled */
raw_spin_lock_irqsave(&pwq->pool->lock, flags);
- if (pwq_activate_first_inactive(pwq)) {
+ if (pwq_activate_first_inactive(pwq, true)) {
activated = true;
kick_pool(pwq->pool);
}
@@ -4909,7 +5133,9 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
/* init wq */
wq->flags = flags;
wq->max_active = max_active;
- wq->saved_max_active = max_active;
+ wq->min_active = min(max_active, WQ_DFL_MIN_ACTIVE);
+ wq->saved_max_active = wq->max_active;
+ wq->saved_min_active = wq->min_active;
mutex_init(&wq->mutex);
atomic_set(&wq->nr_pwqs_to_flush, 0);
INIT_LIST_HEAD(&wq->pwqs);
@@ -5076,7 +5302,8 @@ EXPORT_SYMBOL_GPL(destroy_workqueue);
* @wq: target workqueue
* @max_active: new max_active value.
*
- * Set max_active of @wq to @max_active.
+ * Set max_active of @wq to @max_active. See the alloc_workqueue() function
+ * comment.
*
* CONTEXT:
* Don't call from IRQ context.
@@ -5093,6 +5320,9 @@ void workqueue_set_max_active(struct workqueue_struct *wq, int max_active)
wq->flags &= ~__WQ_ORDERED;
wq->saved_max_active = max_active;
+ if (wq->flags & WQ_UNBOUND)
+ wq->saved_min_active = min(wq->saved_min_active, max_active);
+
wq_adjust_max_active(wq);
mutex_unlock(&wq->mutex);
}
--
2.43.0
To prepare for unbound nr_active handling improvements, move work activation
part of pwq_activate_inactive_work() into __pwq_activate_work() and add
pwq_activate_work() which tests WORK_STRUCT_INACTIVE and updates nr_active.
pwq_activate_first_inactive() and try_to_grab_pending() are updated to use
pwq_activate_work(). The latter conversion is functionally identical. For
the former, this conversion adds an unnecessary WORK_STRUCT_INACTIVE
testing. This is temporary and will be removed by the next patch.
Signed-off-by: Tejun Heo <[email protected]>
---
kernel/workqueue.c | 31 +++++++++++++++++++++++++------
1 file changed, 25 insertions(+), 6 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 09f0ec086726..2cd275b34df2 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1461,16 +1461,36 @@ static bool pwq_is_empty(struct pool_workqueue *pwq)
return !pwq->nr_active && list_empty(&pwq->inactive_works);
}
-static void pwq_activate_inactive_work(struct work_struct *work)
+static void __pwq_activate_work(struct pool_workqueue *pwq,
+ struct work_struct *work)
{
- struct pool_workqueue *pwq = get_work_pwq(work);
-
trace_workqueue_activate_work(work);
if (list_empty(&pwq->pool->worklist))
pwq->pool->watchdog_ts = jiffies;
move_linked_works(work, &pwq->pool->worklist, NULL);
__clear_bit(WORK_STRUCT_INACTIVE_BIT, work_data_bits(work));
+}
+
+/**
+ * pwq_activate_work - Activate a work item if inactive
+ * @pwq: pool_workqueue @work belongs to
+ * @work: work item to activate
+ *
+ * Returns %true if activated. %false if already active.
+ */
+static bool pwq_activate_work(struct pool_workqueue *pwq,
+ struct work_struct *work)
+{
+ struct worker_pool *pool = pwq->pool;
+
+ lockdep_assert_held(&pool->lock);
+
+ if (!(*work_data_bits(work) & WORK_STRUCT_INACTIVE))
+ return false;
+
pwq->nr_active++;
+ __pwq_activate_work(pwq, work);
+ return true;
}
static void pwq_activate_first_inactive(struct pool_workqueue *pwq)
@@ -1478,7 +1498,7 @@ static void pwq_activate_first_inactive(struct pool_workqueue *pwq)
struct work_struct *work = list_first_entry(&pwq->inactive_works,
struct work_struct, entry);
- pwq_activate_inactive_work(work);
+ pwq_activate_work(pwq, work);
}
/**
@@ -1616,8 +1636,7 @@ static int try_to_grab_pending(struct work_struct *work, bool is_dwork,
* management later on and cause stall. Make sure the work
* item is activated before grabbing.
*/
- if (*work_data_bits(work) & WORK_STRUCT_INACTIVE)
- pwq_activate_inactive_work(work);
+ pwq_activate_work(pwq, work);
list_del_init(&work->entry);
pwq_dec_nr_in_flight(pwq, *work_data_bits(work));
--
2.43.0
wq_adjust_max_active() needs to activate work items after max_active is
increased. Previously, it did that by visiting each pwq once activating all
that could be activated. While this makes sense with per-pwq nr_active,
nr_active will be shared across multiple pwqs for unbound wqs. Then, we'd
want to round-robin through pwqs to be fairer.
In preparation, this patch makes wq_adjust_max_active() round-robin pwqs
while activating. While the activation ordering changes, this shouldn't
cause user-noticeable behavior changes.
Signed-off-by: Tejun Heo <[email protected]>
---
kernel/workqueue.c | 31 +++++++++++++++++++------------
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 7eb1925087fd..f90f797e73a1 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -4700,7 +4700,7 @@ static int init_rescuer(struct workqueue_struct *wq)
*/
static void wq_adjust_max_active(struct workqueue_struct *wq)
{
- struct pool_workqueue *pwq;
+ bool activated;
lockdep_assert_held(&wq->mutex);
@@ -4720,19 +4720,26 @@ static void wq_adjust_max_active(struct workqueue_struct *wq)
*/
WRITE_ONCE(wq->max_active, wq->saved_max_active);
- for_each_pwq(pwq, wq) {
- unsigned long flags;
-
- /* this function can be called during early boot w/ irq disabled */
- raw_spin_lock_irqsave(&pwq->pool->lock, flags);
-
- while (pwq_activate_first_inactive(pwq))
- ;
+ /*
+ * Round-robin through pwq's activating the first inactive work item
+ * until max_active is filled.
+ */
+ do {
+ struct pool_workqueue *pwq;
- kick_pool(pwq->pool);
+ activated = false;
+ for_each_pwq(pwq, wq) {
+ unsigned long flags;
- raw_spin_unlock_irqrestore(&pwq->pool->lock, flags);
- }
+ /* can be called during early boot w/ irq disabled */
+ raw_spin_lock_irqsave(&pwq->pool->lock, flags);
+ if (pwq_activate_first_inactive(pwq)) {
+ activated = true;
+ kick_pool(pwq->pool);
+ }
+ raw_spin_unlock_irqrestore(&pwq->pool->lock, flags);
+ }
+ } while (activated);
}
__printf(1, 4)
--
2.43.0
To track the first possible NUMA node and the number of online CPUs in each
NUMA node, respectively. These will be used to implement system-wide
nr_active for unbound workqueues.
Signed-off-by: Tejun Heo <[email protected]>
---
kernel/workqueue.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index f90f797e73a1..3f45baa96d51 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -53,6 +53,7 @@
#include <linux/nmi.h>
#include <linux/kvm_para.h>
#include <linux/delay.h>
+#include <linux/topology.h>
#include "workqueue_internal.h"
@@ -341,6 +342,8 @@ struct wq_pod_type {
int *cpu_pod; /* cpu -> pod */
};
+static int first_possible_node __read_mostly;
+static int node_nr_cpus[MAX_NUMNODES] __read_mostly;
static struct wq_pod_type wq_pod_types[WQ_AFFN_NR_TYPES];
static enum wq_affn_scope wq_affn_dfl = WQ_AFFN_CACHE;
@@ -5617,6 +5620,8 @@ int workqueue_online_cpu(unsigned int cpu)
struct workqueue_struct *wq;
int pi;
+ node_nr_cpus[cpu_to_node(cpu)]++;
+
mutex_lock(&wq_pool_mutex);
for_each_pool(pool, pi) {
@@ -5672,6 +5677,8 @@ int workqueue_offline_cpu(unsigned int cpu)
}
mutex_unlock(&wq_pool_mutex);
+ node_nr_cpus[cpu_to_node(cpu)]--;
+
return 0;
}
@@ -6687,6 +6694,9 @@ void __init workqueue_init_early(void)
BUILD_BUG_ON(__alignof__(struct pool_workqueue) < __alignof__(long long));
+ first_possible_node = first_node(node_states[N_POSSIBLE]);
+ node_nr_cpus[cpu_to_node(0)]++;
+
BUG_ON(!alloc_cpumask_var(&wq_unbound_cpumask, GFP_KERNEL));
BUG_ON(!alloc_cpumask_var(&wq_requested_unbound_cpumask, GFP_KERNEL));
BUG_ON(!zalloc_cpumask_var(&wq_isolated_cpumask, GFP_KERNEL));
--
2.43.0
Hello, Tejun
On Sat, Jan 13, 2024 at 8:29 AM Tejun Heo <[email protected]> wrote:
> + */
> +static int wq_node_max_active(struct workqueue_struct *wq, int node)
> +{
> + int min_active = READ_ONCE(wq->min_active);
> + int max_active = READ_ONCE(wq->max_active);
> + int node_max_active;
> +
> + if (node == NUMA_NO_NODE)
> + return min_active;
> +
> + node_max_active = DIV_ROUND_UP(max_active * node_nr_cpus[node],
> + num_online_cpus());
node_nr_cpus[node] and num_online_cpus() are global values, they might
not suitable
for this particular workqueue and might cause skewed proportions.
the cache values:
pwq->pool->attrs->pool_nr_online_cpus =
cpumask_weight_and(pwq->pool->attrs->__pod_cpumask, cpu_online_mask);
wq->wq_nr_online_cpus =
cpumask_weight_and(wq->unbound_attrs->cpumask, cpu_online_mask);
can be used instead. They can be calculated at creation and cpuhotplug.
pool_nr_online_cpus doesn't contribute to the pool's hash value.
Or the result of wq_node_max_active() can be cached in struct wq_node_nr_active,
see the comment next.
> -static bool pwq_tryinc_nr_active(struct pool_workqueue *pwq)
> +static bool pwq_tryinc_nr_active(struct pool_workqueue *pwq, bool fill)
> {
> struct workqueue_struct *wq = pwq->wq;
> struct worker_pool *pool = pwq->pool;
> struct wq_node_nr_active *nna = wq_node_nr_active(wq, pool->node);
> - bool obtained;
> + bool obtained = false;
> + int node_max_active;
>
> lockdep_assert_held(&pool->lock);
>
> - obtained = pwq->nr_active < READ_ONCE(wq->max_active);
> + if (!nna) {
> + /* per-cpu workqueue, pwq->nr_active is sufficient */
> + obtained = pwq->nr_active < READ_ONCE(wq->max_active);
> + goto out;
> + }
> +
> + /*
> + * Unbound workqueue uses per-node shared nr_active $nna. If @pwq is
> + * already waiting on $nna, pwq_dec_nr_active() will maintain the
> + * concurrency level. Don't jump the line.
> + *
> + * We need to ignore the pending test after max_active has increased as
> + * pwq_dec_nr_active() can only maintain the concurrency level but not
> + * increase it. This is indicated by @fill.
> + */
> + if (!list_empty(&pwq->pending_node) && likely(!fill))
> + goto out;
> +
> + node_max_active = wq_node_max_active(wq, pool->node);
It is a hot path for unbound workqueues, I think the result of
wq_node_max_active()
should be cached in struct wq_node_nr_active.
The result be calculated at creation, cpuhotplug, and changing max_active.
>
> /**
> * pwq_activate_first_inactive - Activate the first inactive work item on a pwq
> * @pwq: pool_workqueue of interest
> + * @fill: max_active may have increased, try to increase concurrency level
I think it is also legitimate to increase the concurrency level ASAP
when called from try_to_grab_pending() path.
Thanks
Lai
Hello, Tejun
On Sat, Jan 13, 2024 at 8:29 AM Tejun Heo <[email protected]> wrote:
> @@ -327,6 +337,7 @@ struct workqueue_struct {
> /* hot fields used during command issue, aligned to cacheline */
> unsigned int flags ____cacheline_aligned; /* WQ: WQ_* flags */
> struct pool_workqueue __percpu __rcu **cpu_pwq; /* I: per-cpu pwqs */
> + struct wq_node_nr_active **node_nr_active; /* I: per-node nr_active */
flexible array can be used here.
Thanks
Lai
On Fri, Jan 19, 2024 at 03:55:49PM +0800, Lai Jiangshan wrote:
> Hello, Tejun
>
> On Sat, Jan 13, 2024 at 8:29 AM Tejun Heo <[email protected]> wrote:
>
> > @@ -327,6 +337,7 @@ struct workqueue_struct {
> > /* hot fields used during command issue, aligned to cacheline */
> > unsigned int flags ____cacheline_aligned; /* WQ: WQ_* flags */
> > struct pool_workqueue __percpu __rcu **cpu_pwq; /* I: per-cpu pwqs */
> > + struct wq_node_nr_active **node_nr_active; /* I: per-node nr_active */
>
> flexible array can be used here.
Yeah, will do that.
Thanks.
--
tejun
Hello,
On Fri, Jan 19, 2024 at 03:54:24PM +0800, Lai Jiangshan wrote:
> node_nr_cpus[node] and num_online_cpus() are global values, they might
> not suitable
> for this particular workqueue and might cause skewed proportions.
Good point.
> the cache values:
>
> pwq->pool->attrs->pool_nr_online_cpus =
> cpumask_weight_and(pwq->pool->attrs->__pod_cpumask, cpu_online_mask);
>
> wq->wq_nr_online_cpus =
> cpumask_weight_and(wq->unbound_attrs->cpumask, cpu_online_mask);
>
> can be used instead. They can be calculated at creation and cpuhotplug.
> pool_nr_online_cpus doesn't contribute to the pool's hash value.
>
> Or the result of wq_node_max_active() can be cached in struct wq_node_nr_active,
> see the comment next.
..
> > + node_max_active = wq_node_max_active(wq, pool->node);
>
> It is a hot path for unbound workqueues, I think the result of
> wq_node_max_active()
> should be cached in struct wq_node_nr_active.
I'm skeptical this would make a meaningful difference but if we're
calculating the numbers per-workqueue anyways, maybe this won't add too much
complexity.
> > /**
> > * pwq_activate_first_inactive - Activate the first inactive work item on a pwq
> > * @pwq: pool_workqueue of interest
> > + * @fill: max_active may have increased, try to increase concurrency level
>
> I think it is also legitimate to increase the concurrency level ASAP
> when called from try_to_grab_pending() path.
Can you elaborate why that'd be useful?
Thanks.
--
tejun
Hello, Tejun
On Wed, Jan 24, 2024 at 8:30 AM Tejun Heo <[email protected]> wrote:
> > > /**
> > > * pwq_activate_first_inactive - Activate the first inactive work item on a pwq
> > > * @pwq: pool_workqueue of interest
> > > + * @fill: max_active may have increased, try to increase concurrency level
> >
> > I think it is also legitimate to increase the concurrency level ASAP
> > when called from try_to_grab_pending() path.
>
> Can you elaborate why that'd be useful?
>
If an active item for a PWQ is canceled, the PWQ will lose a hard-earned
nr_active and have to wait on the round-robbin queue for another nr_active.
It seems it is unfair for this PWQ. If a user's program pattern is
queuing-checking-cancelling items, it can cause delays for other users
sharing the same workqueue.
Thanks
Lai
Hello, Lai.
On Wed, Jan 24, 2024 at 10:54:26AM +0800, Lai Jiangshan wrote:
> If an active item for a PWQ is canceled, the PWQ will lose a hard-earned
> nr_active and have to wait on the round-robbin queue for another nr_active.
>
> It seems it is unfair for this PWQ. If a user's program pattern is
> queuing-checking-cancelling items, it can cause delays for other users
> sharing the same workqueue.
pwq's get RR-queued iff there's congetion in the workqueue and when an
inactive work item gets activated, it gets dispatched for execution right
away. Note that for unbound workqueues, need_more_worker() is always true
when there are active work items on the worklist.
ie. The only time there's meaningful time window between a work item getting
activated and starting execution is when more workers need to be created and
the system is under significant memory pressure. Note that the former is
always a temporary condition as we retain workers for a while once they're
created.
IOW, there's no meaningful time window in which a work item which was
activated after waiting on node_nr_active pending list and then can get
canceled. It's a possible but fringe scenario which won't happen in any
meaningful frequency and even when that happens the impact isn't much to
worry about. I don't think it's a good idea to add complications for such
cases.
Thanks.
--
tejun