cgroup/for-next: WARNING: possible circular locking dependency detected in cpuset_write_resmask

CPU1

cpus_read_lock+0x3e/0x80
static_key_slow_inc+0xe/0xa0
cpuset_css_online+0x62/0x330
online_css+0x26/0x80
cgroup_apply_control_enable+0x266/0x3d0
cgroup_mkdir+0x37d/0x4f0
kernfs_iop_mkdir+0x53/0x80
vfs_mkdir+0x10e/0x1a0
SyS_mkdirat+0xb3/0xe0
entry_SYSCALL_64_fastpath+0x23/0x9a

CPU0

lock_acquire+0xec/0x1e0
__mutex_lock+0x89/0x920
cpuset_write_resmask+0x61/0x1100
cgroup_file_write+0x7b/0x200
kernfs_fop_write+0x112/0x1a0
__vfs_write+0x23/0x150
vfs_write+0xc8/0x1c0
SyS_write+0x45/0xa0
entry_SYSCALL_64_fastpath+0x23/0x9a

CPU0 CPU1
---- ----

lock(cpu_hotplug_lock.rw_sem);
                                lock(cpuset_mutex);
                                lock(cpu_hotplug_lock.rw_sem);

lock(cpuset_mutex);

Change locking order of cpu_hotplug_lock.rw_sem and cpuset_mutex in cpuset_css_online(). Use _cpuslocked() version for static_branch_inc/static_branch_dec in cpuset_inc()/cpuset_dec().

Signed-off-by Prateek Sood <[email protected]>

---
 include/linux/cpuset.h     |  8 ++++----
 include/linux/jump_label.h | 10 ++++++++--
 kernel/cgroup/cpuset.c     |  4 ++--
 kernel/jump_label.c        | 13 +++++++++++++
 4 files changed, 27 insertions(+), 8 deletions(-)

include/linux/cpuset.h +4 -4

index 2ab910f..5aadc25 100644
@@ -40,14 +40,14 @@ static inline bool cpusets_enabled(void)
 
 static inline void cpuset_inc(void)
 {
-	static_branch_inc(&cpusets_pre_enable_key);
-	static_branch_inc(&cpusets_enabled_key);
+	static_branch_inc_cpuslocked(&cpusets_pre_enable_key);
+	static_branch_inc_cpuslocked(&cpusets_enabled_key);
 }
 
 static inline void cpuset_dec(void)
 {
-	static_branch_dec(&cpusets_enabled_key);
-	static_branch_dec(&cpusets_pre_enable_key);
+	static_branch_dec_cpuslocked(&cpusets_enabled_key);
+	static_branch_dec_cpuslocked(&cpusets_pre_enable_key);
 }
 
 extern int cpuset_init(void);

include/linux/jump_label.h +8 -2

