[PATCH-wq v3 0/4] workqueue: Enable unbound cpumask update on ordered workqueues

[PATCH-wq v3 0/4] workqueue: Enable unbound cpumask update on ordered workqueues

[Waiman Long]

 v3:
  - [v2] https://lore.kernel.org/lkml/20240203154334.791910-1-longman@redhat.com/
  - Drop patch 1 as it has been merged into the for-6.9 branch.
  - Use rcu_access_pointer() to access wq->dfl_pwq.
  - Use RCU protection instead of acquiring wq->mutex in
    apply_wqattrs_cleanup().

 v2:
  - [v1] https://lore.kernel.org/all/20240130183336.511948-1-longman@redhat.com/
  - Rebased on top of wq's for-v6.9 branch.
  - Use the new pwq_tryinc_nr_active() mechanism to freeze the new
    pwq of an ordered workqueue until the old pwq has been properly
    drained to maintain ordering.
  - Make rescuer follow changes in workqueue unbound cpumask as well
    as its sysfs cpumask, if available.

Ordered workqueues does not currently follow changes made to the
global unbound cpumask because per-pool workqueue changes may break
the ordering guarantee. IOW, a work function in an ordered workqueue
may run on a cpuset isolated CPU.

This series enables ordered workqueues to follow changes made to
the global unbound cpumask by temporaily freeze the newly allocated
pool_workqueue by using the new frozen flag to freeze execution of
newly queued work items until the old pwq has been properly flushed.

The cpumask of the rescuer task of each workqueue is also made to follow
changes in workqueue unbound cpumask as well as its sysfs cpumask,
if available.

Juri Lelli (1):
  kernel/workqueue: Let rescuers follow unbound wq cpumask changes

Waiman Long (3):
  workqueue: Enable unbound cpumask update on ordered workqueues
  workqueue: Thaw frozen pwq in workqueue_apply_unbound_cpumask()
  workqueue: Bind unbound workqueue rescuer to wq_unbound_cpumask

 kernel/workqueue.c | 127 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 113 insertions(+), 14 deletions(-)

-- 
2.39.3

[PATCH-wq v3 1/4] workqueue: Enable unbound cpumask update on ordered workqueues

[Waiman Long]

Ordered workqueues does not currently follow changes made to the
global unbound cpumask because per-pool workqueue changes may break
the ordering guarantee. IOW, a work function in an ordered workqueue
may run on an isolated CPU.

This patch enables ordered workqueues to follow changes made to
the global unbound cpumask by temporaily freeze the newly allocated
pool_workqueue by using the new frozen flag to freeze execution of
newly queued work items until the old pwq has been properly flushed.

This enables ordered workqueues to follow the unbound cpumask changes
like other unbound workqueues at the expense of some delay in execution
of work functions during the transition period.

Signed-off-by: Waiman Long <longman@redhat.com>
Tested-by: Juri Lelli <juri.lelli@redhat.com>
---
 kernel/workqueue.c | 93 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 80 insertions(+), 13 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 68c48489eab3..9b107e8a2c15 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -255,6 +255,7 @@ struct pool_workqueue {
 	int			refcnt;		/* L: reference count */
 	int			nr_in_flight[WORK_NR_COLORS];
 						/* L: nr of in_flight works */
+	int			frozen;		/* L: temporarily frozen */
 
 	/*
 	 * nr_active management and WORK_STRUCT_INACTIVE:
@@ -1702,6 +1703,9 @@ static bool pwq_tryinc_nr_active(struct pool_workqueue *pwq, bool fill)
 
 	lockdep_assert_held(&pool->lock);
 
+	if (pwq->frozen)
+		return false;
+
 	if (!nna) {
 		/* BH or per-cpu workqueue, pwq->nr_active is sufficient */
 		obtained = pwq->nr_active < READ_ONCE(wq->max_active);
@@ -1782,6 +1786,21 @@ static bool pwq_activate_first_inactive(struct pool_workqueue *pwq, bool fill)
 	}
 }
 
