[PATCH 0/7] Add wq_online_cpumask and remove cpus_read_lock() from apply_wqattrs_lock()

[PATCH 0/7] Add wq_online_cpumask and remove cpus_read_lock() from apply_wqattrs_lock()

[Lai Jiangshan]

From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

The new wq_online_mask mirrors the cpu_online_mask except during
hotplugging; specifically, it differs between the hotplugging stages
of workqueue_offline_cpu() and workqueue_online_cpu(), during which
the transitioning CPU is not represented in the mask.

With wq_online_cpumask, cpus_read_lock() is unneeded for wqattrs changes.


Lai Jiangshan (7):
  workqueue: Add wq_online_cpumask
  workqueue: Simplify wq_calc_pod_cpumask() with wq_online_cpumask
  workqueue: Remove cpus_read_lock() from apply_wqattrs_lock()
  workqueue: Remove the unneeded cpumask empty check in
    wq_calc_pod_cpumask()
  workqueue: Remove the argument @cpu_going_down from
    wq_calc_pod_cpumask()
  workqueue: Remove the arguments @hotplug_cpu and @online from
    wq_update_pod()
  workqueue: Rename wq_update_pod() to unbound_wq_update_pwq()

Cc: Tejun Heo <tj@kernel.org>

 kernel/workqueue.c | 76 +++++++++++++++++++---------------------------
 1 file changed, 32 insertions(+), 44 deletions(-)

-- 
2.19.1.6.gb485710b

[PATCH 1/7] workqueue: Add wq_online_cpumask

[Lai Jiangshan]

From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

The new wq_online_mask mirrors the cpu_online_mask except during
hotplugging; specifically, it differs between the hotplugging stages
of workqueue_offline_cpu() and workqueue_online_cpu(), during which
the transitioning CPU is not represented in the mask.

Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
---
 kernel/workqueue.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 5d362290c2e8..985ab9230fe0 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -444,6 +444,9 @@ static struct rcuwait manager_wait = __RCUWAIT_INITIALIZER(manager_wait);
 static LIST_HEAD(workqueues);		/* PR: list of all workqueues */
 static bool workqueue_freezing;		/* PL: have wqs started freezing? */
 
+/* PL: mirror the cpu_online_mask excluding the CPU in the midst of hotplugging */
+static cpumask_var_t wq_online_cpumask;
+
 /* PL&A: allowable cpus for unbound wqs and work items */
 static cpumask_var_t wq_unbound_cpumask;
 
@@ -6583,6 +6586,8 @@ int workqueue_online_cpu(unsigned int cpu)
 
 	mutex_lock(&wq_pool_mutex);
 
+	cpumask_set_cpu(cpu, wq_online_cpumask);
+
 	for_each_pool(pool, pi) {
 		/* BH pools aren't affected by hotplug */
 		if (pool->flags & POOL_BH)
@@ -6629,6 +6634,9 @@ int workqueue_offline_cpu(unsigned int cpu)
 
 	/* update pod affinity of unbound workqueues */
 	mutex_lock(&wq_pool_mutex);
+
+	cpumask_clear_cpu(cpu, wq_online_cpumask);
+
 	list_for_each_entry(wq, &workqueues, list) {
 		struct workqueue_attrs *attrs = wq->unbound_attrs;
 
@@ -7650,10 +7658,12 @@ void __init workqueue_init_early(void)
 
 	BUILD_BUG_ON(__alignof__(struct pool_workqueue) < __alignof__(long long));
 
+	BUG_ON(!alloc_cpumask_var(&wq_online_cpumask, GFP_KERNEL));
 	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));
 
+	cpumask_copy(wq_online_cpumask, cpu_online_mask);
 	cpumask_copy(wq_unbound_cpumask, cpu_possible_mask);
 	restrict_unbound_cpumask("HK_TYPE_WQ", housekeeping_cpumask(HK_TYPE_WQ));
 	restrict_unbound_cpumask("HK_TYPE_DOMAIN", housekeeping_cpumask(HK_TYPE_DOMAIN));
-- 
2.19.1.6.gb485710b

[PATCH 2/7] workqueue: Simplify wq_calc_pod_cpumask() with wq_online_cpumask

[Lai Jiangshan]

From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

Avoid relying on cpu_online_mask for wqattrs changes so that
cpus_read_lock() can be removed from apply_wqattrs_lock().