index c7b368c..890e21c 100644
@@ -160,6 +160,8 @@ extern void arch_jump_label_transform_static(struct jump_entry *entry,
 extern int jump_label_text_reserved(void *start, void *end);
 extern void static_key_slow_inc(struct static_key *key);
 extern void static_key_slow_dec(struct static_key *key);
+extern void static_key_slow_incr_cpuslocked(struct static_key *key);
+extern void static_key_slow_decr_cpuslocked(struct static_key *key);
 extern void jump_label_apply_nops(struct module *mod);
 extern int static_key_count(struct static_key *key);
 extern void static_key_enable(struct static_key *key);

@@ -259,6 +261,8 @@ static inline void static_key_disable(struct static_key *key)
 
 #define static_key_enable_cpuslocked(k)		static_key_enable((k))
 #define static_key_disable_cpuslocked(k)	static_key_disable((k))
+#define static_key_slow_incr_cpuslocked(k)	static_key_slow_inc((k))
+#define static_key_slow_decr_cpuslocked(k)	static_key_slow_dec((k))
 
 #define STATIC_KEY_INIT_TRUE	{ .enabled = ATOMIC_INIT(1) }
 #define STATIC_KEY_INIT_FALSE	{ .enabled = ATOMIC_INIT(0) }

@@ -414,8 +418,10 @@ struct static_key_false {
  * Advanced usage; refcount, branch is enabled when: count != 0
  */
 
-#define static_branch_inc(x)		static_key_slow_inc(&(x)->key)
-#define static_branch_dec(x)		static_key_slow_dec(&(x)->key)
+#define static_branch_inc(x)			static_key_slow_inc(&(x)->key)
+#define static_branch_dec(x)			static_key_slow_dec(&(x)->key)
+#define static_branch_inc_cpuslocked(x)		static_key_slow_incr_cpuslocked(&(x)->key)
+#define static_branch_dec_cpuslocked(x)		static_key_slow_decr_cpuslocked(&(x)->key)
 
 /*
  * Normal usage; boolean enable/disable.

kernel/cgroup/cpuset.c +2 -2

index 227bc25..4ad8bae 100644
@@ -1985,7 +1985,7 @@ static int cpuset_css_online(struct cgroup_subsys_state *css)
 	if (!parent)
 		return 0;
 
-	mutex_lock(&cpuset_mutex);
+	cpuset_sched_change_begin();
 
 	set_bit(CS_ONLINE, &cs->flags);
 	if (is_spread_page(parent))

@@ -2034,7 +2034,7 @@ static int cpuset_css_online(struct cgroup_subsys_state *css)
 	cpumask_copy(cs->effective_cpus, parent->cpus_allowed);
 	spin_unlock_irq(&callback_lock);
 out_unlock:
-	mutex_unlock(&cpuset_mutex);
+	cpuset_sched_change_end();
 	return 0;
 }
 

kernel/jump_label.c +6 -0

index 8594d24..dde0eaa 100644
@@ -126,6 +126,12 @@ void static_key_slow_inc(struct static_key *key)
 }
 EXPORT_SYMBOL_GPL(static_key_slow_inc);
 
+void static_key_slow_incr_cpuslocked(struct static_key *key)
+{
+	static_key_slow_inc_cpuslocked(key);
+}
+EXPORT_SYMBOL_GPL(static_key_slow_incr_cpuslocked);
+
 void static_key_enable_cpuslocked(struct static_key *key)
 {
 	STATIC_KEY_CHECK_USE(key);

@@ -229,6 +235,13 @@ void static_key_slow_dec(struct static_key *key)
 }
 EXPORT_SYMBOL_GPL(static_key_slow_dec);
 

Any feedback/suggestion for this patch?

Hello,

Quoted 1 line · Prateek Sood

Any feedback/suggestion for this patch?

Sorry about the delay. I'm a bit worried because it feels like we're chasing a squirrel. I'll think through the recent changes and this one and get back to you.

Hello, again.

Quoted 8 lines · Tejun Heo

Hello,

Quoted 1 line · Prateek Sood

Any feedback/suggestion for this patch?

Sorry about the delay. I'm a bit worried because it feels like we're chasing a squirrel. I'll think through the recent changes and this one and get back to you.

Can you please take a look at the following pending commit?

https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git/commit/?h=for-4.15-fixes&id=e8b3f8db7aad99fcc5234fc5b89984ff6620de3d

AFAICS, this should remove the circular dependency you originally reported. I'll revert the two cpuset commits for now.

Quoted 14 lines · Tejun Heo

Hello, again.

Quoted 8 lines · Tejun Heo

Hello,

Quoted 1 line · Prateek Sood

Any feedback/suggestion for this patch?

Sorry about the delay. I'm a bit worried because it feels like we're chasing a squirrel. I'll think through the recent changes and this one and get back to you.

Can you please take a look at the following pending commit?

Quoted 4 lines

https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git/commit/?h=for-4.15-fixes&id=e8b3f8db7aad99fcc5234fc5b89984ff6620de3d

AFAICS, this should remove the circular dependency you originally reported. I'll revert the two cpuset commits for now.

So I liked his patches in that we would be able to go back to synchronous sched_domain building.

Quoted 20 lines · Peter Zijlstra

Quoted 18 lines · Tejun Heo

Hello, again.

Quoted 8 lines · Tejun Heo

Hello,

Quoted 1 line · Prateek Sood

Any feedback/suggestion for this patch?

Sorry about the delay. I'm a bit worried because it feels like we're chasing a squirrel. I'll think through the recent changes and this one and get back to you.

Can you please take a look at the following pending commit?

https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git/commit/?h=for-4.15-fixes&id=e8b3f8db7aad99fcc5234fc5b89984ff6620de3d

AFAICS, this should remove the circular dependency you originally reported. I'll revert the two cpuset commits for now.

Quoted 3 lines

So I liked his patches in that we would be able to go back to synchronous sched_domain building.

https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git/commit/?h=for-4.15-fixes&id=e8b3f8db7aad99fcc5234fc5b89984ff6620de3d

This will fix the original circular locking dependency issue. I will let you both (Peter & TJ) to decide on which one to pick.

There is one deadlock issue during cgroup migration from cpu hotplug path when a task T is being moved from source to destination cgroup.

kworker/0:0
cpuset_hotplug_workfn()
cpuset_hotplug_update_tasks()
hotplug_update_tasks_legacy()
remove_tasks_in_empty_cpuset()
cgroup_transfer_tasks() // stuck in iterator loop
cgroup_migrate()
cgroup_migrate_add_task()

In cgroup_migrate_add_task() it checks for PF_EXITING flag of task T. Task T will not migrate to destination cgroup. css_task_iter_start() will keep pointing to task T in loop waiting for task T cg_list node to be removed.

Hello, Prateek.

Quoted 13 lines · Prateek Sood

There is one deadlock issue during cgroup migration from cpu hotplug path when a task T is being moved from source to destination cgroup.

kworker/0:0
cpuset_hotplug_workfn()
cpuset_hotplug_update_tasks()
hotplug_update_tasks_legacy()
remove_tasks_in_empty_cpuset()
cgroup_transfer_tasks() // stuck in iterator loop
cgroup_migrate()
cgroup_migrate_add_task()

Quoted 4 lines

In cgroup_migrate_add_task() it checks for PF_EXITING flag of task T. Task T will not migrate to destination cgroup. css_task_iter_start() will keep pointing to task T in loop waiting for task T cg_list node to be removed.

Heh, that's a bug in cgroup_transfer_tasks() which happened because I forgot to update when we changed how we handle exiting tasks. The right thing to do here is making cgroup_transfer_tasks() repeat iff there were a valid migration target which didn't get transferred.

Thanks.

Did you mean something like below. If not then could you please share a patch for this problem in cgroup_transfer_tasks().

Hello, Prateek.

Quoted 3 lines · Prateek Sood

Did you mean something like below. If not then could you please share a patch for this problem in cgroup_transfer_tasks().

Oh we surely can add a new iterator but we can just count in cgroup_transfer_tasks() too, right?

Thanks.

Following are two ways to improve cgroup_transfer_tasks(). In both cases task in PF_EXITING state would be left in source cgroup. It would be removed from cgroup_exit() in exit path.

kernel/cgroup/cgroup-v1.c +0 -0

index 024085d..e2bdcdb 100644

Hello, Prateek.

Quoted 9 lines · Prateek Sood

Following are two ways to improve cgroup_transfer_tasks(). In both cases task in PF_EXITING state would be left in source cgroup. It would be removed from cgroup_exit() in exit path.

kernel/cgroup/cgroup-v1.c +0 -0

index 024085d..e2bdcdb 100644

kernel/cgroup/cgroup-v1.c int cgroup_transfer_tasks(struct cgroup *to, struct cgroup *from) · line 129

Quoted 8 lines

kernel/cgroup/cgroup-v1.c +4 -1

@@ -123,7 +123,10 @@ int cgroup_transfer_tasks(struct cgroup *to, struct cgroup *from)
         */
        do {
                css_task_iter_start(&from->self, 0, &it);
-               task = css_task_iter_next(&it);
+               do {
+                       task = css_task_iter_next(&it);
+               } while (task && (task & PF_EXITING))
+

Yeah, this looks good to me. We can't just make a single pass as in the other one because we can race aginst fork. And PF_EXITING being left behind is what was happening previously too anyway. They can't be moved.

Thanks.

Deadlock during cgroup migration from cpu hotplug path when a task T is being moved from source to destination cgroup.

Task T inserted a work item in the worklist of cpu0 low priority worker pool. It is waiting for expedited grace period work item to execute. This work item will only be executed once kworker/0:0 complete execution of cpuset_hotplug_workfn().

kworker/0:0 ==> Task T ==>kworker/0:0

In case of PF_EXITING task being migrated from source to destination cgroup, migrate next available task in source cgroup.

Change-Id I8874fb04479c136cae4dabd5c168c7749df66664

Signed-off-by Prateek Sood <[email protected]>

Quoted 2 lines

AFAICS, this should remove the circular dependency you originally reported. I'll revert the two cpuset commits for now.

So I liked his patches in that we would be able to go back to synchronous sched_domain building.

Hello, Peter.

Quoted 5 lines · Peter Zijlstra

Quoted 2 lines

AFAICS, this should remove the circular dependency you originally reported. I'll revert the two cpuset commits for now.

So I liked his patches in that we would be able to go back to synchronous sched_domain building.

Ah, yeah, that's a separate issue but didn't we intentionally make that asynchronous? IIRC, cpuset migration can take a really long time when the memory migration is turned on and doing that synchronously could mess up the system.

Thanks.

This change makes the usage of cpuset_hotplug_workfn() from cpu hotplug path synchronous. For memory hotplug it still remains asynchronous.

Memory migration happening from cpuset_hotplug_workfn() is already asynchronous by queuing cpuset_migrate_mm_workfn() in cpuset_migrate_mm_wq.

cpuset_hotplug_workfn()
cpuset_hotplug_workfn(()
cpuset_migrate_mm()
queue_work(cpuset_migrate_mm_wq)

It seems that memory migration latency might not have impact with this change.

Please let me know if you meant something else by cpuset migration taking time when memory migration is turned on.

Hello, Prateek.

Quoted 3 lines · Prateek Sood

This change makes the usage of cpuset_hotplug_workfn() from cpu hotplug path synchronous. For memory hotplug it still remains asynchronous.

Ah, right.

Quoted 9 lines

Memory migration happening from cpuset_hotplug_workfn() is already asynchronous by queuing cpuset_migrate_mm_workfn() in cpuset_migrate_mm_wq.

cpuset_hotplug_workfn()
cpuset_hotplug_workfn(()
cpuset_migrate_mm()
queue_work(cpuset_migrate_mm_wq)

Quoted 5 lines

It seems that memory migration latency might not have impact with this change.

Please let me know if you meant something else by cpuset migration taking time when memory migration is turned on.

No, I didn't. I was just confused about which part became synchronous. So, I don't have anything against making the cpu part synchronous, but let's not do that as the fix to the deadlocks cuz, while we can avoid them by changing cpuset, I don't think cpuset is the root cause for them. If there are benefits to making cpuset cpu migration synchronous, let's do that for those benefits.

Thanks.

Hello, Prateek.

Quoted 3 lines · Prateek Sood

This change makes the usage of cpuset_hotplug_workfn() from cpu hotplug path synchronous. For memory hotplug it still remains asynchronous.

Ah, right.

Quoted 9 lines

Memory migration happening from cpuset_hotplug_workfn() is already asynchronous by queuing cpuset_migrate_mm_workfn() in cpuset_migrate_mm_wq.

cpuset_hotplug_workfn()
cpuset_hotplug_workfn(()
cpuset_migrate_mm()
queue_work(cpuset_migrate_mm_wq)

Quoted 5 lines

It seems that memory migration latency might not have impact with this change.

Please let me know if you meant something else by cpuset migration taking time when memory migration is turned on.

No, I didn't. I was just confused about which part became synchronous. So, I don't have anything against making the cpu part synchronous, but let's not do that as the fix to the deadlocks cuz, while we can avoid them by changing cpuset, I don't think cpuset is the root cause for them. If there are benefits to making cpuset cpu migration synchronous, let's do that for those benefits.

Thanks.

task T is waiting for cpuset_mutex acquired
by kworker/2:1

sh ==> cpuhp/2 ==> kworker/2:1 ==> sh

kworker/2:3 ==> kthreadd ==> Task T ==> kworker/2:1

It seems that my earlier patch set should fix this scenario:
1) Inverting locking order of cpuset_mutex and cpu_hotplug_lock.
2) Make cpuset hotplug work synchronous.

Could you please share your feedback.

Hello,

Quoted 11 lines · Prateek Sood

task T is waiting for cpuset_mutex acquired
by kworker/2:1

sh ==> cpuhp/2 ==> kworker/2:1 ==> sh

kworker/2:3 ==> kthreadd ==> Task T ==> kworker/2:1

It seems that my earlier patch set should fix this scenario:
1) Inverting locking order of cpuset_mutex and cpu_hotplug_lock.
2) Make cpuset hotplug work synchronous.

Quoted 1 line

Could you please share your feedback.

Hmm... this can also be resolved by adding WQ_MEM_RECLAIM to the synchronize rcu workqueue, right? Given the wide-spread usages of synchronize_rcu and friends, maybe that's the right solution, or at least something we also need to do, for this particular deadlock?

Again, I don't have anything against making the domain rebuliding part of cpuset operations synchronous and these tricky deadlock scenarios do indicate that doing so would probably be beneficial. That said, tho, these scenarios seem more of manifestations of other problems exposed through kthreadd dependency than anything else.

Thanks.

Signature · 1 line

tejun

Quoted 14 lines · Tejun Heo

Hello,

Quoted 11 lines · Prateek Sood

task T is waiting for cpuset_mutex acquired
by kworker/2:1

sh ==> cpuhp/2 ==> kworker/2:1 ==> sh

kworker/2:3 ==> kthreadd ==> Task T ==> kworker/2:1

It seems that my earlier patch set should fix this scenario:
1) Inverting locking order of cpuset_mutex and cpu_hotplug_lock.
2) Make cpuset hotplug work synchronous.

