Eliminate deadlock between CPU hotplug and expedited grace periods
Subsystems (3)
RFC PATCH 1/1 Eliminate deadlock between CPU hotplug and expedited grace periods
Currently, the expedited grace-period primitives do get_online_cpus(). This greatly simplifies their implementation, but means that calls to them holding locks that are acquired by CPU-hotplug notifiers (to say nothing of calls to these primitives from CPU-hotplug notifiers) can deadlock. But this is starting to become inconvenient:
https://lkml.org/lkml/2014/8/5/754
This commit avoids the deadlock and retains the simplicity by creating a try_get_online_cpus(), which returns false if the get_online_cpus() reference count could not immediately be incremented. If a call to try_get_online_cpus() returns true, the expedited primitives operate as before. If a call returns false, the expedited primitives fall back to normal grace-period operations. This falling back of course results in increased grace-period latency, but only during times when CPU hotplug operations are actually in flight. The effect should therefore be negligible during normal operation.
include/linux/cpu.h +2 -0
@@ -213,6 +213,7 @@ extern struct bus_type cpu_subsys;extern void cpu_hotplug_begin (void );extern void cpu_hotplug_done (void );extern void get_online_cpus (void ); +extern bool try_get_online_cpus (void );extern void put_online_cpus (void );extern void cpu_hotplug_disable (void );extern void cpu_hotplug_enable (void );
@@ -230,6 +231,7 @@ int cpu_down(unsigned int cpu);static inline void cpu_hotplug_begin (void ) {}static inline void cpu_hotplug_done (void ) {}#define get_online_cpus () do { } while (0) +#define try_get_online_cpus () true#define put_online_cpus () do { } while (0)#define cpu_hotplug_disable () do { } while (0)#define cpu_hotplug_enable () do { } while (0)
include/linux/lockdep.h +1 -0
@@ -505,6 +505,7 @@ static inline void print_irqtrace_events(struct task_struct *curr)#define lock_map_acquire (l) lock_acquire_exclusive(l, 0, 0, NULL, _THIS_IP_)#define lock_map_acquire_read (l) lock_acquire_shared_recursive(l, 0, 0, NULL, _THIS_IP_) +#define lock_map_acquire_tryread (l) lock_acquire_shared_recursive(l, 0, 1, NULL, _THIS_IP_)#define lock_map_release (l) lock_release(l, 1, _THIS_IP_)#ifdef CONFIG_PROVE_LOCKING
kernel/cpu.c +15 -1
@@ -79,6 +79,8 @@ static struct {/* Lockdep annotations for get/put_online_cpus() and cpu_hotplug_begin/end() */ #define cpuhp_lock_acquire_read () lock_map_acquire_read(&cpu_hotplug.dep_map) +#define cpuhp_lock_acquire_tryread () \ + lock_map_acquire_tryread(&cpu_hotplug.dep_map)#define cpuhp_lock_acquire () lock_map_acquire(&cpu_hotplug.dep_map)#define cpuhp_lock_release () lock_map_release(&cpu_hotplug.dep_map)
@@ -91,10 +93,22 @@ void get_online_cpus(void)mutex_lock (&cpu_hotplug.lock); cpu_hotplug.refcount++;mutex_unlock (&cpu_hotplug.lock); - }EXPORT_SYMBOL_GPL (get_online_cpus); +bool try_get_online_cpus (void ) +{ +if (cpu_hotplug.active_writer == current) +return true; +if (!mutex_trylock (&cpu_hotplug.lock)) +return false; +cpuhp_lock_acquire_tryread (); + cpu_hotplug.refcount++; +mutex_unlock (&cpu_hotplug.lock); +return true; +} +EXPORT_SYMBOL_GPL (try_get_online_cpus); +void put_online_cpus (void ) {if (cpu_hotplug.active_writer == current)
kernel/rcu/tree.c +6 -6
@@ -2940,11 +2940,6 @@ static int synchronize_sched_expedited_cpu_stop(void *data)* restructure your code to batch your updates, and then use a single * synchronize_sched() instead. * -* Note that it is illegal to call this function while holding any lock -* that is acquired by a CPU-hotplug notifier. And yes, it is also illegal -* to call this function from a CPU-hotplug notifier. Failing to observe -* these restriction will result in deadlock. -* * This implementation can be thought of as an application of ticket * locking to RCU, with sync_sched_expedited_started and * sync_sched_expedited_done taking on the roles of the halves
@@ -2994,7 +2989,12 @@ void synchronize_sched_expedited(void)*/ snap =atomic_long_inc_return (&rsp->expedited_start); firstsnap = snap; -get_online_cpus (); +if (!try_get_online_cpus ()) { +/* CPU hotplug operation in flight, fall back to normal GP. */ +wait_rcu_gp (call_rcu_sched); +atomic_long_inc (&rsp->expedited_normal); +return ; + }WARN_ON_ONCE (cpu_is_offline (raw_smp_processor_id ()));/*
kernel/rcu/tree_plugin.h +5 -6
@@ -793,11 +793,6 @@ sync_rcu_preempt_exp_init(struct rcu_state *rsp, struct rcu_node *rnp)* In fact, if you are using synchronize_rcu_expedited() in a loop, * please restructure your code to batch your updates, and then Use a * single synchronize_rcu() instead. -* -* Note that it is illegal to call this function while holding any lock -* that is acquired by a CPU-hotplug notifier. And yes, it is also illegal -* to call this function from a CPU-hotplug notifier. Failing to observe -* these restriction will result in deadlock. */ void synchronize_rcu_expedited (void ) {
@@ -819,7 +814,11 @@ void synchronize_rcu_expedited(void)* being boosted. This simplifies the process of moving tasks * from leaf to root rcu_node structures. */ -get_online_cpus (); +if (!try_get_online_cpus ()) { +/* CPU-hotplug operation in flight, fall back to normal GP. */ +wait_rcu_gp (call_rcu); +return ; + }/* * Acquire lock, falling back to synchronize_rcu() if too many
Quoted 17 lines · Paul E. McKenney
Currently, the expedited grace-period primitives do
get_online_cpus(). This greatly simplifies their implementation, but means that calls to them holding locks that are acquired by CPU-hotplug notifiers (to say nothing of calls to these primitives from CPU-hotplug notifiers) can deadlock. But this is starting to become inconvenient:
https://lkml.org/lkml/2014/8/5/754This commit avoids the deadlock and retains the simplicity by creating a
try_get_online_cpus(), which returns false if theget_online_cpus()reference count could not immediately be incremented. If a call totry_get_online_cpus()returns true, the expedited primitives operate as before. If a call returns false, the expedited primitives fall back to normal grace-period operations. This falling back of course results in increased grace-period latency, but only during times when CPU hotplug operations are actually in flight. The effect should therefore be negligible during normal operation.
Quoted 5 lines
Signed-off-by Paul E. McKenney <[email protected]>Cc Josh Triplett <[email protected]>Cc "Rafael J. Wysocki" <[email protected]>Cc Lan Tianyu <[email protected]>
Hi Paul:
I tested this patch and it fixes my issue. Thanks.
Quoted 129 lines
include/linux/cpu.h +2
-0@@ -213,6 +213,7 @@ extern struct bus_type cpu_subsys;extern void cpu_hotplug_begin (void );extern void cpu_hotplug_done (void );extern void get_online_cpus (void ); +extern bool try_get_online_cpus (void );extern void put_online_cpus (void );extern void cpu_hotplug_disable (void );extern void cpu_hotplug_enable (void );@@ -230,6 +231,7 @@ int cpu_down(unsigned int cpu);static inline void cpu_hotplug_begin (void ) {}static inline void cpu_hotplug_done (void ) {}#define get_online_cpus () do { } while (0) +#define try_get_online_cpus () true#define put_online_cpus () do { } while (0)#define cpu_hotplug_disable () do { } while (0)#define cpu_hotplug_enable () do { } while (0)include/linux/lockdep.h +1
-0@@ -505,6 +505,7 @@ static inline void print_irqtrace_events(struct task_struct *curr)#define lock_map_acquire (l) lock_acquire_exclusive(l, 0, 0, NULL, _THIS_IP_)#define lock_map_acquire_read (l) lock_acquire_shared_recursive(l, 0, 0, NULL, _THIS_IP_) +#define lock_map_acquire_tryread (l) lock_acquire_shared_recursive(l, 0, 1, NULL, _THIS_IP_)#define lock_map_release (l) lock_release(l, 1, _THIS_IP_)#ifdef CONFIG_PROVE_LOCKING kernel/cpu.c +15
-1@@ -79,6 +79,8 @@ static struct {/* Lockdep annotations for get/put_online_cpus() and cpu_hotplug_begin/end() */ #define cpuhp_lock_acquire_read () lock_map_acquire_read(&cpu_hotplug.dep_map) +#define cpuhp_lock_acquire_tryread () \ + lock_map_acquire_tryread(&cpu_hotplug.dep_map)#define cpuhp_lock_acquire () lock_map_acquire(&cpu_hotplug.dep_map)#define cpuhp_lock_release () lock_map_release(&cpu_hotplug.dep_map)@@ -91,10 +93,22 @@ void get_online_cpus(void)mutex_lock (&cpu_hotplug.lock); cpu_hotplug.refcount++;mutex_unlock (&cpu_hotplug.lock); - }EXPORT_SYMBOL_GPL (get_online_cpus); +bool try_get_online_cpus (void ) +{ +if (cpu_hotplug.active_writer == current) +return true; +if (!mutex_trylock (&cpu_hotplug.lock)) +return false; +cpuhp_lock_acquire_tryread (); + cpu_hotplug.refcount++; +mutex_unlock (&cpu_hotplug.lock); +return true; +} +EXPORT_SYMBOL_GPL (try_get_online_cpus); +void put_online_cpus (void ) {if (cpu_hotplug.active_writer == current)kernel/rcu/tree.c +6
-6@@ -2940,11 +2940,6 @@ static int synchronize_sched_expedited_cpu_stop(void *data)* restructure your code to batch your updates, and then use a single * synchronize_sched() instead. * -* Note that it is illegal to call this function while holding any lock -* that is acquired by a CPU-hotplug notifier. And yes, it is also illegal -* to call this function from a CPU-hotplug notifier. Failing to observe -* these restriction will result in deadlock. -* * This implementation can be thought of as an application of ticket * locking to RCU, with sync_sched_expedited_started and * sync_sched_expedited_done taking on the roles of the halves @@ -2994,7 +2989,12 @@ void synchronize_sched_expedited(void)*/ snap =atomic_long_inc_return (&rsp->expedited_start); firstsnap = snap; -get_online_cpus (); +if (!try_get_online_cpus ()) { +/* CPU hotplug operation in flight, fall back to normal GP. */ +wait_rcu_gp (call_rcu_sched); +atomic_long_inc (&rsp->expedited_normal); +return ; + }WARN_ON_ONCE (cpu_is_offline (raw_smp_processor_id ()));/* kernel/rcu/tree_plugin.h +5
-6@@ -793,11 +793,6 @@ sync_rcu_preempt_exp_init(struct rcu_state *rsp, struct rcu_node *rnp)* In fact, if you are using synchronize_rcu_expedited() in a loop, * please restructure your code to batch your updates, and then Use a * single synchronize_rcu() instead. -* -* Note that it is illegal to call this function while holding any lock -* that is acquired by a CPU-hotplug notifier. And yes, it is also illegal -* to call this function from a CPU-hotplug notifier. Failing to observe -* these restriction will result in deadlock. */ void synchronize_rcu_expedited (void ) {@@ -819,7 +814,11 @@ void synchronize_rcu_expedited(void)* being boosted. This simplifies the process of moving tasks * from leaf to root rcu_node structures. */ -get_online_cpus (); +if (!try_get_online_cpus ()) { +/* CPU-hotplug operation in flight, fall back to normal GP. */ +wait_rcu_gp (call_rcu); +return ; + }/* * Acquire lock, falling back to synchronize_rcu() if too many
Signature · 2 lines
Best regards Tianyu Lan
Quoted 24 lines · Lan Tianyu
Quoted 17 lines · Paul E. McKenney
Currently, the expedited grace-period primitives do
get_online_cpus(). This greatly simplifies their implementation, but means that calls to them holding locks that are acquired by CPU-hotplug notifiers (to say nothing of calls to these primitives from CPU-hotplug notifiers) can deadlock. But this is starting to become inconvenient:
https://lkml.org/lkml/2014/8/5/754This commit avoids the deadlock and retains the simplicity by creating a
try_get_online_cpus(), which returns false if theget_online_cpus()reference count could not immediately be incremented. If a call totry_get_online_cpus()returns true, the expedited primitives operate as before. If a call returns false, the expedited primitives fall back to normal grace-period operations. This falling back of course results in increased grace-period latency, but only during times when CPU hotplug operations are actually in flight. The effect should therefore be negligible during normal operation.Quoted 5 lines
Signed-off-by Paul E. McKenney <[email protected]>Cc Josh Triplett <[email protected]>Cc "Rafael J. Wysocki" <[email protected]>Cc Lan Tianyu <[email protected]>
Quoted 4 lines
Hi Paul:
I tested this patch and it fixes my issue. Thanks.Tested-by Lan Tianyu <[email protected]>
Glad it worked for you, and thank you for your testing efforts!
Thanx, Paul
Quoted 135 lines
Quoted 129 lines
include/linux/cpu.h +2
-0@@ -213,6 +213,7 @@ extern struct bus_type cpu_subsys;extern void cpu_hotplug_begin (void );extern void cpu_hotplug_done (void );extern void get_online_cpus (void ); +extern bool try_get_online_cpus (void );extern void put_online_cpus (void );extern void cpu_hotplug_disable (void );extern void cpu_hotplug_enable (void );@@ -230,6 +231,7 @@ int cpu_down(unsigned int cpu);static inline void cpu_hotplug_begin (void ) {}static inline void cpu_hotplug_done (void ) {}#define get_online_cpus () do { } while (0) +#define try_get_online_cpus () true#define put_online_cpus () do { } while (0)#define cpu_hotplug_disable () do { } while (0)#define cpu_hotplug_enable () do { } while (0)include/linux/lockdep.h +1
-0@@ -505,6 +505,7 @@ static inline void print_irqtrace_events(struct task_struct *curr)#define lock_map_acquire (l) lock_acquire_exclusive(l, 0, 0, NULL, _THIS_IP_)#define lock_map_acquire_read (l) lock_acquire_shared_recursive(l, 0, 0, NULL, _THIS_IP_) +#define lock_map_acquire_tryread (l) lock_acquire_shared_recursive(l, 0, 1, NULL, _THIS_IP_)#define lock_map_release (l) lock_release(l, 1, _THIS_IP_)#ifdef CONFIG_PROVE_LOCKING kernel/cpu.c +15
-1@@ -79,6 +79,8 @@ static struct {/* Lockdep annotations for get/put_online_cpus() and cpu_hotplug_begin/end() */ #define cpuhp_lock_acquire_read () lock_map_acquire_read(&cpu_hotplug.dep_map) +#define cpuhp_lock_acquire_tryread () \ + lock_map_acquire_tryread(&cpu_hotplug.dep_map)#define cpuhp_lock_acquire () lock_map_acquire(&cpu_hotplug.dep_map)#define cpuhp_lock_release () lock_map_release(&cpu_hotplug.dep_map)@@ -91,10 +93,22 @@ void get_online_cpus(void)mutex_lock (&cpu_hotplug.lock); cpu_hotplug.refcount++;mutex_unlock (&cpu_hotplug.lock); - }EXPORT_SYMBOL_GPL (get_online_cpus); +bool try_get_online_cpus (void ) +{ +if (cpu_hotplug.active_writer == current) +return true; +if (!mutex_trylock (&cpu_hotplug.lock)) +return false; +cpuhp_lock_acquire_tryread (); + cpu_hotplug.refcount++; +mutex_unlock (&cpu_hotplug.lock); +return true; +} +EXPORT_SYMBOL_GPL (try_get_online_cpus); +void put_online_cpus (void ) {if (cpu_hotplug.active_writer == current)kernel/rcu/tree.c +6
-6@@ -2940,11 +2940,6 @@ static int synchronize_sched_expedited_cpu_stop(void *data)* restructure your code to batch your updates, and then use a single * synchronize_sched() instead. * -* Note that it is illegal to call this function while holding any lock -* that is acquired by a CPU-hotplug notifier. And yes, it is also illegal -* to call this function from a CPU-hotplug notifier. Failing to observe -* these restriction will result in deadlock. -* * This implementation can be thought of as an application of ticket * locking to RCU, with sync_sched_expedited_started and * sync_sched_expedited_done taking on the roles of the halves @@ -2994,7 +2989,12 @@ void synchronize_sched_expedited(void)*/ snap =atomic_long_inc_return (&rsp->expedited_start); firstsnap = snap; -get_online_cpus (); +if (!try_get_online_cpus ()) { +/* CPU hotplug operation in flight, fall back to normal GP. */ +wait_rcu_gp (call_rcu_sched); +atomic_long_inc (&rsp->expedited_normal); +return ; + }WARN_ON_ONCE (cpu_is_offline (raw_smp_processor_id ()));/* kernel/rcu/tree_plugin.h +5
-6@@ -793,11 +793,6 @@ sync_rcu_preempt_exp_init(struct rcu_state *rsp, struct rcu_node *rnp)* In fact, if you are using synchronize_rcu_expedited() in a loop, * please restructure your code to batch your updates, and then Use a * single synchronize_rcu() instead. -* -* Note that it is illegal to call this function while holding any lock -* that is acquired by a CPU-hotplug notifier. And yes, it is also illegal -* to call this function from a CPU-hotplug notifier. Failing to observe -* these restriction will result in deadlock. */ void synchronize_rcu_expedited (void ) {@@ -819,7 +814,11 @@ void synchronize_rcu_expedited(void)* being boosted. This simplifies the process of moving tasks * from leaf to root rcu_node structures. */ -get_online_cpus (); +if (!try_get_online_cpus ()) { +/* CPU-hotplug operation in flight, fall back to normal GP. */ +wait_rcu_gp (call_rcu); +return ; + }/* * Acquire lock, falling back to synchronize_rcu() if too many Signature · 2 lines
Best regards Tianyu Lan
Peter Zijlstra
maintainerQuoted 6 lines · Paul E. McKenney
Currently, the expedited grace-period primitives do
get_online_cpus(). This greatly simplifies their implementation, but means that calls to them holding locks that are acquired by CPU-hotplug notifiers (to say nothing of calls to these primitives from CPU-hotplug notifiers) can deadlock. But this is starting to become inconvenient:
https://lkml.org/lkml/2014/8/5/754
Please recap the actual problem; the link might die and the actual mail linked to isn't very useful in any case.
Quoted 9 lines
This commit avoids the deadlock and retains the simplicity by creating a
try_get_online_cpus(), which returns false if theget_online_cpus()reference count could not immediately be incremented. If a call totry_get_online_cpus()returns true, the expedited primitives operate as before. If a call returns false, the expedited primitives fall back to normal grace-period operations. This falling back of course results in increased grace-period latency, but only during times when CPU hotplug operations are actually in flight. The effect should therefore be negligible during normal operation.
URGH.. I really hate that. The hotplug interface is already too horrible, we should not add such hacks to it.
How about ripping that rcu_expedited stuff out instead? That's all conditional anyhow, so might as well not do it.
Quoted 10 lines · Peter Zijlstra
Quoted 6 lines · Paul E. McKenney
Currently, the expedited grace-period primitives do
get_online_cpus(). This greatly simplifies their implementation, but means that calls to them holding locks that are acquired by CPU-hotplug notifiers (to say nothing of calls to these primitives from CPU-hotplug notifiers) can deadlock. But this is starting to become inconvenient:
https://lkml.org/lkml/2014/8/5/754Please recap the actual problem; the link might die and the actual mail linked to isn't very useful in any case.
Will do.
Quoted 10 lines
Quoted 9 lines
This commit avoids the deadlock and retains the simplicity by creating a
try_get_online_cpus(), which returns false if theget_online_cpus()reference count could not immediately be incremented. If a call totry_get_online_cpus()returns true, the expedited primitives operate as before. If a call returns false, the expedited primitives fall back to normal grace-period operations. This falling back of course results in increased grace-period latency, but only during times when CPU hotplug operations are actually in flight. The effect should therefore be negligible during normal operation.
Quoted 2 lines
URGH.. I really hate that. The hotplug interface is already too horrible, we should not add such hacks to it.
We do have try_ interfaces to a number of other subsystems, so I don't believe that it qualifies as such a hack.
Quoted 2 lines
How about ripping that
rcu_expeditedstuff out instead? That's all conditional anyhow, so might as well not do it.
In what way is the expedited stuff conditional?
Thanx, Paul
Peter Zijlstra
maintainerQuoted 5 lines · Paul E. McKenney
Quoted 2 lines
URGH.. I really hate that. The hotplug interface is already too horrible, we should not add such hacks to it.
We do have
try_interfaces to a number of other subsystems, so I don't believe that it qualifies as such a hack.
We do indeed, but I'm not sure about adding this to the hotplug stuff.
Also; not really understanding the problem doesn't help.
Quoted 4 lines
Quoted 2 lines
How about ripping that
rcu_expeditedstuff out instead? That's all conditional anyhow, so might as well not do it.In what way is the expedited stuff conditional?
synchronize_sched() conditionally calls synchronize_sched_expedited() and its condition: rcu_expedited, gets set/cleared on pm notifiers and nr_cpu_ids.
Quoted 8 lines · Peter Zijlstra
Quoted 5 lines · Paul E. McKenney
Quoted 2 lines
URGH.. I really hate that. The hotplug interface is already too horrible, we should not add such hacks to it.
We do have
try_interfaces to a number of other subsystems, so I don't believe that it qualifies as such a hack.We do indeed, but I'm not sure about adding this to the hotplug stuff.
Looks pretty straightforward to me.
Quoted 1 line
Also; not really understanding the problem doesn't help.
The current implementation of synchronize_sched_expedited() calls get_online_cpus(). Some of the ACPI code needs to hold the acpi_ioremap_lock mutex across synchronize_sched_expedited(), and also needs to acquire this same mutex from a CPU hotplug notifier. This results in deadlock between the cpu_hotplug.lock mutex and the acpi_ioremap_lock mutex.
Normal RCU grace periods avoid this by synchronizing on a lock acquired by the RCU CPU-hotplug notifiers, but this does not work for the expedited grace periods because the outgoing CPU can be running random tasks for quite some time after RCU's notifier executes. So the fix is just to drop back to a normal grace period when there is a CPU-hotplug operation in progress.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
How about ripping that
rcu_expeditedstuff out instead? That's all conditional anyhow, so might as well not do it.In what way is the expedited stuff conditional?
synchronize_sched()conditionally callssynchronize_sched_expedited()and its condition:rcu_expedited, gets set/cleared on pm notifiers andnr_cpu_ids.
There are also direct calls to both synchronize_sched_expedited() and synchronize_rcu_expedited().
Thanx, Paul
Peter Zijlstra
maintainerQuoted 12 lines · Paul E. McKenney
Quoted 8 lines · Peter Zijlstra
Quoted 5 lines · Paul E. McKenney
Quoted 2 lines
URGH.. I really hate that. The hotplug interface is already too horrible, we should not add such hacks to it.
We do have
try_interfaces to a number of other subsystems, so I don't believe that it qualifies as such a hack.We do indeed, but I'm not sure about adding this to the hotplug stuff.
Looks pretty straightforward to me.
Quoted 8 lines
Quoted 1 line
Also; not really understanding the problem doesn't help.
The current implementation of
synchronize_sched_expedited()callsget_online_cpus(). Some of the ACPI code needs to hold theacpi_ioremap_lockmutex acrosssynchronize_sched_expedited(), and also needs to acquire this same mutex from a CPU hotplug notifier. This results in deadlock between the cpu_hotplug.lock mutex and theacpi_ioremap_lockmutex.
acpi_ioremap_lock cpu_hotplug_begin()
synchronize_sched() acpi_ioremap_lock
get_online_cpus()
So yes, AB-BA.
Quoted 6 lines
Normal RCU grace periods avoid this by synchronizing on a lock acquired by the RCU CPU-hotplug notifiers, but this does not work for the expedited grace periods because the outgoing CPU can be running random tasks for quite some time after RCU's notifier executes. So the fix is just to drop back to a normal grace period when there is a CPU-hotplug operation in progress.
So why are we 'normally' doing an expedited call here anyhow?
Quoted 11 lines
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
How about ripping that
rcu_expeditedstuff out instead? That's all conditional anyhow, so might as well not do it.In what way is the expedited stuff conditional?
synchronize_sched()conditionally callssynchronize_sched_expedited()and its condition:rcu_expedited, gets set/cleared on pm notifiers andnr_cpu_ids.There are also direct calls to both
synchronize_sched_expedited()andsynchronize_rcu_expedited().
But those are not within hotplug bits. Also weren't we removing them? I thought we didn't appreciate spraying IPIs like they do?
Quoted 29 lines · Peter Zijlstra
Quoted 12 lines · Paul E. McKenney
Quoted 8 lines · Peter Zijlstra
Quoted 5 lines · Paul E. McKenney
Quoted 2 lines
URGH.. I really hate that. The hotplug interface is already too horrible, we should not add such hacks to it.
We do have
try_interfaces to a number of other subsystems, so I don't believe that it qualifies as such a hack.We do indeed, but I'm not sure about adding this to the hotplug stuff.
Looks pretty straightforward to me.
Quoted 8 lines
Quoted 1 line
Also; not really understanding the problem doesn't help.
The current implementation of
synchronize_sched_expedited()callsget_online_cpus(). Some of the ACPI code needs to hold theacpi_ioremap_lockmutex acrosssynchronize_sched_expedited(), and also needs to acquire this same mutex from a CPU hotplug notifier. This results in deadlock between the cpu_hotplug.lock mutex and theacpi_ioremap_lockmutex.
acpi_ioremap_lockcpu_hotplug_begin()
synchronize_sched()acpi_ioremap_lock
get_online_cpus()So yes, AB-BA.
Quoted 8 lines
Quoted 6 lines
Normal RCU grace periods avoid this by synchronizing on a lock acquired by the RCU CPU-hotplug notifiers, but this does not work for the expedited grace periods because the outgoing CPU can be running random tasks for quite some time after RCU's notifier executes. So the fix is just to drop back to a normal grace period when there is a CPU-hotplug operation in progress.
So why are we 'normally' doing an expedited call here anyhow?
Presumably because they set either the boot parameter or the sysfs variable that causes synchronize_sched() to so synchronize_sched_expedited().
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
How about ripping that
rcu_expeditedstuff out instead? That's all conditional anyhow, so might as well not do it.In what way is the expedited stuff conditional?
synchronize_sched()conditionally callssynchronize_sched_expedited()and its condition:rcu_expedited, gets set/cleared on pm notifiers andnr_cpu_ids.
Quoted 5 lines
Quoted 2 lines
There are also direct calls to both
synchronize_sched_expedited()andsynchronize_rcu_expedited().But those are not within hotplug bits. Also weren't we removing them? I thought we didn't appreciate spraying IPIs like they do?
I hadn't heard anything about removing them, but making the expedited primitives a bit less IPI-happy is on my list.
Thanx, Paul
Peter Zijlstra
maintainerQuoted 7 lines · Paul E. McKenney
Quoted 7 lines
Quoted 6 lines
Normal RCU grace periods avoid this by synchronizing on a lock acquired by the RCU CPU-hotplug notifiers, but this does not work for the expedited grace periods because the outgoing CPU can be running random tasks for quite some time after RCU's notifier executes. So the fix is just to drop back to a normal grace period when there is a CPU-hotplug operation in progress.
Quoted 5 lines
Quoted 1 line
So why are we 'normally' doing an expedited call here anyhow?
Presumably because they set either the boot parameter or the sysfs variable that causes
synchronize_sched()to sosynchronize_sched_expedited().
That's not a why but a how. Why does that option exist, why are we doing this?
I cannot actually find a sysfs variable that controls this though; only the rcu_pm_notifier. It seems to favour doing an expedited call when suspending on 'small' machines.
Quoted 5 lines
Quoted 2 lines
But those are not within hotplug bits. Also weren't we removing them? I thought we didn't appreciate spraying IPIs like they do?
I hadn't heard anything about removing them, but making the expedited primitives a bit less IPI-happy is on my list.
I had some recollections of removing a fair number of expedited calls, but its was a long while ago so what do I know ;-)
Making them less IPI happy would be good indeed.
Quoted 8 lines · Peter Zijlstra
Quoted 7 lines · Paul E. McKenney
Quoted 7 lines
Quoted 6 lines
Normal RCU grace periods avoid this by synchronizing on a lock acquired by the RCU CPU-hotplug notifiers, but this does not work for the expedited grace periods because the outgoing CPU can be running random tasks for quite some time after RCU's notifier executes. So the fix is just to drop back to a normal grace period when there is a CPU-hotplug operation in progress.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
So why are we 'normally' doing an expedited call here anyhow?
Presumably because they set either the boot parameter or the sysfs variable that causes
synchronize_sched()to sosynchronize_sched_expedited().That's not a why but a how. Why does that option exist, why are we doing this?
As you say below, to reduce RCU grace-period latency on small systems. And one level of abstraction's why is another level's how. ;-)
Quoted 3 lines
I cannot actually find a sysfs variable that controls this though; only the
rcu_pm_notifier. It seems to favour doing an expedited call when suspending on 'small' machines.
See rcu_expedited_store() in kernel/ksysfs.c. Or the rcu_expedited module_param() in kernel/rcu/update.c.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
But those are not within hotplug bits. Also weren't we removing them? I thought we didn't appreciate spraying IPIs like they do?
I hadn't heard anything about removing them, but making the expedited primitives a bit less IPI-happy is on my list.
I had some recollections of removing a fair number of expedited calls, but its was a long while ago so what do I know ;-)
If a given use case can tolerate the latency of normal calls, then the normal calls certainly are preferable, no two ways about it.
Quoted 1 line
Making them less IPI happy would be good indeed.
Priority of this task duly increased. ;-)
Thanx, Paul
Quoted 17 lines · Paul E. McKenney
Currently, the expedited grace-period primitives do
get_online_cpus(). This greatly simplifies their implementation, but means that calls to them holding locks that are acquired by CPU-hotplug notifiers (to say nothing of calls to these primitives from CPU-hotplug notifiers) can deadlock. But this is starting to become inconvenient:
https://lkml.org/lkml/2014/8/5/754This commit avoids the deadlock and retains the simplicity by creating a
try_get_online_cpus(), which returns false if theget_online_cpus()reference count could not immediately be incremented. If a call totry_get_online_cpus()returns true, the expedited primitives operate as before. If a call returns false, the expedited primitives fall back to normal grace-period operations. This falling back of course results in increased grace-period latency, but only during times when CPU hotplug operations are actually in flight. The effect should therefore be negligible during normal operation.
Quoted 4 lines
Signed-off-by Paul E. McKenney <[email protected]>Cc Josh Triplett <[email protected]>Cc "Rafael J. Wysocki" <[email protected]>Cc Lan Tianyu <[email protected]>
Hi Paul:
What's the status of the patch? Will you push it? Thanks.
Signature · 2 lines
Best regards Tianyu Lan
Quoted 18 lines · Lan Tianyu
Quoted 17 lines · Paul E. McKenney
Currently, the expedited grace-period primitives do
get_online_cpus(). This greatly simplifies their implementation, but means that calls to them holding locks that are acquired by CPU-hotplug notifiers (to say nothing of calls to these primitives from CPU-hotplug notifiers) can deadlock. But this is starting to become inconvenient:
https://lkml.org/lkml/2014/8/5/754This commit avoids the deadlock and retains the simplicity by creating a
try_get_online_cpus(), which returns false if theget_online_cpus()reference count could not immediately be incremented. If a call totry_get_online_cpus()returns true, the expedited primitives operate as before. If a call returns false, the expedited primitives fall back to normal grace-period operations. This falling back of course results in increased grace-period latency, but only during times when CPU hotplug operations are actually in flight. The effect should therefore be negligible during normal operation.
Quoted 7 lines
Quoted 4 lines
Signed-off-by Paul E. McKenney <[email protected]>Cc Josh Triplett <[email protected]>Cc "Rafael J. Wysocki" <[email protected]>Cc Lan Tianyu <[email protected]>Hi Paul:
What's the status of the patch? Will you push it? Thanks.
By default, it would go into 3.19. Do you need it earlier?
Thanx, Paul
Quoted 24 lines · Paul E. McKenney
Quoted 23 lines · Lan Tianyu
Quoted 21 lines · Paul E. McKenney
Currently, the expedited grace-period primitives do
get_online_cpus(). This greatly simplifies their implementation, but means that calls to them holding locks that are acquired by CPU-hotplug notifiers (to say nothing of calls to these primitives from CPU-hotplug notifiers) can deadlock. But this is starting to become inconvenient:
https://lkml.org/lkml/2014/8/5/754This commit avoids the deadlock and retains the simplicity by creating a
try_get_online_cpus(), which returns false if theget_online_cpus()reference count could not immediately be incremented. If a call totry_get_online_cpus()returns true, the expedited primitives operate as before. If a call returns false, the expedited primitives fall back to normal grace-period operations. This falling back of course results in increased grace-period latency, but only during times when CPU hotplug operations are actually in flight. The effect should therefore be negligible during normal operation.Signed-off-by Paul E. McKenney <[email protected]>Cc Josh Triplett <[email protected]>Cc "Rafael J. Wysocki" <[email protected]>Cc Lan Tianyu <[email protected]>
Quoted 4 lines
Quoted 2 lines
Hi Paul:
What's the status of the patch? Will you push it? Thanks.By default, it would go into 3.19. Do you need it earlier?
IMO, this is a dead lock bug which is hard to reproduce and the patch should go into v3.17 and stable tree?
Quoted 3 lines
Thanx, Paul
Signature · 2 lines
Best regards Tianyu Lan
Quoted 25 lines · Lan Tianyu
Quoted 24 lines · Paul E. McKenney
Quoted 23 lines · Lan Tianyu
Quoted 21 lines · Paul E. McKenney
Currently, the expedited grace-period primitives do
get_online_cpus(). This greatly simplifies their implementation, but means that calls to them holding locks that are acquired by CPU-hotplug notifiers (to say nothing of calls to these primitives from CPU-hotplug notifiers) can deadlock. But this is starting to become inconvenient:
https://lkml.org/lkml/2014/8/5/754This commit avoids the deadlock and retains the simplicity by creating a
try_get_online_cpus(), which returns false if theget_online_cpus()reference count could not immediately be incremented. If a call totry_get_online_cpus()returns true, the expedited primitives operate as before. If a call returns false, the expedited primitives fall back to normal grace-period operations. This falling back of course results in increased grace-period latency, but only during times when CPU hotplug operations are actually in flight. The effect should therefore be negligible during normal operation.Signed-off-by Paul E. McKenney <[email protected]>Cc Josh Triplett <[email protected]>Cc "Rafael J. Wysocki" <[email protected]>Cc Lan Tianyu <[email protected]>
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Hi Paul:
What's the status of the patch? Will you push it? Thanks.By default, it would go into 3.19. Do you need it earlier?
IMO, this is a dead lock bug which is hard to reproduce and the patch should go into v3.17 and stable tree?
The problem with pushing for v3.17 is that I would have to rebase that commit to the bottom of my current stack and redo all my testing. If there were any problems, I could not only miss v3.17, but also miss the v3.18 merge window.
So, given that the next merge window happens pretty soon, how about v3.18 and the stable tree?
Thanx, Paul
Quoted 34 lines · Paul E. McKenney
Quoted 25 lines · Lan Tianyu
Quoted 24 lines · Paul E. McKenney
Quoted 23 lines · Lan Tianyu
Quoted 21 lines · Paul E. McKenney
Currently, the expedited grace-period primitives do
get_online_cpus(). This greatly simplifies their implementation, but means that calls to them holding locks that are acquired by CPU-hotplug notifiers (to say nothing of calls to these primitives from CPU-hotplug notifiers) can deadlock. But this is starting to become inconvenient:
https://lkml.org/lkml/2014/8/5/754This commit avoids the deadlock and retains the simplicity by creating a
try_get_online_cpus(), which returns false if theget_online_cpus()reference count could not immediately be incremented. If a call totry_get_online_cpus()returns true, the expedited primitives operate as before. If a call returns false, the expedited primitives fall back to normal grace-period operations. This falling back of course results in increased grace-period latency, but only during times when CPU hotplug operations are actually in flight. The effect should therefore be negligible during normal operation.Signed-off-by Paul E. McKenney <[email protected]>Cc Josh Triplett <[email protected]>Cc "Rafael J. Wysocki" <[email protected]>Cc Lan Tianyu <[email protected]>Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Hi Paul:
What's the status of the patch? Will you push it? Thanks.By default, it would go into 3.19. Do you need it earlier?
IMO, this is a dead lock bug which is hard to reproduce and the patch should go into v3.17 and stable tree?
Quoted 7 lines
The problem with pushing for v3.17 is that I would have to rebase that commit to the bottom of my current stack and redo all my testing. If there were any problems, I could not only miss v3.17, but also miss the v3.18 merge window.
So, given that the next merge window happens pretty soon, how about v3.18 and the stable tree?
That sounds good to me.
Rafael
Quoted 40 lines · Rafael J. Wysocki
Quoted 39 lines · Paul E. McKenney
Quoted 25 lines · Lan Tianyu
Quoted 24 lines · Paul E. McKenney
Quoted 23 lines · Lan Tianyu
Quoted 21 lines · Paul E. McKenney
Currently, the expedited grace-period primitives do
get_online_cpus(). This greatly simplifies their implementation, but means that calls to them holding locks that are acquired by CPU-hotplug notifiers (to say nothing of calls to these primitives from CPU-hotplug notifiers) can deadlock. But this is starting to become inconvenient:
https://lkml.org/lkml/2014/8/5/754This commit avoids the deadlock and retains the simplicity by creating a
try_get_online_cpus(), which returns false if theget_online_cpus()reference count could not immediately be incremented. If a call totry_get_online_cpus()returns true, the expedited primitives operate as before. If a call returns false, the expedited primitives fall back to normal grace-period operations. This falling back of course results in increased grace-period latency, but only during times when CPU hotplug operations are actually in flight. The effect should therefore be negligible during normal operation.Signed-off-by Paul E. McKenney <[email protected]>Cc Josh Triplett <[email protected]>Cc "Rafael J. Wysocki" <[email protected]>Cc Lan Tianyu <[email protected]>Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Hi Paul:
What's the status of the patch? Will you push it? Thanks.By default, it would go into 3.19. Do you need it earlier?
IMO, this is a dead lock bug which is hard to reproduce and the patch should go into v3.17 and stable tree?
The problem with pushing for v3.17 is that I would have to rebase that commit to the bottom of my current stack and redo all my testing. If there were any problems, I could not only miss v3.17, but also miss the v3.18 merge window.
Quoted 4 lines
Quoted 2 lines
So, given that the next merge window happens pretty soon, how about v3.18 and the stable tree?
That sounds good to me.
Very good, I have added it to my v3.18 queue.
Thanx, Paul