+/**
+ * thaw_pwq - thaw a frozen pool_workqueue
+ * @pwq: pool_workqueue to be thawed
+ */
+static void thaw_pwq(struct pool_workqueue *pwq)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&pwq->pool->lock, flags);
+	pwq->frozen = false;
+	if (pwq_activate_first_inactive(pwq, true))
+		kick_pool(pwq->pool);
+	raw_spin_unlock_irqrestore(&pwq->pool->lock, flags);
+}
+
 /**
  * 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
@@ -4740,6 +4759,18 @@ static void pwq_release_workfn(struct kthread_work *work)
 		mutex_lock(&wq->mutex);
 		list_del_rcu(&pwq->pwqs_node);
 		is_last = list_empty(&wq->pwqs);
+
+		/*
+		 * For ordered workqueue with a frozen dfl_pwq, thaw it now.
+		 */
+		if (!is_last && (wq->flags & __WQ_ORDERED_EXPLICIT)) {
+			struct pool_workqueue *dfl_pwq;
+
+			dfl_pwq = rcu_access_pointer(wq->dfl_pwq);
+			if (dfl_pwq && dfl_pwq->frozen)
+				thaw_pwq(dfl_pwq);
+		}
+
 		mutex_unlock(&wq->mutex);
 	}
 
@@ -4906,7 +4937,22 @@ static void apply_wqattrs_cleanup(struct apply_wqattrs_ctx *ctx)
 
 		for_each_possible_cpu(cpu)
 			put_pwq_unlocked(ctx->pwq_tbl[cpu]);
+
+		/*
+		 * Acquire rcu_read_lock() before refcnt can become 0 to
+		 * ensure that ctx->dfl_pwq won't be freed.
+		 */
+		rcu_read_lock();
 		put_pwq_unlocked(ctx->dfl_pwq);
+		if ((ctx->wq->flags & __WQ_ORDERED_EXPLICIT) &&
+		     ctx->dfl_pwq && !ctx->dfl_pwq->refcnt) {
+			struct pool_workqueue *dfl_pwq;
+
+			dfl_pwq = rcu_access_pointer(ctx->wq->dfl_pwq);
+			if (dfl_pwq && dfl_pwq->frozen)
+				thaw_pwq(dfl_pwq);
+		}
+		rcu_read_unlock();
 
 		free_workqueue_attrs(ctx->attrs);
 