Quoted 6 lines

Quoted 1 line

Could you please share your feedback.

Hmm... this can also be resolved by adding WQ_MEM_RECLAIM to the synchronize rcu workqueue, right? Given the wide-spread usages of synchronize_rcu and friends, maybe that's the right solution, or at least something we also need to do, for this particular deadlock?

To make WQ_MEM_RECLAIM work, I need to dynamically allocate RCU's workqueues, correct? Or is there some way to mark a statically allocated workqueue as WQ_MEM_RECLAIM after the fact?

I can dynamically allocate them, but I need to carefully investigate boot-time use. So if it is possible to be lazy, I do want to take the easy way out. ;-)

Actually, after taking a quick look, could you please supply me with a way of mark a statically allocated workqueue as WQ_MEM_RECLAIM after the fact? Otherwise, I end up having to check for the workqueue having

been allocated pretty much each time I use it, which is going to be an open invitation for bugs. Plus it looks like there are ways that RCU's workqueue wakeups can be executed during very early boot, which can be handled, but again in a rather messy fashion.

In contrast, given a way of mark a statically allocated workqueue as WQ_MEM_RECLAIM after the fact, I simply continue initializing the workqueue at early boot, and then add the WQ_MEM_RECLAIM marking some arbitrarily chosen time after the scheduler has been initialized.