And with wq_online_cpumask, attrs->__pod_cpumask doesn't need to be
reused as a temporary storage to calculate if the pod have any online
CPUs @attrs wants since @cpu_going_down is not in the wq_online_cpumask.

Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
---
 kernel/workqueue.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 985ab9230fe0..9f454a9c04c8 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5156,20 +5156,14 @@ static void wq_calc_pod_cpumask(struct workqueue_attrs *attrs, int cpu,
 	const struct wq_pod_type *pt = wqattrs_pod_type(attrs);
 	int pod = pt->cpu_pod[cpu];
 
-	/* does @pod have any online CPUs @attrs wants? */
+	/* calculate possible CPUs in @pod that @attrs wants */
 	cpumask_and(attrs->__pod_cpumask, pt->pod_cpus[pod], attrs->cpumask);
-	cpumask_and(attrs->__pod_cpumask, attrs->__pod_cpumask, cpu_online_mask);
-	if (cpu_going_down >= 0)
-		cpumask_clear_cpu(cpu_going_down, attrs->__pod_cpumask);
-
-	if (cpumask_empty(attrs->__pod_cpumask)) {
+	/* does @pod have any online CPUs @attrs wants? */
+	if (!cpumask_intersects(attrs->__pod_cpumask, wq_online_cpumask)) {
 		cpumask_copy(attrs->__pod_cpumask, attrs->cpumask);
 		return;
 	}
 
-	/* yeap, return possible CPUs in @pod that @attrs wants */
-	cpumask_and(attrs->__pod_cpumask, attrs->cpumask, pt->pod_cpus[pod]);
-
 	if (cpumask_empty(attrs->__pod_cpumask))
 		pr_warn_once("WARNING: workqueue cpumask: online intersect > "
 				"possible intersect\n");
-- 
2.19.1.6.gb485710b

[PATCH 3/7] workqueue: Remove cpus_read_lock() from apply_wqattrs_lock()

[Lai Jiangshan]

From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

The pwq creations and installations have been reworked based on
wq_online_cpumask rather than cpu_online_mask.

So cpus_read_lock() is unneeded during wqattrs changes.

Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
---
 kernel/workqueue.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 9f454a9c04c8..64876d391e7c 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5123,15 +5123,12 @@ static struct pool_workqueue *alloc_unbound_pwq(struct workqueue_struct *wq,
 
 static void apply_wqattrs_lock(void)
 {
-	/* CPUs should stay stable across pwq creations and installations */
-	cpus_read_lock();
 	mutex_lock(&wq_pool_mutex);
 }
 
 static void apply_wqattrs_unlock(void)
 {
 	mutex_unlock(&wq_pool_mutex);
-	cpus_read_unlock();
 }
 
 /**
-- 
2.19.1.6.gb485710b

[PATCH 4/7] workqueue: Remove the unneeded cpumask empty check in wq_calc_pod_cpumask()

[Lai Jiangshan]

From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

The cpumask empty check in wq_calc_pod_cpumask() has long been useless.
It just works purely as documents which states that the cpumask is not
possible empty after the function returns.

Now the code above is even more explicit that the cpumask is not empty,
so the document-only empty check can be removed.

Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
---
 kernel/workqueue.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 64876d391e7c..01d5ea1af60a 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5160,10 +5160,6 @@ static void wq_calc_pod_cpumask(struct workqueue_attrs *attrs, int cpu,
 		cpumask_copy(attrs->__pod_cpumask, attrs->cpumask);
 		return;
 	}
-
-	if (cpumask_empty(attrs->__pod_cpumask))
-		pr_warn_once("WARNING: workqueue cpumask: online intersect > "
-				"possible intersect\n");
 }
 
 /* install @pwq into @wq and return the old pwq, @cpu < 0 for dfl_pwq */
-- 
2.19.1.6.gb485710b

[PATCH 5/7] workqueue: Remove the argument @cpu_going_down from wq_calc_pod_cpumask()

[Lai Jiangshan]

From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

wq_calc_pod_cpumask() uses wq_online_cpumask, which excludes the cpu
going down, so the argument cpu_going_down is unused and can be removed.

Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
---
 kernel/workqueue.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 01d5ea1af60a..97ddccf8cd0e 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5135,10 +5135,8 @@ static void apply_wqattrs_unlock(void)
  * wq_calc_pod_cpumask - calculate a wq_attrs' cpumask for a pod
  * @attrs: the wq_attrs of the default pwq of the target workqueue
  * @cpu: the target CPU
- * @cpu_going_down: if >= 0, the CPU to consider as offline
  *
- * Calculate the cpumask a workqueue with @attrs should use on @pod. If
- * @cpu_going_down is >= 0, that cpu is considered offline during calculation.
+ * Calculate the cpumask a workqueue with @attrs should use on @pod.
  * The result is stored in @attrs->__pod_cpumask.
  *
  * If pod affinity is not enabled, @attrs->cpumask is always used. If enabled
@@ -5147,8 +5145,7 @@ static void apply_wqattrs_unlock(void)
  *
  * The caller is responsible for ensuring that the cpumask of @pod stays stable.
  */
-static void wq_calc_pod_cpumask(struct workqueue_attrs *attrs, int cpu,
-				int cpu_going_down)
+static void wq_calc_pod_cpumask(struct workqueue_attrs *attrs, int cpu)
 {
 	const struct wq_pod_type *pt = wqattrs_pod_type(attrs);
 	int pod = pt->cpu_pod[cpu];
@@ -5244,7 +5241,7 @@ apply_wqattrs_prepare(struct workqueue_struct *wq,
 			ctx->dfl_pwq->refcnt++;
 			ctx->pwq_tbl[cpu] = ctx->dfl_pwq;
 		} else {
-			wq_calc_pod_cpumask(new_attrs, cpu, -1);
+			wq_calc_pod_cpumask(new_attrs, cpu);
 			ctx->pwq_tbl[cpu] = alloc_unbound_pwq(wq, new_attrs);
 			if (!ctx->pwq_tbl[cpu])
 				goto out_free;
@@ -5378,7 +5375,6 @@ int apply_workqueue_attrs(struct workqueue_struct *wq,
 static void wq_update_pod(struct workqueue_struct *wq, int cpu,
 			  int hotplug_cpu, bool online)
 {
-	int off_cpu = online ? -1 : hotplug_cpu;
 	struct pool_workqueue *old_pwq = NULL, *pwq;
 	struct workqueue_attrs *target_attrs;
 
@@ -5398,7 +5394,7 @@ static void wq_update_pod(struct workqueue_struct *wq, int cpu,
 	wqattrs_actualize_cpumask(target_attrs, wq_unbound_cpumask);
 
 	/* nothing to do if the target cpumask matches the current pwq */
-	wq_calc_pod_cpumask(target_attrs, cpu, off_cpu);
+	wq_calc_pod_cpumask(target_attrs, cpu);
 	if (wqattrs_equal(target_attrs, unbound_pwq(wq, cpu)->pool->attrs))
 		return;
 
-- 
2.19.1.6.gb485710b

[PATCH 6/7] workqueue: Remove the arguments @hotplug_cpu and @online from wq_update_pod()

[Lai Jiangshan]

From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

The arguments @hotplug_cpu and @online are not used in wq_update_pod()
since the functions called by wq_update_pod() don't need them.

Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
---
 kernel/workqueue.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 97ddccf8cd0e..21e1c5787ba4 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5354,8 +5354,6 @@ int apply_workqueue_attrs(struct workqueue_struct *wq,
  * wq_update_pod - update pod affinity of a wq for CPU hot[un]plug
  * @wq: the target workqueue
  * @cpu: the CPU to update pool association for
- * @hotplug_cpu: the CPU coming up or going down
- * @online: whether @cpu is coming up or going down
  *
  * This function is to be called from %CPU_DOWN_PREPARE, %CPU_ONLINE and
  * %CPU_DOWN_FAILED.  @cpu is being hot[un]plugged, update pod affinity of
@@ -5372,8 +5370,7 @@ int apply_workqueue_attrs(struct workqueue_struct *wq,
  * CPU_DOWN. If a workqueue user wants strict affinity, it's the user's
  * responsibility to flush the work item from CPU_DOWN_PREPARE.
  */
-static void wq_update_pod(struct workqueue_struct *wq, int cpu,
-			  int hotplug_cpu, bool online)
+static void wq_update_pod(struct workqueue_struct *wq, int cpu)
 {
 	struct pool_workqueue *old_pwq = NULL, *pwq;
 	struct workqueue_attrs *target_attrs;
@@ -6593,7 +6590,7 @@ int workqueue_online_cpu(unsigned int cpu)
 			int tcpu;
 
 			for_each_cpu(tcpu, pt->pod_cpus[pt->cpu_pod[cpu]])
-				wq_update_pod(wq, tcpu, cpu, true);
+				wq_update_pod(wq, tcpu);
 
 			mutex_lock(&wq->mutex);
 			wq_update_node_max_active(wq, -1);
@@ -6628,7 +6625,7 @@ int workqueue_offline_cpu(unsigned int cpu)
 			int tcpu;
 
 			for_each_cpu(tcpu, pt->pod_cpus[pt->cpu_pod[cpu]])
-				wq_update_pod(wq, tcpu, cpu, false);
+				wq_update_pod(wq, tcpu);
 
 			mutex_lock(&wq->mutex);
 			wq_update_node_max_active(wq, cpu);
@@ -6917,7 +6914,7 @@ static int wq_affn_dfl_set(const char *val, const struct kernel_param *kp)
 
 	list_for_each_entry(wq, &workqueues, list) {
 		for_each_online_cpu(cpu) {
-			wq_update_pod(wq, cpu, cpu, true);
+			wq_update_pod(wq, cpu);
 		}
 	}
 
@@ -7928,7 +7925,7 @@ void __init workqueue_init_topology(void)
 	 */
 	list_for_each_entry(wq, &workqueues, list) {
 		for_each_online_cpu(cpu)
-			wq_update_pod(wq, cpu, cpu, true);
+			wq_update_pod(wq, cpu);
 		if (wq->flags & WQ_UNBOUND) {
 			mutex_lock(&wq->mutex);
 			wq_update_node_max_active(wq, -1);
-- 
2.19.1.6.gb485710b

[PATCH 7/7] workqueue: Rename wq_update_pod() to unbound_wq_update_pwq()

[Lai Jiangshan]

From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

What wq_update_pod() does is just to update the pwq of the specific
cpu.  Rename it and update the comments.

Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
---
Avoiding to call unbound_wq_update_pwq() in wq_affn_dfl_set() is in my
todo list.  So the comments assuming unbound_wq_update_pwq() only to be
called in cpu hotplug path is unchanged.

 kernel/workqueue.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 21e1c5787ba4..cd6f2950ef6c 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -433,7 +433,7 @@ static struct wq_pod_type wq_pod_types[WQ_AFFN_NR_TYPES];
 static enum wq_affn_scope wq_affn_dfl = WQ_AFFN_CACHE;
 
 /* buf for wq_update_unbound_pod_attrs(), protected by CPU hotplug exclusion */
-static struct workqueue_attrs *wq_update_pod_attrs_buf;
+static struct workqueue_attrs *unbound_wq_update_pwq_attrs_buf;
 
 static DEFINE_MUTEX(wq_pool_mutex);	/* protects pools and workqueues list */
 static DEFINE_MUTEX(wq_pool_attach_mutex); /* protects worker attach/detach */
@@ -5351,13 +5351,12 @@ int apply_workqueue_attrs(struct workqueue_struct *wq,
 }
 
 /**
- * wq_update_pod - update pod affinity of a wq for CPU hot[un]plug
+ * unbound_wq_update_pwq - update a pwq slot for CPU hot[un]plug
  * @wq: the target workqueue
- * @cpu: the CPU to update pool association for
+ * @cpu: the CPU to update the pwq slot for
  *
  * This function is to be called from %CPU_DOWN_PREPARE, %CPU_ONLINE and
- * %CPU_DOWN_FAILED.  @cpu is being hot[un]plugged, update pod affinity of
- * @wq accordingly.
+ * %CPU_DOWN_FAILED.  @cpu is in the same pod of the CPU being hot[un]plugged.
  *
  *
  * If pod affinity can't be adjusted due to memory allocation failure, it falls
@@ -5370,7 +5369,7 @@ int apply_workqueue_attrs(struct workqueue_struct *wq,
  * CPU_DOWN. If a workqueue user wants strict affinity, it's the user's
  * responsibility to flush the work item from CPU_DOWN_PREPARE.
  */
-static void wq_update_pod(struct workqueue_struct *wq, int cpu)
+static void unbound_wq_update_pwq(struct workqueue_struct *wq, int cpu)
 {
 	struct pool_workqueue *old_pwq = NULL, *pwq;
 	struct workqueue_attrs *target_attrs;
@@ -5385,7 +5384,7 @@ static void wq_update_pod(struct workqueue_struct *wq, int cpu)
 	 * Let's use a preallocated one.  The following buf is protected by
 	 * CPU hotplug exclusion.
 	 */
-	target_attrs = wq_update_pod_attrs_buf;
+	target_attrs = unbound_wq_update_pwq_attrs_buf;
 
 	copy_workqueue_attrs(target_attrs, wq->unbound_attrs);
 	wqattrs_actualize_cpumask(target_attrs, wq_unbound_cpumask);
@@ -6590,7 +6589,7 @@ int workqueue_online_cpu(unsigned int cpu)
 			int tcpu;
 
 			for_each_cpu(tcpu, pt->pod_cpus[pt->cpu_pod[cpu]])
-				wq_update_pod(wq, tcpu);
+				unbound_wq_update_pwq(wq, tcpu);
 
 			mutex_lock(&wq->mutex);
 			wq_update_node_max_active(wq, -1);
@@ -6625,7 +6624,7 @@ int workqueue_offline_cpu(unsigned int cpu)
 			int tcpu;
 
 			for_each_cpu(tcpu, pt->pod_cpus[pt->cpu_pod[cpu]])
-				wq_update_pod(wq, tcpu);
+				unbound_wq_update_pwq(wq, tcpu);
 
 			mutex_lock(&wq->mutex);
 			wq_update_node_max_active(wq, cpu);
@@ -6913,9 +6912,8 @@ static int wq_affn_dfl_set(const char *val, const struct kernel_param *kp)
 	wq_affn_dfl = affn;
 
 	list_for_each_entry(wq, &workqueues, list) {
-		for_each_online_cpu(cpu) {
-			wq_update_pod(wq, cpu);
-		}
+		for_each_online_cpu(cpu)
+			unbound_wq_update_pwq(wq, cpu);
 	}
 
 	mutex_unlock(&wq_pool_mutex);
@@ -7654,8 +7652,8 @@ void __init workqueue_init_early(void)
 
 	pwq_cache = KMEM_CACHE(pool_workqueue, SLAB_PANIC);
 
-	wq_update_pod_attrs_buf = alloc_workqueue_attrs();
-	BUG_ON(!wq_update_pod_attrs_buf);
+	unbound_wq_update_pwq_attrs_buf = alloc_workqueue_attrs();
+	BUG_ON(!unbound_wq_update_pwq_attrs_buf);
 
 	/*
 	 * If nohz_full is enabled, set power efficient workqueue as unbound.
@@ -7920,12 +7918,12 @@ void __init workqueue_init_topology(void)
 
 	/*
 	 * Workqueues allocated earlier would have all CPUs sharing the default
-	 * worker pool. Explicitly call wq_update_pod() on all workqueue and CPU
-	 * combinations to apply per-pod sharing.
+	 * worker pool. Explicitly call unbound_wq_update_pwq() on all workqueue
+	 * and CPU combinations to apply per-pod sharing.
 	 */
 	list_for_each_entry(wq, &workqueues, list) {
 		for_each_online_cpu(cpu)
-			wq_update_pod(wq, cpu);
+			unbound_wq_update_pwq(wq, cpu);
 		if (wq->flags & WQ_UNBOUND) {
 			mutex_lock(&wq->mutex);
 			wq_update_node_max_active(wq, -1);
-- 
2.19.1.6.gb485710b

[PATCH UPDATED 3/7] workqueue: Remove cpus_read_lock() from apply_wqattrs_lock()

[Tejun Heo]

From 49d94fbe1f8c275c793e298e8a38278e90029cf6 Mon Sep 17 00:00:00 2001
From: Lai Jiangshan <jiangshan.ljs@antgroup.com>
Date: Thu, 11 Jul 2024 16:35:43 +0800
Subject: [PATCH] workqueue: Remove cpus_read_lock() from apply_wqattrs_lock()

1726a1713590 ("workqueue: Put PWQ allocation and WQ enlistment in the same
lock C.S.") led to the following possible deadlock:

  WARNING: possible recursive locking detected
  6.10.0-rc5-00004-g1d4c6111406c #1 Not tainted
   --------------------------------------------
   swapper/0/1 is trying to acquire lock:
   c27760f4 (cpu_hotplug_lock){++++}-{0:0}, at: alloc_workqueue (kernel/workqueue.c:5152 kernel/workqueue.c:5730)

   but task is already holding lock:
   c27760f4 (cpu_hotplug_lock){++++}-{0:0}, at: padata_alloc (kernel/padata.c:1007)
   ...
   stack backtrace:
   ...
   cpus_read_lock (include/linux/percpu-rwsem.h:53 kernel/cpu.c:488)
   alloc_workqueue (kernel/workqueue.c:5152 kernel/workqueue.c:5730)
   padata_alloc (kernel/padata.c:1007 (discriminator 1))
   pcrypt_init_padata (crypto/pcrypt.c:327 (discriminator 1))
   pcrypt_init (crypto/pcrypt.c:353)
   do_one_initcall (init/main.c:1267)
   do_initcalls (init/main.c:1328 (discriminator 1) init/main.c:1345 (discriminator 1))
   kernel_init_freeable (init/main.c:1364)
   kernel_init (init/main.c:1469)
   ret_from_fork (arch/x86/kernel/process.c:153)
   ret_from_fork_asm (arch/x86/entry/entry_32.S:737)
   entry_INT80_32 (arch/x86/entry/entry_32.S:944)

This is caused by pcrypt allocating a workqueue while holding
cpus_read_lock(), so workqueue code can't do it again as that can lead to
deadlocks if down_write starts after the first down_read.

The pwq creations and installations have been reworked based on
wq_online_cpumask rather than cpu_online_mask making cpus_read_lock() is
unneeded during wqattrs changes. Fix the deadlock by removing
cpus_read_lock() from apply_wqattrs_lock().

tj: Updated changelog.

Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
Fixes: 1726a1713590 ("workqueue: Put PWQ allocation and WQ enlistment in the same lock C.S.")
Link: http://lkml.kernel.org/r/202407081521.83b627c1-lkp@intel.com
Acked-by: Tejun Heo <tj@kernel.org>
---
 kernel/workqueue.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index a345a72395e7..9b7c1fcd934b 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5113,15 +5113,12 @@ static struct pool_workqueue *alloc_unbound_pwq(struct workqueue_struct *wq,
 
 static void apply_wqattrs_lock(void)
 {
-	/* CPUs should stay stable across pwq creations and installations */
-	cpus_read_lock();
 	mutex_lock(&wq_pool_mutex);
 }
 
 static void apply_wqattrs_unlock(void)
 {
 	mutex_unlock(&wq_pool_mutex);
-	cpus_read_unlock();
 }
 
 /**
-- 
2.45.2

Re: [PATCH 0/7] Add wq_online_cpumask and remove cpus_read_lock() from apply_wqattrs_lock()

[Tejun Heo]

On Thu, Jul 11, 2024 at 04:35:40PM +0800, Lai Jiangshan wrote:
> From: Lai Jiangshan <jiangshan.ljs@antgroup.com>
> 
> The new wq_online_mask mirrors the cpu_online_mask except during
> hotplugging; specifically, it differs between the hotplugging stages
> of workqueue_offline_cpu() and workqueue_online_cpu(), during which
> the transitioning CPU is not represented in the mask.
> 
> With wq_online_cpumask, cpus_read_lock() is unneeded for wqattrs changes.
> 
> 
> Lai Jiangshan (7):
>   workqueue: Add wq_online_cpumask
>   workqueue: Simplify wq_calc_pod_cpumask() with wq_online_cpumask
>   workqueue: Remove cpus_read_lock() from apply_wqattrs_lock()
>   workqueue: Remove the unneeded cpumask empty check in
>     wq_calc_pod_cpumask()
>   workqueue: Remove the argument @cpu_going_down from
>     wq_calc_pod_cpumask()
>   workqueue: Remove the arguments @hotplug_cpu and @online from
>     wq_update_pod()
>   workqueue: Rename wq_update_pod() to unbound_wq_update_pwq()

Applied 1-7 to wq/for-6.11. I updated a changelog to clarify why the change
is needed.

Thanks.

-- 
tejun

Re: [PATCH UPDATED 3/7] workqueue: Remove cpus_read_lock() from apply_wqattrs_lock()

[Daniel Jordan]

On Thu, Jul 11, 2024 at 07:11:16AM GMT, Tejun Heo wrote:
> From 49d94fbe1f8c275c793e298e8a38278e90029cf6 Mon Sep 17 00:00:00 2001
> From: Lai Jiangshan <jiangshan.ljs@antgroup.com>
> Date: Thu, 11 Jul 2024 16:35:43 +0800
> Subject: [PATCH] workqueue: Remove cpus_read_lock() from apply_wqattrs_lock()
> 
> 1726a1713590 ("workqueue: Put PWQ allocation and WQ enlistment in the same
> lock C.S.") led to the following possible deadlock:
> 
>   WARNING: possible recursive locking detected
>   6.10.0-rc5-00004-g1d4c6111406c #1 Not tainted
>    --------------------------------------------
>    swapper/0/1 is trying to acquire lock:
>    c27760f4 (cpu_hotplug_lock){++++}-{0:0}, at: alloc_workqueue (kernel/workqueue.c:5152 kernel/workqueue.c:5730)
> 
>    but task is already holding lock:
>    c27760f4 (cpu_hotplug_lock){++++}-{0:0}, at: padata_alloc (kernel/padata.c:1007)
>    ...
>    stack backtrace:
>    ...
>    cpus_read_lock (include/linux/percpu-rwsem.h:53 kernel/cpu.c:488)
>    alloc_workqueue (kernel/workqueue.c:5152 kernel/workqueue.c:5730)
>    padata_alloc (kernel/padata.c:1007 (discriminator 1))
>    pcrypt_init_padata (crypto/pcrypt.c:327 (discriminator 1))
>    pcrypt_init (crypto/pcrypt.c:353)
>    do_one_initcall (init/main.c:1267)
>    do_initcalls (init/main.c:1328 (discriminator 1) init/main.c:1345 (discriminator 1))
>    kernel_init_freeable (init/main.c:1364)
>    kernel_init (init/main.c:1469)
>    ret_from_fork (arch/x86/kernel/process.c:153)
>    ret_from_fork_asm (arch/x86/entry/entry_32.S:737)
>    entry_INT80_32 (arch/x86/entry/entry_32.S:944)
> 
> This is caused by pcrypt allocating a workqueue while holding
> cpus_read_lock(), so workqueue code can't do it again as that can lead to
> deadlocks if down_write starts after the first down_read.
> 
> The pwq creations and installations have been reworked based on
> wq_online_cpumask rather than cpu_online_mask making cpus_read_lock() is
> unneeded during wqattrs changes. Fix the deadlock by removing
> cpus_read_lock() from apply_wqattrs_lock().

pcrypt/padata doesn't need to hold cpus_read_lock during
alloc_workqueue.

I didn't look closely at the workqueue changes that avoid this issue,
but I can remove the restriction in padata if it helps to reduce
complexity in workqueue.

I saw the recent wq pull though, so if it's fine as is, that's ok too.

Re: [PATCH UPDATED 3/7] workqueue: Remove cpus_read_lock() from apply_wqattrs_lock()

[Tejun Heo]

Hello, Daniel.

On Mon, Jul 15, 2024 at 11:13:56AM -0400, Daniel Jordan wrote:
...
> pcrypt/padata doesn't need to hold cpus_read_lock during
> alloc_workqueue.
> 
> I didn't look closely at the workqueue changes that avoid this issue,
> but I can remove the restriction in padata if it helps to reduce
> complexity in workqueue.
> 
> I saw the recent wq pull though, so if it's fine as is, that's ok too.

I think it'd be better if workqueue can put as little restrictions as
possible to its users. alloc_workqueue() is a bit of a boundary case, I
think, but if workqueue can avoid it, I think it's better that way, so no
need to change it from pcrypt/padata.

Thanks.

-- 
tejun

Re: [PATCH 3/7] workqueue: Remove cpus_read_lock() from apply_wqattrs_lock()

[Pengfei Xu]

Hi Jiangshan and all,

Greetings!

I found the WARNING in alloc_workqueue in "next-20240715" tag by local
syzkaller:

Found the first bad commit:
19af45757383 workqueue: Remove cpus_read_lock() from apply_wqattrs_lock()
Reverted this commit and this issue was gone.

All detailed info: https://github.com/xupengfe/syzkaller_logs/tree/main/240715_174449_alloc_workqueue
Syzkaller reproduced code: https://github.com/xupengfe/syzkaller_logs/blob/main/240715_174449_alloc_workqueue/repro.c
Syzkaller repro syscall:https://github.com/xupengfe/syzkaller_logs/blob/main/240715_174449_alloc_workqueue/repro.prog
Mount image: https://github.com/xupengfe/syzkaller_logs/raw/main/240715_174449_alloc_workqueue/mount_0.gz
Kconfig(make olddefconfig):https://github.com/xupengfe/syzkaller_logs/blob/main/240715_174449_alloc_workqueue/kconfig_origin
Bisect info: https://github.com/xupengfe/syzkaller_logs/blob/main/240715_174449_alloc_workqueue/bisect_info.log
bzImage: https://github.com/xupengfe/syzkaller_logs/raw/main/240715_174449_alloc_workqueue/bzImage_91e3b24eb7d297d9d99030800ed96944b8652eaf.tar.gz
Issue dmesg: https://github.com/xupengfe/syzkaller_logs/blob/main/240715_174449_alloc_workqueue/91e3b24eb7d297d9d99030800ed96944b8652eaf_dmesg.log

"
[   30.328518] ------------[ cut here ]------------
[   30.329306] WARNING: CPU: 1 PID: 733 at kernel/cpu.c:527 lockdep_assert_cpus_held+0xd3/0x100
[   30.329339] Modules linked in:
[   30.329347] CPU: 1 UID: 0 PID: 733 Comm: repro Not tainted 6.10.0-next-20240715-91e3b24eb7d2-dirty #1
[   30.329364] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
[   30.329372] RIP: 0010:lockdep_assert_cpus_held+0xd3/0x100
[   30.329391] Code: e8 02 cc 3e 00 be ff ff ff ff 48 c7 c7 d0 67 f0 86 e8 31 65 56 04 31 ff 89 c3 89 c6 e8 26 c7 3e 00 85 db 75 cc e8 dd cb 3e 00 <0f> 0b eb c3 48 c7 c7 44 ed c0 87 e8 bd 58 a4 00 e9 59 ff ff ff 48
[   30.329404] RSP: 0018:ffff8880142cf8d0 EFLAGS: 00010293
[   30.329415] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffffff81275c6a
[   30.329424] RDX: ffff888010a70000 RSI: ffffffff81275c73 RDI: 0000000000000005
[   30.329432] RBP: ffff8880142cf8d8 R08: 0000000000000000 R09: fffffbfff0f82dbc
[   30.329440] R10: 0000000000000000 R11: 0000000000000001 R12: ffff88800e54f9d8
[   30.329448] R13: ffff88800e54f820 R14: ffff88801349fc00 R15: ffff88800e54f800
[   30.329456] FS:  00007fa859b71740(0000) GS:ffff88806c500000(0000) knlGS:0000000000000000
[   30.329467] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   30.329476] CR2: 00007fa85143f000 CR3: 000000001f226005 CR4: 0000000000770ef0
[   30.329488] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   30.329497] DR3: 0000000000000000 DR6: 00000000ffff07f0 DR7: 0000000000000400
[   30.329505] PKRU: 55555554
[   30.329509] Call Trace:
[   30.329513]  <TASK>
[   30.329520]  ? show_regs+0xa8/0xc0
[   30.329544]  ? __warn+0xf3/0x380
[   30.329562]  ? report_bug+0x25e/0x4b0
[   30.329590]  ? lockdep_assert_cpus_held+0xd3/0x100
[   30.329611]  ? report_bug+0x2cb/0x4b0
[   30.329627]  ? alloc_workqueue+0x920/0x1940
[   30.329645]  ? lockdep_assert_cpus_held+0xd3/0x100
[   30.329665]  ? handle_bug+0xa2/0x130
[   30.329689]  ? exc_invalid_op+0x3c/0x80
[   30.329713]  ? asm_exc_invalid_op+0x1f/0x30
[   30.329741]  ? lockdep_assert_cpus_held+0xca/0x100
[   30.329770]  ? lockdep_assert_cpus_held+0xd3/0x100
[   30.329790]  ? lockdep_assert_cpus_held+0xd3/0x100
[   30.329813]  alloc_workqueue+0x9b0/0x1940
[   30.329847]  ? __pfx_alloc_workqueue+0x10/0x10
[   30.329871]  ? __fget_files+0x253/0x4c0
[   30.329890]  ? __sanitizer_cov_trace_switch+0x58/0xa0
[   30.329922]  loop_configure+0xbb2/0x11f0
[   30.329960]  lo_ioctl+0x930/0x1aa0
[   30.329982]  ? __pfx_mark_lock.part.0+0x10/0x10
[   30.329997]  ? __lock_acquire+0xd87/0x5c90
[   30.330020]  ? __pfx_lo_ioctl+0x10/0x10
[   30.330053]  ? __pfx___lock_acquire+0x10/0x10
[   30.330074]  ? __kasan_check_read+0x15/0x20
[   30.330091]  ? __lock_acquire+0x1b0f/0x5c90
[   30.330116]  ? __sanitizer_cov_trace_switch+0x58/0xa0
[   30.330195]  ? __this_cpu_preempt_check+0x21/0x30
[   30.330216]  ? __pfx_lo_ioctl+0x10/0x10
[   30.330239]  blkdev_ioctl+0x2a9/0x6b0
[   30.330255]  ? __pfx_blkdev_ioctl+0x10/0x10
[   30.330271]  ? __sanitizer_cov_trace_const_cmp4+0x1a/0x20
[   30.330290]  ? security_file_ioctl+0x9d/0xd0
[   30.330360]  ? __pfx_blkdev_ioctl+0x10/0x10
[   30.330376]  __x64_sys_ioctl+0x1b9/0x230
[   30.330397]  x64_sys_call+0x1227/0x2140
[   30.330414]  do_syscall_64+0x6d/0x140
[   30.330434]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   30.330450] RIP: 0033:0x7fa85983ec6b
[   30.330462] Code: 73 01 c3 48 8b 0d b5 b1 1b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 85 b1 1b 00 f7 d8 64 89 01 48
[   30.330474] RSP: 002b:00007fff65352a38 EFLAGS: 00000217 ORIG_RAX: 0000000000000010
[   30.330487] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fa85983ec6b
[   30.330495] RDX: 0000000000000003 RSI: 0000000000004c00 RDI: 0000000000000004
[   30.330503] RBP: 00007fff65352a70 R08: 00000000ffffffff R09: 0000000000000000
[   30.330511] R10: 0000000000000000 R11: 0000000000000217 R12: 00007fff65352dc8
[   30.330519] R13: 0000000000402cea R14: 0000000000404e08 R15: 00007fa859bbc000
[   30.330553]  </TASK>
[   30.330558] irq event stamp: 1577
[   30.330562] hardirqs last  enabled at (1579): [<ffffffff81457403>] vprintk_store+0x413/0xa90
[   30.330586] hardirqs last disabled at (1580): [<ffffffff8145778a>] vprintk_store+0x79a/0xa90
[   30.330608] softirqs last  enabled at (730): [<ffffffff81289719>] __irq_exit_rcu+0xa9/0x120
[   30.330624] softirqs last disabled at (715): [<ffffffff81289719>] __irq_exit_rcu+0xa9/0x120
[   30.330639] ---[ end trace 0000000000000000 ]---
"

I hope it's helpful.

Thank you!

---

If you don't need the following environment to reproduce the problem or if you
already have one reproduced environment, please ignore the following information.

How to reproduce:
git clone https://gitlab.com/xupengfe/repro_vm_env.git
cd repro_vm_env
tar -xvf repro_vm_env.tar.gz
cd repro_vm_env; ./start3.sh  // it needs qemu-system-x86_64 and I used v7.1.0
  // start3.sh will load bzImage_2241ab53cbb5cdb08a6b2d4688feb13971058f65 v6.2-rc5 kernel
  // You could change the bzImage_xxx as you want
  // Maybe you need to remove line "-drive if=pflash,format=raw,readonly=on,file=./OVMF_CODE.fd \" for different qemu version
You could use below command to log in, there is no password for root.
ssh -p 10023 root@localhost

After login vm(virtual machine) successfully, you could transfer reproduced
binary to the vm by below way, and reproduce the problem in vm:
gcc -pthread -o repro repro.c
scp -P 10023 repro root@localhost:/root/

Get the bzImage for target kernel:
Please use target kconfig and copy it to kernel_src/.config
make olddefconfig
make -jx bzImage           //x should equal or less than cpu num your pc has

Fill the bzImage file into above start3.sh to load the target kernel in vm.


Tips:
If you already have qemu-system-x86_64, please ignore below info.
If you want to install qemu v7.1.0 version:
git clone https://github.com/qemu/qemu.git
cd qemu
git checkout -f v7.1.0
mkdir build
cd build
yum install -y ninja-build.x86_64
yum -y install libslirp-devel.x86_64
../configure --target-list=x86_64-softmmu --enable-kvm --enable-vnc --enable-gtk --enable-sdl --enable-usb-redir --enable-slirp
make
make install

Best Regards,
Thanks!


On 2024-07-11 at 16:35:43 +0800, Lai Jiangshan wrote:
> From: Lai Jiangshan <jiangshan.ljs@antgroup.com>
> 
> The pwq creations and installations have been reworked based on
> wq_online_cpumask rather than cpu_online_mask.
> 
> So cpus_read_lock() is unneeded during wqattrs changes.
> 
> Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
> ---
>  kernel/workqueue.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 9f454a9c04c8..64876d391e7c 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -5123,15 +5123,12 @@ static struct pool_workqueue *alloc_unbound_pwq(struct workqueue_struct *wq,
>  
>  static void apply_wqattrs_lock(void)
>  {
> -	/* CPUs should stay stable across pwq creations and installations */
> -	cpus_read_lock();
>  	mutex_lock(&wq_pool_mutex);
>  }
>  
>  static void apply_wqattrs_unlock(void)
>  {
>  	mutex_unlock(&wq_pool_mutex);
> -	cpus_read_unlock();
>  }
>  
>  /**
> -- 
> 2.19.1.6.gb485710b
>