@@ -4966,6 +5012,15 @@ apply_wqattrs_prepare(struct workqueue_struct *wq,
 	cpumask_copy(new_attrs->__pod_cpumask, new_attrs->cpumask);
 	ctx->attrs = new_attrs;
 
+	/*
+	 * For initialized ordered workqueues, there is only one pwq (dfl_pwq).
+	 * Temporarily the frozen flag of ctx->dfl_pwq to freeze the execution
+	 * of newly queued work items until execution of older work items in
+	 * the old pwq has completed.
+	 */
+	if (!list_empty(&wq->pwqs) && (wq->flags & __WQ_ORDERED_EXPLICIT))
+		ctx->dfl_pwq->frozen = true;
+
 	ctx->wq = wq;
 	return ctx;
 
@@ -5006,13 +5061,8 @@ static int apply_workqueue_attrs_locked(struct workqueue_struct *wq,
 	if (WARN_ON(!(wq->flags & WQ_UNBOUND)))
 		return -EINVAL;
 
-	/* creating multiple pwqs breaks ordering guarantee */
-	if (!list_empty(&wq->pwqs)) {
-		if (WARN_ON(wq->flags & __WQ_ORDERED_EXPLICIT))
-			return -EINVAL;
-
+	if (!list_empty(&wq->pwqs) && !(wq->flags & __WQ_ORDERED_EXPLICIT))
 		wq->flags &= ~__WQ_ORDERED;
-	}
 
 	ctx = apply_wqattrs_prepare(wq, attrs, wq_unbound_cpumask);
 	if (IS_ERR(ctx))
@@ -6504,11 +6554,29 @@ static int workqueue_apply_unbound_cpumask(const cpumask_var_t unbound_cpumask)
 		if (!(wq->flags & WQ_UNBOUND) || (wq->flags & __WQ_DESTROYING))
 			continue;
 
-		/* creating multiple pwqs breaks ordering guarantee */
+		/*
+		 * We does not support changing cpumask of an ordered workqueue
+		 * again before the previous cpumask change is completed.
+		 * Sleep up to 100ms in 10ms interval to allow previous
+		 * operation to complete and skip it if not done by then.
+		 */
 		if (!list_empty(&wq->pwqs)) {
-			if (wq->flags & __WQ_ORDERED_EXPLICIT)
-				continue;
-			wq->flags &= ~__WQ_ORDERED;
+			struct pool_workqueue *dfl_pwq;
+
+			dfl_pwq = rcu_access_pointer(wq->dfl_pwq);
+			if (!(wq->flags & __WQ_ORDERED_EXPLICIT)) {
+				wq->flags &= ~__WQ_ORDERED;
+			} else if (dfl_pwq && dfl_pwq->frozen) {
+				int i;
+
+				for (i = 0; i < 10; i++) {
+					msleep(10);
+					if (!dfl_pwq->frozen)
+						break;
+				}
+				if (WARN_ON_ONCE(dfl_pwq->frozen))
+					continue;
+			}
 		}
 
 		ctx = apply_wqattrs_prepare(wq, wq->unbound_attrs, unbound_cpumask);
@@ -7024,9 +7092,8 @@ int workqueue_sysfs_register(struct workqueue_struct *wq)
 	int ret;
 
 	/*
-	 * Adjusting max_active or creating new pwqs by applying
-	 * attributes breaks ordering guarantee.  Disallow exposing ordered
-	 * workqueues.
+	 * Adjusting max_active breaks ordering guarantee.  Disallow exposing
+	 * ordered workqueues.
 	 */
 	if (WARN_ON(wq->flags & __WQ_ORDERED_EXPLICIT))
 		return -EINVAL;
-- 
2.39.3

[PATCH-wq v3 2/4] workqueue: Thaw frozen pwq in workqueue_apply_unbound_cpumask()

[Waiman Long]

workqueue_apply_unbound_cpumask() cannot proceed with an ordered
workqueue if its dfl_pwq is still frozen. Just do a sleep wait for
it to be thawed may not work in some cases if pwq_release_workfn() is
somehow prevented from being called due to resources (e.g. wq_pool_mutex)
that are held by its caller.

To break the logjam, we have to actively check if the frozen dfl_pwq
is ready to be thawed and call thaw_pwq() directly if so.

Signed-off-by: Waiman Long <longman@redhat.com>
Tested-by: Juri Lelli <juri.lelli@redhat.com>
---
 kernel/workqueue.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 9b107e8a2c15..f453f339f74a 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -6541,6 +6541,33 @@ void thaw_workqueues(void)
 }
 #endif /* CONFIG_FREEZER */
 
+/*
+ * Check the given ordered workqueue to see if its non-default pwq's have
+ * zero reference count and if so thaw the frozen default pwq.
+ *
+ * Return:
+ * %true if dfl_pwq has been thawed or %false otherwise.
+ */
+static bool ordered_workqueue_ref_check(struct workqueue_struct *wq)
+{
+	struct pool_workqueue *dfl_pwq = rcu_access_pointer(wq->dfl_pwq);
+	struct pool_workqueue *pwq;
+	int refs = 0;
+
+	if (!READ_ONCE(dfl_pwq->frozen))
+		return true;
+	mutex_lock(&wq->mutex);
+	for_each_pwq(pwq, wq) {
+		if (pwq == dfl_pwq)
+			continue;
+		refs += pwq->refcnt;
+	}
+	if (!refs)
+		thaw_pwq(dfl_pwq);
+	mutex_unlock(&wq->mutex);
+	return !refs;
+}
+
 static int workqueue_apply_unbound_cpumask(const cpumask_var_t unbound_cpumask)
 {
 	LIST_HEAD(ctxs);
@@ -6566,12 +6593,12 @@ static int workqueue_apply_unbound_cpumask(const cpumask_var_t unbound_cpumask)
 			dfl_pwq = rcu_access_pointer(wq->dfl_pwq);
 			if (!(wq->flags & __WQ_ORDERED_EXPLICIT)) {
 				wq->flags &= ~__WQ_ORDERED;
-			} else if (dfl_pwq && dfl_pwq->frozen) {
+			} else if (dfl_pwq && !ordered_workqueue_ref_check(wq)) {
 				int i;
 
 				for (i = 0; i < 10; i++) {
 					msleep(10);
-					if (!dfl_pwq->frozen)
+					if (ordered_workqueue_ref_check(wq))
 						break;
 				}
 				if (WARN_ON_ONCE(dfl_pwq->frozen))
-- 
2.39.3

[PATCH-wq v3 3/4] kernel/workqueue: Let rescuers follow unbound wq cpumask changes

[Waiman Long]

From: Juri Lelli <juri.lelli@redhat.com>

When workqueue cpumask changes are committed the associated rescuer (if
one exists) affinity is not touched and this might be a problem down the
line for isolated setups.

Make sure rescuers affinity is updated every time a workqueue cpumask
changes, so that rescuers can't break isolation.

 [longman: set_cpus_allowed_ptr() will block until the designated task
  is enqueued on an allowed CPU, no wake_up_process() needed. Also use
  the unbound_effective_cpumask() helper as suggested by Tejun.]

Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
Signed-off-by: Waiman Long <longman@redhat.com>
Tested-by: Juri Lelli <juri.lelli@redhat.com>
---
 kernel/workqueue.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index f453f339f74a..bbd4269d2729 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5049,6 +5049,11 @@ static void apply_wqattrs_commit(struct apply_wqattrs_ctx *ctx)
 	/* update node_nr_active->max */
 	wq_update_node_max_active(ctx->wq, -1);
 
+	/* rescuer needs to respect wq cpumask changes */
+	if (ctx->wq->rescuer)
+		set_cpus_allowed_ptr(ctx->wq->rescuer->task,
+				     unbound_effective_cpumask(ctx->wq));
+
 	mutex_unlock(&ctx->wq->mutex);
 }
 
-- 
2.39.3

[PATCH-wq v3 4/4] workqueue: Bind unbound workqueue rescuer to wq_unbound_cpumask

[Waiman Long]

Commit 85f0ab43f9de ("kernel/workqueue: Bind rescuer to unbound
cpumask for WQ_UNBOUND") modified init_rescuer() to bind rescuer of
an unbound workqueue to the cpumask in wq->unbound_attrs. However
unbound_attrs->cpumask's of all workqueues are initialized to
cpu_possible_mask and will only be changed if it has the WQ_SYSFS flag
to expose a cpumask sysfs file to be written by users. So this patch
doesn't achieve what it is intended to do.

If an unbound workqueue is created after wq_unbound_cpumask is modified
and there is no more unbound cpumask update after that, the unbound
rescuer will be bound to all CPUs unless the workqueue is created
with the WQ_SYSFS flag and an user explicitly modified its cpumask
sysfs file.  Fix this problem by binding directly to wq_unbound_cpumask
in init_rescuer().

Fixes: 85f0ab43f9de ("kernel/workqueue: Bind rescuer to unbound cpumask for WQ_UNBOUND")
Signed-off-by: Waiman Long <longman@redhat.com>
Tested-by: Juri Lelli <juri.lelli@redhat.com>
---
 kernel/workqueue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index bbd4269d2729..6077d5d663c3 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5300,7 +5300,7 @@ static int init_rescuer(struct workqueue_struct *wq)
 
 	wq->rescuer = rescuer;
 	if (wq->flags & WQ_UNBOUND)
-		kthread_bind_mask(rescuer->task, wq->unbound_attrs->cpumask);
+		kthread_bind_mask(rescuer->task, wq_unbound_cpumask);
 	else
 		kthread_bind_mask(rescuer->task, cpu_possible_mask);
 	wake_up_process(rescuer->task);
-- 
2.39.3

Re: [PATCH-wq v3 0/4] workqueue: Enable unbound cpumask update on ordered workqueues

[Tejun Heo]

On Mon, Feb 05, 2024 at 02:45:58PM -0500, Waiman Long wrote:
>  v3:
>   - [v2] https://lore.kernel.org/lkml/20240203154334.791910-1-longman@redhat.com/
>   - Drop patch 1 as it has been merged into the for-6.9 branch.
>   - Use rcu_access_pointer() to access wq->dfl_pwq.
>   - Use RCU protection instead of acquiring wq->mutex in
>     apply_wqattrs_cleanup().

Looks like we raced each other. I'll wait for v4.

Thanks.

-- 
tejun

Re: [PATCH-wq v3 0/4] workqueue: Enable unbound cpumask update on ordered workqueues

[Tejun Heo]

On Mon, Feb 05, 2024 at 09:53:09AM -1000, Tejun Heo wrote:
> On Mon, Feb 05, 2024 at 02:45:58PM -0500, Waiman Long wrote:
> >  v3:
> >   - [v2] https://lore.kernel.org/lkml/20240203154334.791910-1-longman@redhat.com/
> >   - Drop patch 1 as it has been merged into the for-6.9 branch.
> >   - Use rcu_access_pointer() to access wq->dfl_pwq.
> >   - Use RCU protection instead of acquiring wq->mutex in
> >     apply_wqattrs_cleanup().
> 
> Looks like we raced each other. I'll wait for v4.

BTW, please don't bother to handle __WQ_ORDERED being cleared. We are very
close to removing the implicit ORDERED promotion, so we should be able to
apply the patch to remove the distinction between explicitly and implicitly
ordered workqueues.

Thanks.

-- 
tejun

Re: [PATCH-wq v3 0/4] workqueue: Enable unbound cpumask update on ordered workqueues

[Waiman Long]

On 2/5/24 19:04, Tejun Heo wrote:
> On Mon, Feb 05, 2024 at 09:53:09AM -1000, Tejun Heo wrote:
>> On Mon, Feb 05, 2024 at 02:45:58PM -0500, Waiman Long wrote:
>>>   v3:
>>>    - [v2] https://lore.kernel.org/lkml/20240203154334.791910-1-longman@redhat.com/
>>>    - Drop patch 1 as it has been merged into the for-6.9 branch.
>>>    - Use rcu_access_pointer() to access wq->dfl_pwq.
>>>    - Use RCU protection instead of acquiring wq->mutex in
>>>      apply_wqattrs_cleanup().
>> Looks like we raced each other. I'll wait for v4.
> BTW, please don't bother to handle __WQ_ORDERED being cleared. We are very
> close to removing the implicit ORDERED promotion, so we should be able to
> apply the patch to remove the distinction between explicitly and implicitly
> ordered workqueues.

OK, I saw your new commit 3bc1e711c26b ("workqueue: Don't implicitly 
make UNBOUND workqueues w/ @max_active==1 ordered") in the for-6.9 
branch. Will rebase my patch series on top of that and make the 
necessary modification.

Thanks,
Longman

Re: [PATCH-wq v3 0/4] workqueue: Enable unbound cpumask update on ordered workqueues

[Waiman Long]

On 2/5/24 19:04, Tejun Heo wrote:
> On Mon, Feb 05, 2024 at 09:53:09AM -1000, Tejun Heo wrote:
>> On Mon, Feb 05, 2024 at 02:45:58PM -0500, Waiman Long wrote:
>>>   v3:
>>>    - [v2] https://lore.kernel.org/lkml/20240203154334.791910-1-longman@redhat.com/
>>>    - Drop patch 1 as it has been merged into the for-6.9 branch.
>>>    - Use rcu_access_pointer() to access wq->dfl_pwq.
>>>    - Use RCU protection instead of acquiring wq->mutex in
>>>      apply_wqattrs_cleanup().
>> Looks like we raced each other. I'll wait for v4.
> BTW, please don't bother to handle __WQ_ORDERED being cleared. We are very
> close to removing the implicit ORDERED promotion, so we should be able to
> apply the patch to remove the distinction between explicitly and implicitly
> ordered workqueues.

BTW, the workqueue.c file in your latest for-6.9 branch still has a 
reference to __WQ_ORDERED_EXPLICIT in workqueue_apply_unbound_cpumask(). 
Will that break compilation?

Regards,
Longman

Re: [PATCH-wq v3 0/4] workqueue: Enable unbound cpumask update on ordered workqueues

[Tejun Heo]

On Mon, Feb 05, 2024 at 08:24:06PM -0500, Waiman Long wrote:
> 
> On 2/5/24 19:04, Tejun Heo wrote:
> > On Mon, Feb 05, 2024 at 09:53:09AM -1000, Tejun Heo wrote:
> > > On Mon, Feb 05, 2024 at 02:45:58PM -0500, Waiman Long wrote:
> > > >   v3:
> > > >    - [v2] https://lore.kernel.org/lkml/20240203154334.791910-1-longman@redhat.com/
> > > >    - Drop patch 1 as it has been merged into the for-6.9 branch.
> > > >    - Use rcu_access_pointer() to access wq->dfl_pwq.
> > > >    - Use RCU protection instead of acquiring wq->mutex in
> > > >      apply_wqattrs_cleanup().
> > > Looks like we raced each other. I'll wait for v4.
> > BTW, please don't bother to handle __WQ_ORDERED being cleared. We are very
> > close to removing the implicit ORDERED promotion, so we should be able to
> > apply the patch to remove the distinction between explicitly and implicitly
> > ordered workqueues.
> 
> BTW, the workqueue.c file in your latest for-6.9 branch still has a
> reference to __WQ_ORDERED_EXPLICIT in workqueue_apply_unbound_cpumask().
> Will that break compilation?

Right you are. Will post a followup patch.

Thanks.

-- 
tejun