The required change to workqueues looks easy, just move the body of the "if (flags & WQ_MEM_RECLAIM) {" statement in __alloc_workqueue_key() to a separate function, right?

Hello, Paul.

Sorry about the delay. Travel followed by cold. :(

Quoted 3 lines · Paul E. McKenney

Actually, after taking a quick look, could you please supply me with a way of mark a statically allocated workqueue as WQ_MEM_RECLAIM after the fact? Otherwise, I end up having to check for the workqueue having

Hmmm... there is no statically allocated workqueue tho. If you're referring to the system-wide workqueues (system*_wq), they're just created dynamically early during boot.

Quoted 5 lines

been allocated pretty much each time I use it, which is going to be an open invitation for bugs. Plus it looks like there are ways that RCU's workqueue wakeups can be executed during very early boot, which can be handled, but again in a rather messy fashion.

Quoted 8 lines

In contrast, given a way of mark a statically allocated workqueue as WQ_MEM_RECLAIM after the fact, I simply continue initializing the workqueue at early boot, and then add the WQ_MEM_RECLAIM marking some arbitrarily chosen time after the scheduler has been initialized.

The required change to workqueues looks easy, just move the body of the "if (flags & WQ_MEM_RECLAIM) {" statement in __alloc_workqueue_key() to a separate function, right?

Ah, okay, yes, currently, workqueue init is kinda silly in that while it allows init of non-mem-reclaiming workqueues way before workqueue is actually online, it doesn't allow the same for mem-reclaiming ones. As you pointed out, it's just an oversight on my part as the init path split was done initially to accomodate early init of system workqueues.

I'll update the code so that rescuers can be added later too; however, please note that while the work items may be queued, they won't be executed until workqueue_init() is run (the same as now) as there can't be worker threads anyway before that point.

Quoted 4 lines · Tejun Heo

Hello, Paul.

Sorry about the delay. Travel followed by cold. :(

Quoted 8 lines

Quoted 3 lines · Paul E. McKenney

Actually, after taking a quick look, could you please supply me with a way of mark a statically allocated workqueue as WQ_MEM_RECLAIM after the fact? Otherwise, I end up having to check for the workqueue having

Hmmm... there is no statically allocated workqueue tho. If you're referring to the system-wide workqueues (system*_wq), they're just created dynamically early during boot.

Good point, I was confused. But yes, they are conveniently allocated just before the call to rcu_init(), which does work out well. ;-)

Quoted 21 lines

Quoted 5 lines

been allocated pretty much each time I use it, which is going to be an open invitation for bugs. Plus it looks like there are ways that RCU's workqueue wakeups can be executed during very early boot, which can be handled, but again in a rather messy fashion.

Quoted 8 lines

In contrast, given a way of mark a statically allocated workqueue as WQ_MEM_RECLAIM after the fact, I simply continue initializing the workqueue at early boot, and then add the WQ_MEM_RECLAIM marking some arbitrarily chosen time after the scheduler has been initialized.

The required change to workqueues looks easy, just move the body of the "if (flags & WQ_MEM_RECLAIM) {" statement in __alloc_workqueue_key() to a separate function, right?

Ah, okay, yes, currently, workqueue init is kinda silly in that while it allows init of non-mem-reclaiming workqueues way before workqueue is actually online, it doesn't allow the same for mem-reclaiming ones. As you pointed out, it's just an oversight on my part as the init path split was done initially to accomodate early init of system workqueues.

Quoted 4 lines

I'll update the code so that rescuers can be added later too; however, please note that while the work items may be queued, they won't be executed until workqueue_init() is run (the same as now) as there can't be worker threads anyway before that point.

Thank you! I added the following patch to allow RCU access to the init_rescuer() function. Does that work for you, or did you have some other arrangement in mind?

Hello, Paul.

Review

Quoted 6 lines · Paul E. McKenney

(quoted diff) +6 -0

+static int __init rcu_init_wq_rescuer(void)
+{
+	WARN_ON(init_rescuer(rcu_gp_workqueue));
+	return 0;
+}
+core_initcall(rcu_init_wq_rescuer);

So, what I don't get is why RCU needs to call this explicitly. core_initcall() is after workqueue_init() anyway. Why am I missing?

OK, so I can put WQ_MEM_RECLAIM on the early boot creation of RCU's workqueue_struct as shown below, right?

Hello, Paul.

Quoted 2 lines · Paul E. McKenney

OK, so I can put WQ_MEM_RECLAIM on the early boot creation of RCU's workqueue_struct as shown below, right?

Yes, this looks good to me. Just one question.

Quoted 12 lines

kernel/rcu/tree.c +5 -0

+struct workqueue_struct *rcu_gp_workqueue;
+
 void __init rcu_init(void)
 {
 	int cpu;

@@ -4298,6 +4300,10 @@ void __init rcu_init(void)
 		rcu_cpu_starting(cpu);
 		rcutree_online_cpu(cpu);
 	}
+
+	/* Create workqueue for expedited GPs and for Tree SRCU. */
+	rcu_gp_workqueue = alloc_workqueue("rcu_gp", WQ_MEM_RECLAIM, 0);

kernel/rcu/tree.c void __init rcu_init(void) · line 4306

Quoted 1 line

kernel/rcu/tree.c +1 -0

+	WARN_ON(!rcu_gp_workqueue);

The code was previously using both system_power_efficient_wq and system_workqueue (for the expedited path). I guess the options were either using two workqueues or dropping POWER_EFFICIENT. I have no idea how big an impact this will make or whether it'd even be noticeable but maybe it'd be worthwhile to mention that in the description?

Quoted 6 lines

The code was previously using both system_power_efficient_wq and system_workqueue (for the expedited path). I guess the options were either using two workqueues or dropping POWER_EFFICIENT. I have no idea how big an impact this will make or whether it'd even be noticeable but maybe it'd be worthwhile to mention that in the description?

Good point! How about if I change the last paragraph of the commit log to read as follows?

Thanx, Paul

------------------------------------------------------------------------

This commit also causes SRCU to use this new RCU-specific workqueue_struct. Note that SRCU's use of workqueues never blocks them waiting for readers, so this should be safe from a forward-progress viewpoint. Note that this moves SRCU from system_power_efficient_wq to a normal workqueue. In the unlikely event that this results in measurable degradation, a separate power-efficient workqueue will be creates for SRCU.

Hello, Paul.

Quoted 14 lines · Paul E. McKenney

Quoted 6 lines

The code was previously using both system_power_efficient_wq and system_workqueue (for the expedited path). I guess the options were either using two workqueues or dropping POWER_EFFICIENT. I have no idea how big an impact this will make or whether it'd even be noticeable but maybe it'd be worthwhile to mention that in the description?

Good point! How about if I change the last paragraph of the commit log to read as follows?

Thanx, Paul

------------------------------------------------------------------------

Quoted 7 lines

This commit also causes SRCU to use this new RCU-specific workqueue_struct. Note that SRCU's use of workqueues never blocks them waiting for readers, so this should be safe from a forward-progress viewpoint. Note that this moves SRCU from system_power_efficient_wq to a normal workqueue. In the unlikely event that this results in measurable degradation, a separate power-efficient workqueue will be creates for SRCU.

Sounds good. Please feel free to add

Acked-by Tejun Heo <[email protected]>

Quoted 25 lines · Tejun Heo

Hello, Paul.

Quoted 21 lines · Paul E. McKenney

Quoted 6 lines

The code was previously using both system_power_efficient_wq and system_workqueue (for the expedited path). I guess the options were either using two workqueues or dropping POWER_EFFICIENT. I have no idea how big an impact this will make or whether it'd even be noticeable but maybe it'd be worthwhile to mention that in the description?

Good point! How about if I change the last paragraph of the commit log to read as follows?

Thanx, Paul

------------------------------------------------------------------------

This commit also causes SRCU to use this new RCU-specific workqueue_struct. Note that SRCU's use of workqueues never blocks them waiting for readers, so this should be safe from a forward-progress viewpoint. Note that this moves SRCU from system_power_efficient_wq to a normal workqueue. In the unlikely event that this results in measurable degradation, a separate power-efficient workqueue will be creates for SRCU.

Quoted 3 lines

Sounds good. Please feel free to add

Acked-by Tejun Heo <[email protected]>

Done, thank you!

And one additional question... How are we pushing this upstream? By default, I would push things starting this late into the merge window following the next one (v4.17), but would be more than willing to make an exception given that this fixes a valid real-world complaint.

For that matter, if you would rather push my commit along with your pair of commits, that works for me! Either way, please just let me know.

Hello, Paul.

Quoted 4 lines · Paul E. McKenney

And one additional question... How are we pushing this upstream? By default, I would push things starting this late into the merge window following the next one (v4.17), but would be more than willing to make an exception given that this fixes a valid real-world complaint.

I put the workqueue part in the fixes branch. If there's gonna be another rc, I'm gonna push it to Linus. If not, I'll push when the next window opens. I think either way should be fine.

Quoted 2 lines

For that matter, if you would rather push my commit along with your pair of commits, that works for me! Either way, please just let me know.

Oh, let's push these through their respective trees. Please take the RCU one through the usual RCU tree.

Thank you very much.

Hello,

Quoted 5 lines · Prateek Sood

task T is waiting for cpuset_mutex acquired
by kworker/2:1

sh ==> cpuhp/2 ==> kworker/2:1 ==> sh

Quoted 7 lines

kworker/2:3 ==> kthreadd ==> Task T ==> kworker/2:1

It seems that my earlier patch set should fix this scenario:
1) Inverting locking order of cpuset_mutex and cpu_hotplug_lock.
2) Make cpuset hotplug work synchronous.

Could you please share your feedback.

Hmm... this can also be resolved by adding WQ_MEM_RECLAIM to the synchronize rcu workqueue, right? Given the wide-spread usages of synchronize_rcu and friends, maybe that's the right solution, or at least something we also need to do, for this particular deadlock?

Again, I don't have anything against making the domain rebuliding part of cpuset operations synchronous and these tricky deadlock scenarios do indicate that doing so would probably be beneficial. That said, tho, these scenarios seem more of manifestations of other problems exposed through kthreadd dependency than anything else.

Thanks.

My understanding of WQ_MEM_RECLAIM was that it needs to be used for cases where memory pressure could cause deadlocks.

In this case it does not seem to be a memory pressure issue. Overloading WQ_MEM_RECLAIM usage for solution to another problem is the correct approach?

This scenario can be resolved by using WQ_MEM_RECLAIM and a separate workqueue for rcu. But there seems to be a possibility in future if any cpu hotplug callbacks use other predefined workqueues which do not have WQ_MEM_RECLAIM option.