Refcount Based Cpu-Hotplug Revisit.
Subsystems (7)
cover Refcount Based Cpu-Hotplug Revisit.
Hi,
This patch series attempts to revisit the topic of cpu-hotplug locking model.
Prior to this attempt, there were several different suggestions on how it should be implemented.
The ones that were posted before were
a) Refcount + Waitqueue model:
Here the threads that want to avoid a cpu hotplug operation while they are operating in cpu-hotplug critical section, bump up the reference to the global online cpu state.
The thread which wants to perform a cpu-hotplug,
blocks until the reference to the global online state goes to zero. Any threads which want to enter the cpu-hotplug critical section during an ongoing cpu-hotplug operatoin, are blocked using a waitqueue.
The advantange of this model was that it is along the lines of the well known get/put model. Only that it allows sleeping of readers and writers.
The disadvantage, as Andrew pointed out was that there do exist a whole bunch of lock_cpu_hotplug()'s whose existance is undocumented, and an approach like this will not improve such a situation.
b) Per Subsystem cpu-hotplug locks: Each subsystem which has cpu-hotplug critical data, uses a lock to protect that data. Such a subsystem needs to subscribe to the cpu-hotplug notification, especially the CPU_LOCK_ACQUIRE and CPU_LOCK_RELEASE events which are sent before and and after a cpu-hotplug operation. While handling these events respectively, the subsystem lock is taken or released.
The advantage this model offered was that lock was associated with the data, which made easy to understand the purpose of locking.
The disadvantage was that any cpu-hotplug aware function, could not be called from a cpu-hotplug callback path, since we would have acquired the subsystem lock during CPU_LOCK_ACQUIRE and attempting to reacquire it would result in a deadlock.
The case which pointed this limitation out was the implementation of synchronize_sched in preemptible rcu.
c) Freezer based cpu-hotplug:
The idea here was to freeze the system using the process freezer technique which is being used for suspend/hibernate purpose, before performing a cpu-hotplug operation. This would ensure that none of the kernel threads are accessing any of the cpu-hotplug critical data, because they are frozen at well known points.
This would have helped to remove all kinds of locks because when a thread is accessing a cpu-hotplug critical data, it meant that the system was not frozen and hence there would be no cpu-hotplug operation untill the thread either voluntarily calls try_to_freeze or returns out of the kernel.
The disadvantage of this approach was that any kind of dependencies between threads might call the freezer to fail. For eg, thread A is waiting for thread B to accomplish something, but thread B is already frozen, leading to a freeze failure. There could be other subtle races which might be difficult to track.
Some time in May 2007, Linus suggested using the refcount model, and this patch series simplifies and reimplements the Refcount + waitqueue model, based on the discussions in the past and inputs from Vatsa and Rusty.
Patch 1/4: Implements the core refcount + waitqueue model. Patch 2/4: Replaces all the lock_cpu_hotplug/unlock_cpu_hotplug instances
with get_online_cpus()/put_online_cpus()
Patch 3/4: Replaces the subsystem mutexes (we do have three of them now,
in sched.c, slab.c and workqueue.c) with get_online_cpus, put_online_cpus.
Patch 4/4: Eliminates the CPU_DEAD and CPU_UP_CANCELLED event handling
from workqueue.c
The patch series has survived an overnight test with kernbench on i386. and has been tested with Paul Mckenney's latest preemptible rcu code.
Awaiting thy feedback!
Thanks and Regards
gautham.
Signature · 5 lines
Gautham R Shenoy Linux Technology Center IBM India. "Freedom comes with a price tag of responsibility, which is still a bargain, because Freedom is priceless!"
RFC PATCH 1/4 Refcount Based Cpu-Hotplug Implementation
This patch implements a Refcount + Waitqueue based model for cpu-hotplug.
A thread which wants to prevent cpu-hotplug, will now bump up a global refcount and the thread which wants to perform a cpu-hotplug operation will block till the global refcount goes to zero.
The readers, if any, during an ongoing cpu-hotplug operation are blocked until the cpu-hotplug operation is over.
--- include/linux/cpu.h | 2 + init/main.c | 1 kernel/cpu.c | 91 ++++++++++++++++++++++++++++++++++++---------------- 3 files changed, 67 insertions(+), 27 deletions(-)
init/main.c +1 -0
@@ -614,6 +614,7 @@ asmlinkage void __init start_kernel(voidvfs_caches_init_early ();cpuset_init_early ();mem_init (); +cpu_hotplug_init ();kmem_cache_init ();setup_per_cpu_pageset ();numa_policy_init ();
include/linux/cpu.h +2 -0
@@ -97,6 +97,7 @@ static inline void cpuhotplug_mutex_unlomutex_unlock (cpu_hp_mutex); } +extern void cpu_hotplug_init (void );extern void lock_cpu_hotplug (void );extern void unlock_cpu_hotplug (void );#define hotcpu_notifier (fn, pri) { \
@@ -116,6 +117,7 @@ static inline void cpuhotplug_mutex_lockstatic inline void cpuhotplug_mutex_unlock (struct mutex *cpu_hp_mutex) { } +#define cpu_hotplug_init () do { } while (0)#define lock_cpu_hotplug () do { } while (0)#define unlock_cpu_hotplug () do { } while (0)#define hotcpu_notifier (fn, pri) do { (void)(fn); } while (0)
kernel/cpu.c +64 -27
158 lines not on this page · raw message
Signature · 5 lines
Gautham R Shenoy Linux Technology Center IBM India. "Freedom comes with a price tag of responsibility, which is still a bargain, because Freedom is priceless!"
Re: [RFC PATCH 1/4] Refcount Based Cpu-Hotplug Implementation
Quoted 2 lines · On Tuesday 16 October 2007 20:34:17 Gautham R Shenoy wrote:
This patch implements a Refcount + Waitqueue based model for cpu-hotplug.
Hi Gautham,
I can't see where you re-initialize the completion.
kernel/cpu.c static __cpuinitdata RAW_NOTIFIER_HEAD(c · line 95
Quoted 9 lines
kernel/cpu.c +9
-0+static void cpu_hotplug_begin (void ) +{ +mutex_lock (&cpu_hotplug.lock); + cpu_hotplug.active_writer = current; +while (cpu_hotplug.refcount) { +mutex_unlock (&cpu_hotplug.lock); +wait_for_completion (&cpu_hotplug.readers_done); +mutex_lock (&cpu_hotplug.lock); + }
AFAICT this will busy-wait on the second CPU hotplug.
Cheers,
Rusty.
Re: [RFC PATCH 1/4] Refcount Based Cpu-Hotplug Implementation
Quoted 5 lines · Rusty Russell
Quoted 2 lines · On Tuesday 16 October 2007 20:34:17 Gautham R Shenoy wrote:
This patch implements a Refcount + Waitqueue based model for cpu-hotplug.
Hi Gautham,
Hi Rusty,
Quoted 2 lines
I can't see where you re-initialize the completion.
The cpu_hotplug.readers_done is a global variable which has been initialized in cpu_hotplug_init.
So I am wondering is the re-initialization required ?
Quoted 13 lines
kernel/cpu.c static __cpuinitdata RAW_NOTIFIER_HEAD(c · line 95
Quoted 10 lines
kernel/cpu.c +9
-0+static void cpu_hotplug_begin (void ) +{ +mutex_lock (&cpu_hotplug.lock); + cpu_hotplug.active_writer = current; +while (cpu_hotplug.refcount) { +mutex_unlock (&cpu_hotplug.lock); +wait_for_completion (&cpu_hotplug.readers_done); +mutex_lock (&cpu_hotplug.lock); + }AFAICT this will busy-wait on the second CPU hotplug.
Well when the first cpu_hotplug comes out of wait_for_completion, it would have decremented the ->done count, so it's as good as new for the second CPU hotplug, no?
Quoted 2 lines
Cheers,
Rusty.
Thanks and Regards
gautham.
Signature · 5 lines
Gautham R Shenoy Linux Technology Center IBM India. "Freedom comes with a price tag of responsibility, which is still a bargain, because Freedom is priceless!"
Re: [RFC PATCH 1/4] Refcount Based Cpu-Hotplug Implementation
Quoted 7 lines · On Wednesday 17 October 2007 15:37:54 Gautham R Shenoy wrote:
Quoted 5 lines · Rusty Russell
Quoted 2 lines · On Tuesday 16 October 2007 20:34:17 Gautham R Shenoy wrote:
This patch implements a Refcount + Waitqueue based model for cpu-hotplug.
Hi Gautham,
Quoted 8 lines
Hi Rusty,
Quoted 1 line
I can't see where you re-initialize the completion.
The cpu_hotplug.readers_done is a global variable which has been initialized in
cpu_hotplug_init.So I am wondering is the re-initialization required ?
Yes. AFAICT you use this completion on every hotplug. Yet once a completion is completed, it needs to be re-initialized to be reused: it's "complete" and wait_for_completion will return immediately thereafter.
Perhaps you want a waitqueue instead?
Rusty.
Re: [RFC PATCH 1/4] Refcount Based Cpu-Hotplug Implementation
Quoted 12 lines · Rusty Russell
Quoted 11 lines · On Wednesday 17 October 2007 15:37:54 Gautham R Shenoy wrote:
Quoted 5 lines · Rusty Russell
Quoted 2 lines · On Tuesday 16 October 2007 20:34:17 Gautham R Shenoy wrote:
This patch implements a Refcount + Waitqueue based model for cpu-hotplug.
Hi Gautham,
Hi Rusty,
Quoted 1 line
I can't see where you re-initialize the completion.
Quoted 9 lines
Quoted 4 lines
The cpu_hotplug.readers_done is a global variable which has been initialized in
cpu_hotplug_init.So I am wondering is the re-initialization required ?
Yes. AFAICT you use this completion on every hotplug. Yet once a completion is completed, it needs to be re-initialized to be reused: it's "complete" and
wait_for_completionwill return immediately thereafter.
Okay, I thought completion followed the spinlock semantics, and hence didn't require re-initalization. Thanks for that information!
Quoted 1 line
Perhaps you want a waitqueue instead?
Yes, I had considered it. But completion looked appealing since it already had the wait-queuing code. I'll give it a try and repost the series with other changes.
Quoted 2 lines
Rusty.
Thanks and Regards
gautham.
Signature · 5 lines
Gautham R Shenoy Linux Technology Center IBM India. "Freedom comes with a price tag of responsibility, which is still a bargain, because Freedom is priceless!"
Re: [RFC PATCH 1/4] Refcount Based Cpu-Hotplug Implementation
In reply to Gautham R ShenoyQuoted 9 lines · Gautham R Shenoy
Quoted 2 lines · Rusty Russell
I can't see where you re-initialize the completion.
The cpu_hotplug.readers_done is a global variable which has been initialized in
cpu_hotplug_init.So I am wondering is the re-initialization required ?
I don't understand why should we re-initialize the completion too, but see below.
Quoted 11 lines
Quoted 11 lines
kernel/cpu.c static __cpuinitdata RAW_NOTIFIER_HEAD(c · line 95
Quoted 9 lines
kernel/cpu.c +9
-0+static void cpu_hotplug_begin (void ) +{ +mutex_lock (&cpu_hotplug.lock); + cpu_hotplug.active_writer = current; +while (cpu_hotplug.refcount) { +mutex_unlock (&cpu_hotplug.lock); +wait_for_completion (&cpu_hotplug.readers_done); +mutex_lock (&cpu_hotplug.lock); + }AFAICT this will busy-wait on the second CPU hotplug.
Why?
Quoted 3 lines
Well when the first
cpu_hotplugcomes out ofwait_for_completion, it would have decremented the ->done count, so it's as good as new for the second CPU hotplug, no?
No, because we decrement the ->done count only once, but there is no guarantee that ->done == 1 when we get CPU after wakeup. Another reader can do lock_cpu_hotplug/unlock_cpu_hotplug in between, so we have a race.
But I disagree with "Yet once a completion is completed, it needs to be re-initialized to be reused: it's "complete" and wait_for_completion will return immediately thereafter".
Rusty, could you please clarify?
Side note, we don't block the new readers while cpu_hotplug_begin() waits for the completion. I don't think this is a problem, but perhaps it makes sense to document the possible livelock.
Oleg.
Re: [RFC PATCH 1/4] Refcount Based Cpu-Hotplug Implementation
In reply to Gautham R ShenoyThis patch dies on boot for me, using sn2_defconfig. It is dies on the kernel/cpu.c line:
mutex_lock(&cpu_hotplug.lock);
This is in cpu_hotplug_begin(), called from _cpu_up(), called from cpu_up().
I haven't finished verifying this yet, but it looks like the initialization of the mutex cpu_hotplug.lock in cpu_hotplug_init() only happens if CONFIG_HOTPLUG_CPU is enabled, but the use of that mutex in the above mutex_lock() call happens on all configs.
In the sn2_defconfig I'm using, as well as in several other ia64, mips and powerpc configs, CONFIG_HOTPLUG_CPU is, at present anyway, disabled.
For the record, the kernel death messages are:
===
Unable to handle kernel NULL pointer dereference (address 0000000000000000) swapper[1]: Oops 8804682956800 [1] Modules linked in:
Pid: 1, CPU 0, comm: swapper
psr : 00001010085a2010 ifs : 800000000000038b ip : [<a00000010081e080>] Not tainted
ip is at __mutex_lock_slowpath+0x280/0x660
unat: 0000000000000000 pfs : 000000000000038b rsc : 0000000000000003
rnat: 1fcc5c7d8b12a4b4 bsps: 000000000001003e pr : 0000000000001041
ldrs: 0000000000000000 ccv : 0000000000000000 fpsr: 0009804c8a70433f csd : 0000000000000000 ssd : 0000000000000000
b0 : a00000010081e040 b6 : e0000030025bee60 b7 : a0000001005315c0 f6 : 1003e1111111111111111 f7 : 1003e20c49ba5e353f7cf f8 : 1003e00000000000000c8 f9 : 10006c7fffffffd73ea5c f10 : 100019ffffffff8a77fd5 f11 : 1003e0000000000000000 r1 : a000000101064760 r2 : a00000010118ef80 r3 : e00000b020377b20 r8 : e00000b020377d00 r9 : 0000000000004000 r10 : e00000b020370b58 r11 : e00000b020370000 r12 : e00000b020377cf0 r13 : e00000b020370000 r14 : 0000000000000001 r15 : e00000b020377d18 r16 : e00000b020377768 r17 : 00000000dead4ead r18 : a000000100d01380 r19 : a000000100e68300 r20 : 0000000000004000 r21 : 0000000000000010 r22 : 0000000000000000 r23 : 0000000000000000 r24 : 0000000000004000 r25 : ffffffffffffffff r26 : e00000b020377d10 r27 : 0000000000000000 r28 : e00000b020377d00 r29 : e00000b020377d08 r30 : a00000010118efa0 r31 : a00000010118ef98
Call Trace:
[<a000000100013a60>] show_stack+0x40/0xa0
sp=e00000b0203778c0 bsp=e00000b020370f80
[<a000000100014360>] show_regs+0x840/0x880
sp=e00000b020377a90 bsp=e00000b020370f28
[<a000000100036460>] die+0x220/0x300
sp=e00000b020377a90 bsp=e00000b020370ee0
[<a000000100059cd0>] ia64_do_page_fault+0x910/0xa80
sp=e00000b020377a90 bsp=e00000b020370e90
[<a00000010000b120>] ia64_leave_kernel+0x0/0x280
sp=e00000b020377b20 bsp=e00000b020370e90
[<a00000010081e080>] __mutex_lock_slowpath+0x280/0x660
sp=e00000b020377cf0 bsp=e00000b020370e38
[<a00000010081e4a0>] mutex_lock+0x40/0x60
sp=e00000b020377d20 bsp=e00000b020370e18
[<a000000100a2c7e0>] cpu_up+0x220/0x6a0
sp=e00000b020377d20 bsp=e00000b020370dd0
[<a000000100a044d0>] kernel_init+0x250/0x7e0
sp=e00000b020377d30 bsp=e00000b020370d88
[<a000000100011fd0>] kernel_thread_helper+0xd0/0x100
sp=e00000b020377e30 bsp=e00000b020370d60
[<a000000100008dc0>] start_kernel_thread+0x20/0x40
sp=e00000b020377e30 bsp=e00000b020370d60
Kernel panic - not syncing: Attempted to kill init!Signature · 3 lines
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.925.600.0401Re: [RFC PATCH 1/4] Refcount Based Cpu-Hotplug Implementation
With the following patch, the four cpu hotplug patches by Gautham R Shenoy boot successfully on my sn2_defconfig ia64 SN2 Altix system.
This patch moves the cpu_hotplug_init() code outside of the #ifdef CONFIG_HOTPLUG_CPU code.
I will confess to being confused however as to the best way to handle this. Having "cpu_hotplug_init" be a piece of code that has to execute whether or not CONFIG_HOTPLUG_CPU is enabled doesn't seem right.
---
include/linux/cpu.h | 3 +-- kernel/cpu.c | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-)
include/linux/cpu.h +1 -2
@@ -83,6 +83,7 @@ static inline void unregister_cpu_notifi#endif /* CONFIG_SMP */ extern struct sysdev_class cpu_sysdev_class; +extern void cpu_hotplug_init (void );#ifdef CONFIG_HOTPLUG_CPU /* Stop CPUs going up and down. */
@@ -97,7 +98,6 @@ static inline void cpuhotplug_mutex_unlomutex_unlock (cpu_hp_mutex); } -extern void cpu_hotplug_init (void );extern void get_online_cpus (void );extern void put_online_cpus (void );#define hotcpu_notifier (fn, pri) { \
@@ -117,7 +117,6 @@ static inline void cpuhotplug_mutex_lockstatic inline void cpuhotplug_mutex_unlock (struct mutex *cpu_hp_mutex) { } -#define cpu_hotplug_init () do { } while (0)#define get_online_cpus () do { } while (0)#define put_online_cpus () do { } while (0)#define hotcpu_notifier (fn, pri) do { (void)(fn); } while (0)
kernel/cpu.c +2 -2
@@ -38,8 +38,6 @@ static struct {#define writer_exists () (cpu_hotplug.active_writer != NULL) -#ifdef CONFIG_HOTPLUG_CPU -void __init cpu_hotplug_init (void ) { cpu_hotplug.active_writer =NULL ;
@@ -48,6 +46,8 @@ void __init cpu_hotplug_init(void)init_completion (&cpu_hotplug.readers_done); } +#ifdef CONFIG_HOTPLUG_CPU +void get_online_cpus (void ) {might_sleep ();
Signature · 3 lines
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.925.600.0401Gautham R Shenoy
Acked-byRe: [RFC PATCH 1/4] Refcount Based Cpu-Hotplug Implementation
Hi Paul,
Quoted 11 lines · Paul Jackson
With the following patch, the four cpu hotplug patches by Gautham R Shenoy boot successfully on my
sn2_defconfigia64 SN2 Altix system.This patch moves the
cpu_hotplug_init()code outside of the #ifdefCONFIG_HOTPLUG_CPUcode.I will confess to being confused however as to the best way to handle this. Having "
cpu_hotplug_init" be a piece of code that has to execute whether or notCONFIG_HOTPLUG_CPUis enabled doesn't seem right.
Thanks for the patch!
I had not tested the patch series with !CONFIG_HOTPLUG_CPU, so no wonder I missed it.
Quoted 3 lines
Signed-off-by Paul Jackson <[email protected]>
Quoted 58 lines
---
include/linux/cpu.h | 3 +-- kernel/cpu.c | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-)include/linux/cpu.h +1
-2@@ -83,6 +83,7 @@ static inline void unregister_cpu_notifi#endif /* CONFIG_SMP */ extern struct sysdev_class cpu_sysdev_class; +extern void cpu_hotplug_init (void );#ifdef CONFIG_HOTPLUG_CPU /* Stop CPUs going up and down. */ @@ -97,7 +98,6 @@ static inline void cpuhotplug_mutex_unlomutex_unlock (cpu_hp_mutex); } -extern void cpu_hotplug_init (void );extern void get_online_cpus (void );extern void put_online_cpus (void );#define hotcpu_notifier (fn, pri) { \@@ -117,7 +117,6 @@ static inline void cpuhotplug_mutex_lockstatic inline void cpuhotplug_mutex_unlock (struct mutex *cpu_hp_mutex) { } -#define cpu_hotplug_init () do { } while (0)#define get_online_cpus () do { } while (0)#define put_online_cpus () do { } while (0)#define hotcpu_notifier (fn, pri) do { (void)(fn); } while (0)kernel/cpu.c +2
-2@@ -38,8 +38,6 @@ static struct {#define writer_exists () (cpu_hotplug.active_writer != NULL) -#ifdef CONFIG_HOTPLUG_CPU -void __init cpu_hotplug_init (void ) { cpu_hotplug.active_writer =NULL ;@@ -48,6 +46,8 @@ void __init cpu_hotplug_init(void)init_completion (&cpu_hotplug.readers_done); } +#ifdef CONFIG_HOTPLUG_CPU +void get_online_cpus (void ) {might_sleep ();Signature · 4 lines
-- I won't rest till it's the best ... Programmer, Linux Scalability Paul Jackson <[email protected]> 1.925.600.0401
Signature · 5 lines
Gautham R Shenoy Linux Technology Center IBM India. "Freedom comes with a price tag of responsibility, which is still a bargain, because Freedom is priceless!"
Re: [RFC PATCH 1/4] Refcount Based Cpu-Hotplug Implementation
Quoted 1 line · Gautham
Thanks for the patch!
You're welcome.
I wonder what are the chances of this patch set making it into 2.6.24.
The cgroup (aka container) folks Paul Menage and David Rientjes are working with me on the (hopefully) last fix for getting cpusets to work with the cgroup changes. This involves how cpusets adapts to CPUs coming and going from the system due to hotplug/hotunplug or coming and going from cpusets due to user changes in the cpuset configuraton.
Not surprisingly, the code we are struggling with depends on the CPU hotplug support, and this present patch set of yours looks to be a fine improvement to that.
However whatever we do for this last fix has to make it into 2.6.24, because cgroups themselves are targeted for 2.6.24, and I can't let that lead to any serious regressions in existing cpuset capability.
Signature · 3 lines
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.925.600.0401RFC PATCH 2/4 Rename lock_cpu_hotplug to get_online_cpus
In reply to Gautham R ShenoyReplace all lock_cpu_hotplug/unlock_cpu_hotplug from the kernel and use get_online_cpus and put_online_cpus instead as it highlights the refcount semantics in these operations.
--- arch/i386/kernel/cpu/mtrr/main.c | 8 ++++---- arch/i386/kernel/microcode.c | 16 ++++++++-------- arch/mips/kernel/mips-mt-fpaff.c | 10 +++++----- arch/powerpc/platforms/pseries/hotplug-cpu.c | 8 ++++---- arch/powerpc/platforms/pseries/rtasd.c | 8 ++++---- drivers/lguest/core.c | 8 ++++---- drivers/s390/char/sclp_config.c | 4 ++-- include/linux/cpu.h | 10 +++++----- kernel/cpu.c | 8 ++++---- kernel/cpuset.c | 14 +++++++------- kernel/rcutorture.c | 6 +++--- kernel/stop_machine.c | 4 ++-- net/core/flow.c | 4 ++-- 13 files changed, 54 insertions(+), 54 deletions(-)
include/linux/cpu.h +5 -5
@@ -98,8 +98,8 @@ static inline void cpuhotplug_mutex_unlo }extern void cpu_hotplug_init (void ); -extern void lock_cpu_hotplug (void ); -extern void unlock_cpu_hotplug (void ); +extern void get_online_cpus (void ); +extern void put_online_cpus (void );#define hotcpu_notifier (fn, pri) { \ static struct notifier_block fn##_nb = \ { .notifier_call = fn, .priority = pri }; \
@@ -117,9 +117,9 @@ static inline void cpuhotplug_mutex_lockstatic inline void cpuhotplug_mutex_unlock (struct mutex *cpu_hp_mutex) { } -#define cpu_hotplug_init () do { } while (0) -#define lock_cpu_hotplug () do { } while (0) -#define unlock_cpu_hotplug () do { } while (0) +#define cpu_hotplug_init () do { } while (0) +#define get_online_cpus () do { } while (0) +#define put_online_cpus () do { } while (0)#define hotcpu_notifier (fn, pri) do { (void)(fn); } while (0)/* These aren't inline functions due to a GCC bug. */ #define register_hotcpu_notifier (nb) ({ (void)(nb); 0; })
kernel/cpu.c +4 -4
@@ -48,7 +48,7 @@ void __init cpu_hotplug_init(void)init_completion (&cpu_hotplug.readers_done); } -void lock_cpu_hotplug (void ) +void get_online_cpus (void ) {might_sleep ();if (cpu_hotplug.active_writer == current)
@@ -58,9 +58,9 @@ void lock_cpu_hotplug(void)mutex_unlock (&cpu_hotplug.lock); } -EXPORT_SYMBOL_GPL (lock_cpu_hotplug); +EXPORT_SYMBOL_GPL (get_online_cpus); -void unlock_cpu_hotplug (void ) +void put_online_cpus (void ) {if (cpu_hotplug.active_writer == current)return ;
@@ -73,7 +73,7 @@ void unlock_cpu_hotplug(void) mutex_unlock(&cpu_hotplug.lock); } -EXPORT_SYMBOL_GPL (unlock_cpu_hotplug); +EXPORT_SYMBOL_GPL (put_online_cpus);#endif /* CONFIG_HOTPLUG_CPU */
kernel/stop_machine.c +2 -2
@@ -203,13 +203,13 @@ int stop_machine_run(int (*fn)(void *),int ret;/* No CPUs can come up or down during this. */ -lock_cpu_hotplug (); +get_online_cpus (); p =__stop_machine_run (fn, data, cpu);if (!IS_ERR (p)) ret =kthread_stop (p);else ret =PTR_ERR (p); -unlock_cpu_hotplug (); +put_online_cpus ();return ret; }
arch/i386/kernel/cpu/mtrr/main.c +4 -4
@@ -351,7 +351,7 @@ int mtrr_add_page(unsigned long base, un replace =-1 ;/* No CPU hotplug when we change MTRR entries */ -lock_cpu_hotplug (); +get_online_cpus ();/* Search for existing MTRR */ mutex_lock (&mtrr_mutex);for (i =0 ; i < num_var_ranges; ++i) {
@@ -407,7 +407,7 @@ int mtrr_add_page(unsigned long base, un error = i; out:mutex_unlock (&mtrr_mutex); -unlock_cpu_hotplug (); +put_online_cpus ();return error; }
@@ -497,7 +497,7 @@ int mtrr_del_page(int reg, unsigned long max = num_var_ranges;/* No CPU hotplug when we change MTRR entries */ -lock_cpu_hotplug (); +get_online_cpus ();mutex_lock (&mtrr_mutex);if (reg <0 ) {/* Search for existing MTRR */
@@ -538,7 +538,7 @@ int mtrr_del_page(int reg, unsigned long error = reg; out:mutex_unlock (&mtrr_mutex); -unlock_cpu_hotplug (); +put_online_cpus ();return error; }/**
arch/i386/kernel/microcode.c +8 -8
@@ -436,7 +436,7 @@ static ssize_t microcode_write (struct freturn -EINVAL ; } -lock_cpu_hotplug (); +get_online_cpus ();mutex_lock (µcode_mutex); user_buffer = (void __user *) buf;
@@ -447,7 +447,7 @@ static ssize_t microcode_write (struct f ret = (ssize_t )len;mutex_unlock (µcode_mutex); -unlock_cpu_hotplug (); +put_online_cpus ();return ret; }
@@ -658,14 +658,14 @@ static ssize_t reload_store(struct sys_d old = current->cpus_allowed; -lock_cpu_hotplug (); +get_online_cpus ();set_cpus_allowed (current,cpumask_of_cpu (cpu));mutex_lock (µcode_mutex);if (uci->valid) err =cpu_request_microcode (cpu);mutex_unlock (µcode_mutex); -unlock_cpu_hotplug (); +put_online_cpus ();set_cpus_allowed (current, old); }if (err)
@@ -817,9 +817,9 @@ static int __init microcode_init (void)return PTR_ERR (microcode_pdev); } -lock_cpu_hotplug (); +get_online_cpus (); error =sysdev_driver_register (&cpu_sysdev_class, &mc_sysdev_driver); -unlock_cpu_hotplug (); +put_online_cpus ();if (error) {microcode_dev_exit ();platform_device_unregister (microcode_pdev);
@@ -839,9 +839,9 @@ static void __exit microcode_exit (void)unregister_hotcpu_notifier (&mc_cpu_notifier); -lock_cpu_hotplug (); +get_online_cpus ();sysdev_driver_unregister (&cpu_sysdev_class, &mc_sysdev_driver); -unlock_cpu_hotplug (); +put_online_cpus ();platform_device_unregister (microcode_pdev); }
arch/powerpc/platforms/pseries/hotplug-cpu.c +4 -4
@@ -151,7 +151,7 @@ static int pseries_add_processor(structfor (i =0 ; i < nthreads; i++)cpu_set (i, tmp); -lock_cpu_hotplug (); +get_online_cpus ();BUG_ON (!cpus_subset (cpu_present_map, cpu_possible_map));
@@ -188,7 +188,7 @@ static int pseries_add_processor(struct } err =0 ; out_unlock: -unlock_cpu_hotplug (); +put_online_cpus ();return err; }
@@ -209,7 +209,7 @@ static void pseries_remove_processor(str nthreads = len /sizeof (u32); -lock_cpu_hotplug (); +get_online_cpus ();for (i =0 ; i < nthreads; i++) {for_each_present_cpu (cpu) {if (get_hard_smp_processor_id (cpu) != intserv[i])
@@ -223,7 +223,7 @@ static void pseries_remove_processor(strprintk (KERN_WARNING "Could not find cpu to remove " "with physical id 0x%x\n" , intserv[i]); } -unlock_cpu_hotplug (); +put_online_cpus (); } static int pseries_smp_notifier(struct notifier_block *nb,
arch/powerpc/platforms/pseries/rtasd.c +4 -4
@@ -382,7 +382,7 @@ static void do_event_scan_all_cpus(long {int cpu; -lock_cpu_hotplug (); +get_online_cpus (); cpu =first_cpu (cpu_online_map);for (;;) {set_cpus_allowed (current,cpumask_of_cpu (cpu));
@@ -390,15 +390,15 @@ static void do_event_scan_all_cpus(longset_cpus_allowed (current,CPU_MASK_ALL );/* Drop hotplug lock, and sleep for the specified delay */ -unlock_cpu_hotplug (); +put_online_cpus ();msleep_interruptible (delay); -lock_cpu_hotplug (); +get_online_cpus (); cpu =next_cpu (cpu, cpu_online_map);if (cpu ==NR_CPUS )break ; } -unlock_cpu_hotplug (); +put_online_cpus (); }static int rtasd (void *unused)
drivers/s390/char/sclp_config.c +2 -2
@@ -29,12 +29,12 @@ static void sclp_cpu_capability_notify(sstruct sys_device *sysdev; printk(KERN_WARNING TAG "cpu capability changed.\n"); -lock_cpu_hotplug (); +get_online_cpus ();for_each_online_cpu (cpu) { sysdev =get_cpu_sysdev (cpu);kobject_uevent (&sysdev->kobj,KOBJ_CHANGE ); } -unlock_cpu_hotplug (); +put_online_cpus (); }static void sclp_conf_receiver_fn (struct evbuf_header *evbuf)
kernel/rcutorture.c +3 -3
@@ -726,11 +726,11 @@ static void rcu_torture_shuffle_tasks(vocpumask_t tmp_mask =CPU_MASK_ALL ;int i; -lock_cpu_hotplug (); +get_online_cpus ();/* No point in shuffling if there is only one online CPU (ex: UP) */ if (num_online_cpus () ==1 ) { -unlock_cpu_hotplug (); +put_online_cpus ();return ; }
@@ -762,7 +762,7 @@ static void rcu_torture_shuffle_tasks(voelse rcu_idle_cpu--; -unlock_cpu_hotplug (); +put_online_cpus (); }/* Shuffle tasks across CPUs, with the intent of allowing each CPU in the
net/core/flow.c +2 -2
@@ -296,7 +296,7 @@ void flow_cache_flush(void)static DEFINE_MUTEX (flow_flush_sem);/* Don't want cpus going down or up during this. */ -lock_cpu_hotplug (); +get_online_cpus ();mutex_lock (&flow_flush_sem);atomic_set (&info.cpuleft,num_online_cpus ());init_completion (&info.completion);
@@ -308,7 +308,7 @@ void flow_cache_flush(void)wait_for_completion (&info.completion);mutex_unlock (&flow_flush_sem); -unlock_cpu_hotplug (); +put_online_cpus (); }static void __devinit flow_cache_cpu_prepare (int cpu)
arch/mips/kernel/mips-mt-fpaff.c +5 -5
@@ -58,13 +58,13 @@ asmlinkage long mipsmt_sys_sched_setaffiif (copy_from_user (&new_mask, user_mask_ptr,sizeof (new_mask)))return -EFAULT ; -lock_cpu_hotplug (); +get_online_cpus ();read_lock (&tasklist_lock); p =find_process_by_pid (pid);if (!p) {read_unlock (&tasklist_lock); -unlock_cpu_hotplug (); +put_online_cpus ();return -ESRCH ; }
@@ -106,7 +106,7 @@ asmlinkage long mipsmt_sys_sched_setaffi out_unlock:put_task_struct (p); -unlock_cpu_hotplug (); +put_online_cpus ();return retval; }
@@ -125,7 +125,7 @@ asmlinkage long mipsmt_sys_sched_getaffiif (len < real_len)return -EINVAL ; -lock_cpu_hotplug (); +get_online_cpus ();read_lock (&tasklist_lock); retval = -ESRCH ;
@@ -140,7 +140,7 @@ asmlinkage long mipsmt_sys_sched_getaffi out_unlock:read_unlock (&tasklist_lock); -unlock_cpu_hotplug (); +put_online_cpus ();if (retval)return retval;if (copy_to_user (user_mask_ptr, &mask, real_len))
drivers/lguest/core.c +4 -4
@@ -735,7 +735,7 @@ static int __init init(void)/* We don't need the complexity of CPUs coming and going while we're * doing this. */ -lock_cpu_hotplug (); +get_online_cpus ();if (cpu_has_pge) {/* We have a broader idea of "global". */ /* Remember that this was originally set (for cleanup). */ cpu_had_pge =1 ;
@@ -745,7 +745,7 @@ static int __init init(void)/* Turn off the feature in the global feature set. */ clear_bit (X86_FEATURE_PGE , boot_cpu_data.x86_capability); } -unlock_cpu_hotplug (); +put_online_cpus ();/* All good! */ return 0 ;
@@ -759,13 +759,13 @@ static void __exit fini(void)unmap_switcher ();/* If we had PGE before we started, turn it back on now. */ -lock_cpu_hotplug (); +get_online_cpus ();if (cpu_had_pge) {set_bit (X86_FEATURE_PGE , boot_cpu_data.x86_capability);/* adjust_pge's argument "1" means set PGE. */ on_each_cpu (adjust_pge, (void *)1 ,0 ,1 ); } -unlock_cpu_hotplug (); +put_online_cpus (); }/* The Host side of lguest can be a module. This is a nice way for people to
kernel/cpuset.c +7 -7
@@ -536,10 +536,10 @@ static int cpusets_overlap(struct cpuset* * Call with cgroup_mutex held. May take callback_mutex during * call due to the kfifo_alloc() and kmalloc() calls. May nest -* a call to the lock_cpu_hotplug()/unlock_cpu_hotplug() pair. +* a call to the get_online_cpus()/put_online_cpus() pair. * Must not be called holding callback_mutex, because we must not -* call lock_cpu_hotplug() while holding callback_mutex. Elsewhere -* the kernel nests callback_mutex inside lock_cpu_hotplug() calls. +* call get_online_cpus() while holding callback_mutex. Elsewhere +* the kernel nests callback_mutex inside get_online_cpus() calls. * So the reverse nesting would risk an ABBA deadlock. * * The three key local variables below are:
@@ -673,9 +673,9 @@ restart: rebuild:/* Have scheduler rebuild sched domains */ -lock_cpu_hotplug (); +get_online_cpus ();partition_sched_domains (ndoms, doms); -unlock_cpu_hotplug (); +put_online_cpus (); done:if (q && !IS_ERR (q))
@@ -1503,10 +1503,10 @@ static struct cgroup_subsys_state *cpuse* * If the cpuset being removed has its flag 'sched_load_balance' * enabled, then simulate turning sched_load_balance off, which -* will call rebuild_sched_domains(). The lock_cpu_hotplug() +* will call rebuild_sched_domains(). The get_online_cpus() * call in rebuild_sched_domains() must not be made while holding * callback_mutex. Elsewhere the kernel nests callback_mutex inside -* lock_cpu_hotplug() calls. So the reverse nesting would risk an +* get_online_cpus() calls. So the reverse nesting would risk an * ABBA deadlock. */
Signature · 5 lines
Gautham R Shenoy Linux Technology Center IBM India. "Freedom comes with a price tag of responsibility, which is still a bargain, because Freedom is priceless!"
Re: [RFC PATCH 2/4] Rename lock_cpu_hotplug to get_online_cpus
Hi Gautham-
Quoted 3 lines · Gautham R Shenoy
Replace all lock_cpu_hotplug/unlock_cpu_hotplug from the kernel and use
get_online_cpusandput_online_cpusinstead as it highlights the refcount semantics in these operations.
Something other than "get_online_cpus", please? lock_cpu_hotplug() protects cpu_present_map as well as cpu_online_map. For example, some of the powerpc code modified in this patch is made a bit less clear because it is manipulating cpu_present_map, not cpu_online_map.
Quoted 38 lines
Index:
linux-2.6.23/arch/powerpc/platforms/pseries/hotplug-cpu.c
===================================================================arch/powerpc/platforms/pseries/hotplug-cpu.c +4
-4@@ -151,7 +151,7 @@ static int pseries_add_processor(structfor (i =0 ; i < nthreads; i++)cpu_set (i, tmp); -lock_cpu_hotplug (); +get_online_cpus ();BUG_ON (!cpus_subset (cpu_present_map, cpu_possible_map));@@ -188,7 +188,7 @@ static int pseries_add_processor(struct } err =0 ; out_unlock: -unlock_cpu_hotplug (); +put_online_cpus ();return err; }@@ -209,7 +209,7 @@ static void pseries_remove_processor(str nthreads = len /sizeof (u32); -lock_cpu_hotplug (); +get_online_cpus ();for (i =0 ; i < nthreads; i++) {for_each_present_cpu (cpu) {if (get_hard_smp_processor_id (cpu) != intserv[i])@@ -223,7 +223,7 @@ static void pseries_remove_processor(strprintk (KERN_WARNING "Could not find cpu to remove " "with physical id 0x%x\n" , intserv[i]); } -unlock_cpu_hotplug (); +put_online_cpus (); }
Re: [RFC PATCH 2/4] Rename lock_cpu_hotplug to get_online_cpus
Hi Nathan,
Quoted 11 lines
Hi Gautham-
Quoted 3 lines · Gautham R Shenoy
Replace all lock_cpu_hotplug/unlock_cpu_hotplug from the kernel and use
get_online_cpusandput_online_cpusinstead as it highlights the refcount semantics in these operations.Something other than "
get_online_cpus", please?lock_cpu_hotplug()protectscpu_present_mapas well ascpu_online_map. For example, some of the powerpc code modified in this patch is made a bit less clear because it is manipulatingcpu_present_map, notcpu_online_map.
A quick look at the code, and I am wondering why is lock_cpu_hotplug() used there in the first place. It doesn't look like we require any protection against cpus coming up/ going down in the code below, since the cpu-hotplug operation doesn't affect the cpu_present_map.
Can't we use another mutex here instead of the cpu_hotplug mutex here ?
Quoted 40 lines
Quoted 40 lines
Index:
linux-2.6.23/arch/powerpc/platforms/pseries/hotplug-cpu.c
===================================================================arch/powerpc/platforms/pseries/hotplug-cpu.c +4
-4@@ -151,7 +151,7 @@ static int pseries_add_processor(structfor (i =0 ; i < nthreads; i++)cpu_set (i, tmp); -lock_cpu_hotplug (); +get_online_cpus ();BUG_ON (!cpus_subset (cpu_present_map, cpu_possible_map));@@ -188,7 +188,7 @@ static int pseries_add_processor(struct } err =0 ; out_unlock: -unlock_cpu_hotplug (); +put_online_cpus ();return err; }@@ -209,7 +209,7 @@ static void pseries_remove_processor(str nthreads = len /sizeof (u32); -lock_cpu_hotplug (); +get_online_cpus ();for (i =0 ; i < nthreads; i++) {for_each_present_cpu (cpu) {if (get_hard_smp_processor_id (cpu) != intserv[i])@@ -223,7 +223,7 @@ static void pseries_remove_processor(strprintk (KERN_WARNING "Could not find cpu to remove " "with physical id 0x%x\n" , intserv[i]); } -unlock_cpu_hotplug (); +put_online_cpus (); }
Signature · 5 lines
Gautham R Shenoy Linux Technology Center IBM India. "Freedom comes with a price tag of responsibility, which is still a bargain, because Freedom is priceless!"
Re: [RFC PATCH 2/4] Rename lock_cpu_hotplug to get_online_cpus
Quoted 13 lines · Gautham R Shenoy
Hi Nathan,
Quoted 11 lines
Hi Gautham-
Quoted 3 lines · Gautham R Shenoy
Replace all lock_cpu_hotplug/unlock_cpu_hotplug from the kernel and use
get_online_cpusandput_online_cpusinstead as it highlights the refcount semantics in these operations.Something other than "
get_online_cpus", please?lock_cpu_hotplug()protectscpu_present_mapas well ascpu_online_map. For example, some of the powerpc code modified in this patch is made a bit less clear because it is manipulatingcpu_present_map, notcpu_online_map.
Quoted 4 lines
A quick look at the code, and I am wondering why is
lock_cpu_hotplug()used there in the first place. It doesn't look like we require any protection against cpus coming up/ going down in the code below, since the cpu-hotplug operation doesn't affect thecpu_present_map.
The locking is necessary. Changes to cpu_online_map and cpu_present_map must be serialized; otherwise you could end up trying to online a cpu as it is being removed (i.e. cleared from cpu_present_map). Online operations must check that a cpu is present before bringing it up (kernel/cpu.c):
/* Requires cpu_add_remove_lock to be held */
static int __cpuinit _cpu_up(unsigned int cpu, int tasks_frozen)
{
int ret, nr_calls = 0;
void *hcpu = (void *)(long)cpu;
unsigned long mod = tasks_frozen ? CPU_TASKS_FROZEN : 0;
if (cpu_online(cpu) || !cpu_present(cpu))
return -EINVAL;
....Quoted 1 line
Can't we use another mutex here instead of the
cpu_hotplugmutex here ?
I guess so, but I don't really see the need...
Re: [RFC PATCH 2/4] Rename lock_cpu_hotplug to get_online_cpus
Quoted 19 lines · Nathan Lynch
Quoted 13 lines · Gautham R Shenoy
Hi Nathan,
Quoted 11 lines
Hi Gautham-
Quoted 3 lines · Gautham R Shenoy
Replace all lock_cpu_hotplug/unlock_cpu_hotplug from the kernel and use
get_online_cpusandput_online_cpusinstead as it highlights the refcount semantics in these operations.Something other than "
get_online_cpus", please?lock_cpu_hotplug()protectscpu_present_mapas well ascpu_online_map. For example, some of the powerpc code modified in this patch is made a bit less clear because it is manipulatingcpu_present_map, notcpu_online_map.Quoted 4 lines
A quick look at the code, and I am wondering why is
lock_cpu_hotplug()used there in the first place. It doesn't look like we require any protection against cpus coming up/ going down in the code below, since the cpu-hotplug operation doesn't affect thecpu_present_map.
Quoted 5 lines
The locking is necessary. Changes to
cpu_online_mapandcpu_present_mapmust be serialized; otherwise you could end up trying to online a cpu as it is being removed (i.e. cleared fromcpu_present_map). Online operations must check that a cpu is present before bringing it up (kernel/cpu.c):
Fair enough!
But we are not protecting the cpu_present_map here using lock_cpu_hotplug(), now are we?
The lock_cpu_hotplug() here, ensures that no cpu-hotplug operations occur in parallel with a processor add or a processor remove. IOW, we're still ensuring that the cpu_online_map doesn't change while we're changing the cpu_present_map. So I don't see why the name get_online_cpus() should be a problem here. May be we could add a comment as to why we don't want a cpu-hotplug operation to happen while we're adding/removing a processor.
Unless of course, lock_cpu_hotplug() is also used to serialize the add_processor and remove_processor operations. Is that the case here ?
Thanks and Regards
gautham.
Quoted 16 lines
/* Requires
cpu_add_remove_lockto be held */
static int__cpuinit_cpu_up(unsigned int cpu, int tasks_frozen){ int ret, nr_calls = 0; void *hcpu = (void *)(long)cpu; unsigned long mod = tasks_frozen ? CPU_TASKS_FROZEN : 0; if (cpu_online(cpu) || !cpu_present(cpu)) return -EINVAL; ....Quoted 1 line
Can't we use another mutex here instead of the
cpu_hotplugmutex here ?I guess so, but I don't really see the need...
Signature · 5 lines
Gautham R Shenoy Linux Technology Center IBM India. "Freedom comes with a price tag of responsibility, which is still a bargain, because Freedom is priceless!"
Re: [RFC PATCH 2/4] Rename lock_cpu_hotplug to get_online_cpus
Quoted 26 lines · Gautham R Shenoy
Quoted 19 lines · Nathan Lynch
Quoted 13 lines · Gautham R Shenoy
Hi Nathan,
Quoted 11 lines
Hi Gautham-
Quoted 3 lines · Gautham R Shenoy
Replace all lock_cpu_hotplug/unlock_cpu_hotplug from the kernel and use
get_online_cpusandput_online_cpusinstead as it highlights the refcount semantics in these operations.Something other than "
get_online_cpus", please?lock_cpu_hotplug()protectscpu_present_mapas well ascpu_online_map. For example, some of the powerpc code modified in this patch is made a bit less clear because it is manipulatingcpu_present_map, notcpu_online_map.Quoted 4 lines
A quick look at the code, and I am wondering why is
lock_cpu_hotplug()used there in the first place. It doesn't look like we require any protection against cpus coming up/ going down in the code below, since the cpu-hotplug operation doesn't affect thecpu_present_map.Quoted 5 lines
The locking is necessary. Changes to
cpu_online_mapandcpu_present_mapmust be serialized; otherwise you could end up trying to online a cpu as it is being removed (i.e. cleared fromcpu_present_map). Online operations must check that a cpu is present before bringing it up (kernel/cpu.c):
Quoted 4 lines
Fair enough!
But we are not protecting the
cpu_present_maphere usinglock_cpu_hotplug(), now are we?
Yes, we are. In addition to the above, updates to cpu_present_map have to be serialized. pseries_add_processor can be summed up as "find the first N unset bits in cpu_present_map and set them". That's not an atomic operation, so some kind of mutual exclusion is needed.
Quoted 2 lines
The
lock_cpu_hotplug()here, ensures that no cpu-hotplug operations occur in parallel with a processor add or a processor remove.
That's one important effect, but not the only one (see above).
Quoted 3 lines
IOW, we're still ensuring that the
cpu_online_mapdoesn't change while we're changing thecpu_present_map. So I don't see why the nameget_online_cpus()should be a problem here.
The naming is a problem IMO for two reasons:
- lock_cpu_hotplug() protects cpu_present_map as well as cpu_online_map (sigh, I see that Documentation/cpu-hotplug.txt disagrees with me, but my statement holds for powerpc, at least).
- get_online_cpus() implies reference count semantics (as stated in the changelog) but AFAICT it really has a reference count implementation with read-write locking semantics.
Hmm, I think there's another problem here. With your changes, code which relies on the mutual exclusion behavior of lock_cpu_hotplug() (such as pseries_add/remove_processor) will now be able to run concurrently. Probably those functions should use cpu_hotplug_begin/end instead.
Re: [RFC PATCH 2/4] Rename lock_cpu_hotplug to get_online_cpus
Hi Nathan,
Quoted 29 lines
Quoted 28 lines · Gautham R Shenoy
Quoted 19 lines · Nathan Lynch
Quoted 13 lines · Gautham R Shenoy
Hi Nathan,
Quoted 11 lines
Hi Gautham-
Quoted 3 lines · Gautham R Shenoy
Replace all lock_cpu_hotplug/unlock_cpu_hotplug from the kernel and use
get_online_cpusandput_online_cpusinstead as it highlights the refcount semantics in these operations.Something other than "
get_online_cpus", please?lock_cpu_hotplug()protectscpu_present_mapas well ascpu_online_map. For example, some of the powerpc code modified in this patch is made a bit less clear because it is manipulatingcpu_present_map, notcpu_online_map.Quoted 4 lines
A quick look at the code, and I am wondering why is
lock_cpu_hotplug()used there in the first place. It doesn't look like we require any protection against cpus coming up/ going down in the code below, since the cpu-hotplug operation doesn't affect thecpu_present_map.Quoted 5 lines
The locking is necessary. Changes to
cpu_online_mapandcpu_present_mapmust be serialized; otherwise you could end up trying to online a cpu as it is being removed (i.e. cleared fromcpu_present_map). Online operations must check that a cpu is present before bringing it up (kernel/cpu.c):Fair enough!
Quoted 8 lines
Quoted 2 lines
But we are not protecting the
cpu_present_maphere usinglock_cpu_hotplug(), now are we?Yes, we are. In addition to the above, updates to
cpu_present_maphave to be serialized.pseries_add_processorcan be summed up as "find the first N unset bits incpu_present_mapand set them". That's not an atomic operation, so some kind of mutual exclusion is needed.
Okay. But other than pseries_add_processor and pseries_remove_processor, are there any other places where we _change_ the cpu_present_map ?
I agree that we need some kind of synchronization for threads which read the cpu_present_map. But probably we can use a seperate mutex for that.
Quoted 21 lines
Quoted 3 lines
The
lock_cpu_hotplug()here, ensures that no cpu-hotplug operations occur in parallel with a processor add or a processor remove.That's one important effect, but not the only one (see above).
Quoted 3 lines
IOW, we're still ensuring that the
cpu_online_mapdoesn't change while we're changing thecpu_present_map. So I don't see why the nameget_online_cpus()should be a problem here.The naming is a problem IMO for two reasons:
-
lock_cpu_hotplug()protectscpu_present_mapas well ascpu_online_map(sigh, I see thatDocumentation/cpu-hotplug.txtdisagrees with me, but my statement holds for powerpc, at least).-
get_online_cpus()implies reference count semantics (as stated in the changelog) but AFAICT it really has a reference count implementation with read-write locking semantics.
Quoted 5 lines
Hmm, I think there's another problem here. With your changes, code which relies on the mutual exclusion behavior of
lock_cpu_hotplug()(such as pseries_add/remove_processor) will now be able to run concurrently. Probably those functions should use cpu_hotplug_begin/end instead.
One of the primary reasons to move away from the mutex semantics for cpu-hotplug protection, was that there were a lot of places where lock_cpu_hotplug() was used for protecting local data structures too, when they had nothing to do with cpu-hotplug as such, and it resulted in a whole mess. It took people quite sometime to sort things out with the cpufreq subsystem.
Probably it would be a lot cleaner if we use get_online_cpus() for protection against cpu-hotplug and use specific mutexes for serializing accesses to local data structures. Thoughts?
Thanks and Regards
gautham.
Signature · 5 lines
Gautham R Shenoy Linux Technology Center IBM India. "Freedom comes with a price tag of responsibility, which is still a bargain, because Freedom is priceless!"
Re: [RFC PATCH 2/4] Rename lock_cpu_hotplug to get_online_cpus
Quoted 29 lines · Gautham R Shenoy
Quoted 29 lines
Quoted 27 lines · Gautham R Shenoy
Quoted 16 lines · Nathan Lynch
Quoted 10 lines · Gautham R Shenoy
Quoted 9 lines
Quoted 3 lines · Gautham R Shenoy
Replace all lock_cpu_hotplug/unlock_cpu_hotplug from the kernel and use
get_online_cpusandput_online_cpusinstead as it highlights the refcount semantics in these operations.Something other than "
get_online_cpus", please?lock_cpu_hotplug()protectscpu_present_mapas well ascpu_online_map. For example, some of the powerpc code modified in this patch is made a bit less clear because it is manipulatingcpu_present_map, notcpu_online_map.Quoted 4 lines
A quick look at the code, and I am wondering why is
lock_cpu_hotplug()used there in the first place. It doesn't look like we require any protection against cpus coming up/ going down in the code below, since the cpu-hotplug operation doesn't affect thecpu_present_map.Quoted 5 lines
The locking is necessary. Changes to
cpu_online_mapandcpu_present_mapmust be serialized; otherwise you could end up trying to online a cpu as it is being removed (i.e. cleared fromcpu_present_map). Online operations must check that a cpu is present before bringing it up (kernel/cpu.c):Fair enough!
But we are not protecting the
cpu_present_maphere usinglock_cpu_hotplug(), now are we?
Quoted 8 lines
Quoted 5 lines
Yes, we are. In addition to the above, updates to
cpu_present_maphave to be serialized.pseries_add_processorcan be summed up as "find the first N unset bits incpu_present_mapand set them". That's not an atomic operation, so some kind of mutual exclusion is needed.Okay. But other than
pseries_add_processorandpseries_remove_processor, are there any other places where we _change_ thecpu_present_map?
Other arch code e.g. ia64 changes it for add/remove also. But I fail to see how it matters.
Quoted 3 lines
I agree that we need some kind of synchronization for threads which read the
cpu_present_map. But probably we can use a seperate mutex for that.
That would be needless complexity.
Quoted 16 lines
Quoted 10 lines
The naming is a problem IMO for two reasons:
-
lock_cpu_hotplug()protectscpu_present_mapas well ascpu_online_map(sigh, I see thatDocumentation/cpu-hotplug.txtdisagrees with me, but my statement holds for powerpc, at least).-
get_online_cpus()implies reference count semantics (as stated in the changelog) but AFAICT it really has a reference count implementation with read-write locking semantics.Quoted 5 lines
Hmm, I think there's another problem here. With your changes, code which relies on the mutual exclusion behavior of
lock_cpu_hotplug()(such as pseries_add/remove_processor) will now be able to run concurrently. Probably those functions should use cpu_hotplug_begin/end instead.
Quoted 6 lines
One of the primary reasons to move away from the mutex semantics for cpu-hotplug protection, was that there were a lot of places where
lock_cpu_hotplug()was used for protecting local data structures too, when they had nothing to do with cpu-hotplug as such, and it resulted in a whole mess. It took people quite sometime to sort things out with the cpufreq subsystem.
cpu_present_map isn't a "local data structure" any more than cpu_online_map, and it is quite relevant to cpu hotplug. We have to maintain the invariant that the set of cpus online is a subset of cpus present.
Quoted 3 lines
Probably it would be a lot cleaner if we use
get_online_cpus()for protection against cpu-hotplug and use specific mutexes for serializing accesses to local data structures. Thoughts?
I don't feel like I'm getting through here. Let me restate.
If I'm reading them correctly, these patches are changing the behavior of lock_cpu_hotplug() from mutex-style locking to a kind of read-write locking. I think that's fine, but the naming of the new API poorly reflects its real behavior. Conversion of lock_cpu_hotplug() users should be done with care. Most of them - those that need one of the cpu maps to remain unchanged during a critical section - can be considered readers. But a few (such as pseries_add_processor() and pseries_remove_processor()) are writers, because they modify one of the maps.
So, why not:
get_cpus_online -> cpumaps_read_lockput_cpus_online -> cpumaps_read_unlockcpu_hotplug_begin -> cpumaps_write_lockcpu_hotplug_end -> cpumaps_write_unlock
Or something similar?
Re: [RFC PATCH 2/4] Rename lock_cpu_hotplug to get_online_cpus
Quoted 59 lines
Quoted 3 lines
Okay. But other than
pseries_add_processorandpseries_remove_processor, are there any other places where we _change_ thecpu_present_map?Other arch code e.g. ia64 changes it for add/remove also. But I fail to see how it matters.
Quoted 3 lines
I agree that we need some kind of synchronization for threads which read the
cpu_present_map. But probably we can use a seperate mutex for that.That would be needless complexity.
Quoted 16 lines
Quoted 10 lines
The naming is a problem IMO for two reasons:
-
lock_cpu_hotplug()protectscpu_present_mapas well ascpu_online_map(sigh, I see thatDocumentation/cpu-hotplug.txtdisagrees with me, but my statement holds for powerpc, at least).-
get_online_cpus()implies reference count semantics (as stated in the changelog) but AFAICT it really has a reference count implementation with read-write locking semantics.Quoted 5 lines
Hmm, I think there's another problem here. With your changes, code which relies on the mutual exclusion behavior of
lock_cpu_hotplug()(such as pseries_add/remove_processor) will now be able to run concurrently. Probably those functions should use cpu_hotplug_begin/end instead.Quoted 6 lines
One of the primary reasons to move away from the mutex semantics for cpu-hotplug protection, was that there were a lot of places where
lock_cpu_hotplug()was used for protecting local data structures too, when they had nothing to do with cpu-hotplug as such, and it resulted in a whole mess. It took people quite sometime to sort things out with the cpufreq subsystem.
cpu_present_mapisn't a "local data structure" any more thancpu_online_map, and it is quite relevant to cpu hotplug. We have to maintain the invariant that the set of cpus online is a subset of cpus present.Quoted 3 lines
Probably it would be a lot cleaner if we use
get_online_cpus()for protection against cpu-hotplug and use specific mutexes for serializing accesses to local data structures. Thoughts?I don't feel like I'm getting through here. Let me restate.
If I'm reading them correctly, these patches are changing the behavior of
lock_cpu_hotplug()from mutex-style locking to a kind of read-write locking. I think that's fine, but the naming of the new API poorly reflects its real behavior. Conversion oflock_cpu_hotplug()users should be done with care. Most of them - those that need one of the cpu maps to remain unchanged during a critical section - can be considered readers. But a few (such aspseries_add_processor()andpseries_remove_processor()) are writers, because they modify one of the maps.
Quoted 8 lines
So, why not:
get_cpus_online->cpumaps_read_lockput_cpus_online->cpumaps_read_unlockcpu_hotplug_begin->cpumaps_write_lockcpu_hotplug_end->cpumaps_write_unlockOr something similar?
Hi Nathan,
I totally see your point and agree with it.
However, though the new implementation appears to have a read-write-semaphore behaviour it differs in following aspects:
a) This implementation allows natural recursion of reads, just as in the case of preempt_count.
This is not possible in case of a normal read-write lock because if you have something like the following in a timeline,
R1 -- > R2--> W3 --> R1, where
Ri is a read by a thread i and
Wi is a write by a thread i,
then thread1 will deadlock as it will be blocked behind W3, holding the semaphore.
b) Regular Reader Writer semaphores are fair. As in if a new reader arrives when a writer is already present, the new reader waits until the writer in front of it finishes. But, in the new implementation, the new reader will proceed as long as the refcount is non-zero, and the writer will get to run only when the number of readers = 0. However, the readers which arrive when the writer is executing, will block until the writer is done.
Because of these properties, the implementation is more similar to the refcounting, except that it allows the new bunch of readers to sleep during an ongoing write.
Like you pointed out, since the code of pseries_add_processor and other places where the cpu_present_map is being changed during the runtime requires write protection, how if the cpu_hotplug_begin/cpu_hotplug_end support is made available outside cpu.c and appropriately named ?
Personally I would propose the following names to reflect the behaviour, but if the community is okay with the word "lock" in the API's I don't have any problems renaming them to what you've suggested.
/*
* Api's which ensure that the cpu_present_map and the cpu_online_map
* do not change while operating in a critical section.
*/
get_cpu_maps();
put_cpu_maps();
/*
* Api's to serialize the updates to the cpu_present_map/cpu_online_map. */
cpu_map_update_begin();
cpu_map_update_done();
Thanks and Regards
gautham.
Signature · 5 lines
Gautham R Shenoy Linux Technology Center IBM India. "Freedom comes with a price tag of responsibility, which is still a bargain, because Freedom is priceless!"
RFC PATCH 3/4 Replace per-subsystem mutexes with get_online_cpus
In reply to Gautham R ShenoyThis patch converts the known per-subsystem cpu_hotplug mutexes to get_online_cpus put_online_cpus.
It also eliminates the CPU_LOCK_ACQUIRE and CPU_LOCK_RELEASE hotplug notification events.
--- include/linux/notifier.h | 4 +--- kernel/cpu.c | 4 ---- kernel/sched.c | 27 ++++++++++----------------- kernel/workqueue.c | 31 ++++++++++++++++--------------- mm/slab.c | 18 +++++++++++------- 5 files changed, 38 insertions(+), 46 deletions(-)
kernel/sched.c +10 -17
@@ -360,7 +360,6 @@ struct rq { }; static DEFINE_PER_CPU_SHARED_ALIGNED(struct rq, runqueues); -static DEFINE_MUTEX (sched_hotcpu_mutex);static inline void check_preempt_curr (struct rq *rq,struct task_struct *p) {
@@ -4337,13 +4336,13 @@ long sched_setaffinity(pid_t pid, cpumasstruct task_struct *p;int retval; -mutex_lock (&sched_hotcpu_mutex); +get_online_cpus ();read_lock (&tasklist_lock); p =find_process_by_pid (pid);if (!p) {read_unlock (&tasklist_lock); -mutex_unlock (&sched_hotcpu_mutex); +put_online_cpus ();return -ESRCH ; }
@@ -4370,7 +4369,7 @@ long sched_setaffinity(pid_t pid, cpumas out_unlock:put_task_struct (p); -mutex_unlock (&sched_hotcpu_mutex); +put_online_cpus ();return retval; }
@@ -4427,7 +4426,7 @@ long sched_getaffinity(pid_t pid, cpumasstruct task_struct *p;int retval; -mutex_lock (&sched_hotcpu_mutex); +get_online_cpus ();read_lock (&tasklist_lock); retval = -ESRCH ;
@@ -4443,7 +4442,7 @@ long sched_getaffinity(pid_t pid, cpumas out_unlock:read_unlock (&tasklist_lock); -mutex_unlock (&sched_hotcpu_mutex); +put_online_cpus ();return retval; }
@@ -5342,9 +5341,6 @@ migration_call(struct notifier_block *nfstruct rq *rq;switch (action) { -case CPU_LOCK_ACQUIRE : -mutex_lock (&sched_hotcpu_mutex); -break ;case CPU_UP_PREPARE :case CPU_UP_PREPARE_FROZEN :
@@ -5398,7 +5394,7 @@ migration_call(struct notifier_block *nfBUG_ON (rq->nr_running !=0 );/* No need to migrate the tasks: it was best-effort if -* they didn't take sched_hotcpu_mutex. Just wake up +* they didn't do a get_online_cpus(). Just wake up * the requestors. */ spin_lock_irq (&rq->lock);while (!list_empty (&rq->migration_queue)) {
@@ -5412,9 +5408,6 @@ migration_call(struct notifier_block *nfspin_unlock_irq (&rq->lock);break ;#endif -case CPU_LOCK_RELEASE : -mutex_unlock (&sched_hotcpu_mutex); -break ; }return NOTIFY_OK ; }
@@ -6338,10 +6331,10 @@ static int arch_reinit_sched_domains(voi {int err; -mutex_lock (&sched_hotcpu_mutex); +get_online_cpus ();detach_destroy_domains (&cpu_online_map); err =arch_init_sched_domains (&cpu_online_map); -mutex_unlock (&sched_hotcpu_mutex); +put_online_cpus ();return err; }
@@ -6452,12 +6445,12 @@ void __init sched_init_smp(void) {cpumask_t non_isolated_cpus; -mutex_lock (&sched_hotcpu_mutex); +get_online_cpus ();arch_init_sched_domains (&cpu_online_map);cpus_andnot (non_isolated_cpus, cpu_possible_map, cpu_isolated_map);if (cpus_empty (non_isolated_cpus))cpu_set (smp_processor_id (), non_isolated_cpus); -mutex_unlock (&sched_hotcpu_mutex); +put_online_cpus ();/* XXX: Theoretical race here - CPU may be hotplugged now */ hotcpu_notifier (update_sched_domains,0 );
mm/slab.c +11 -7
@@ -730,8 +730,7 @@ static inline void init_lock_keys(void)#endif /* -* 1. Guard access to the cache-chain. -* 2. Protect sanity of cpu_online_map against cpu hotplug events +* Guard access to the cache-chain. */ static DEFINE_MUTEX (cache_chain_mutex);static struct list_head cache_chain;
@@ -1331,12 +1330,11 @@ static int __cpuinit cpuup_callback(struint err =0 ;switch (action) { -case CPU_LOCK_ACQUIRE : -mutex_lock (&cache_chain_mutex); -break ;case CPU_UP_PREPARE :case CPU_UP_PREPARE_FROZEN : +mutex_lock (&cache_chain_mutex); err =cpuup_prepare (cpu); +mutex_unlock (&cache_chain_mutex);break ;case CPU_ONLINE :case CPU_ONLINE_FROZEN :
@@ -1373,9 +1371,8 @@ static int __cpuinit cpuup_callback(stru#endif case CPU_UP_CANCELED :case CPU_UP_CANCELED_FROZEN : +mutex_lock (&cache_chain_mutex);cpuup_canceled (cpu); -break ; -case CPU_LOCK_RELEASE :mutex_unlock (&cache_chain_mutex);break ; }
@@ -2170,6 +2167,7 @@ kmem_cache_create (const char *name, siz* We use cache_chain_mutex to ensure a consistent view of * cpu_online_map as well. Please see cpuup_callback */ +get_online_cpus ();mutex_lock (&cache_chain_mutex);list_for_each_entry (pc, &cache_chain, next) {
@@ -2396,6 +2394,7 @@ oops:panic ("kmem_cache_create(): failed to create slab `%s'\n" , name);mutex_unlock (&cache_chain_mutex); +put_online_cpus ();return cachep; }EXPORT_SYMBOL (kmem_cache_create);
@@ -2547,9 +2546,11 @@ int kmem_cache_shrink(struct kmem_cacheint ret;BUG_ON (!cachep ||in_interrupt ()); +get_online_cpus ();mutex_lock (&cache_chain_mutex); ret =__cache_shrink (cachep);mutex_unlock (&cache_chain_mutex); +put_online_cpus ();return ret; }EXPORT_SYMBOL (kmem_cache_shrink);
@@ -2575,6 +2576,7 @@ void kmem_cache_destroy(struct kmem_cachBUG_ON (!cachep ||in_interrupt ());/* Find the cache in the chain of caches. */ +get_online_cpus ();mutex_lock (&cache_chain_mutex);/* * the chain is never empty, cache_cache is never destroyed
@@ -2584,6 +2586,7 @@ void kmem_cache_destroy(struct kmem_cachslab_error (cachep,"Can't free all objects" );list_add (&cachep->next, &cache_chain);mutex_unlock (&cache_chain_mutex); +put_online_cpus ();return ; }
@@ -2592,6 +2595,7 @@ void kmem_cache_destroy(struct kmem_cach__kmem_cache_destroy (cachep);mutex_unlock (&cache_chain_mutex); +put_online_cpus (); }EXPORT_SYMBOL (kmem_cache_destroy);
kernel/workqueue.c +16 -15
@@ -592,8 +592,6 @@ EXPORT_SYMBOL(schedule_delayed_work_on);* Returns zero on success. * Returns -ve errno on failure. * -* Appears to be racy against CPU hotplug. -* * schedule_on_each_cpu() is very slow. */ int schedule_on_each_cpu (work_func_t func)
@@ -605,7 +603,7 @@ int schedule_on_each_cpu(work_func_t funif (!works)return -ENOMEM ; -preempt_disable ();/* CPU hotplug */ +get_online_cpus ();/* CPU hotplug */ for_each_online_cpu (cpu) {struct work_struct *work =per_cpu_ptr (works, cpu);
@@ -613,7 +611,7 @@ int schedule_on_each_cpu(work_func_t funset_bit (WORK_STRUCT_PENDING ,work_data_bits (work));__queue_work (per_cpu_ptr (keventd_wq->cpu_wq, cpu), work); } -preempt_enable (); +put_online_cpus ();flush_workqueue (keventd_wq);free_percpu (works);return 0 ;
@@ -749,8 +747,10 @@ struct workqueue_struct *__create_workqu err =create_workqueue_thread (cwq, singlethread_cpu);start_workqueue_thread (cwq,-1 ); }else { +get_online_cpus ();mutex_lock (&workqueue_mutex);list_add (&wq->list, &workqueues); +mutex_unlock (&workqueue_mutex);for_each_possible_cpu (cpu) { cwq =init_cpu_workqueue (wq, cpu);
@@ -759,7 +759,7 @@ struct workqueue_struct *__create_workqu err =create_workqueue_thread (cwq, cpu);start_workqueue_thread (cwq, cpu); } -mutex_unlock (&workqueue_mutex); +put_online_cpus (); }if (err) {
@@ -809,6 +809,7 @@ void destroy_workqueue(struct workqueue_struct cpu_workqueue_struct *cwq;int cpu; +get_online_cpus ();mutex_lock (&workqueue_mutex);list_del (&wq->list);mutex_unlock (&workqueue_mutex);
@@ -817,6 +818,7 @@ void destroy_workqueue(struct workqueue_ cwq =per_cpu_ptr (wq->cpu_wq, cpu);cleanup_workqueue_thread (cwq, cpu); } +put_online_cpus ();free_percpu (wq->cpu_wq);kfree (wq);
@@ -830,22 +832,17 @@ static int __devinit workqueue_cpu_callbunsigned int cpu = (unsigned long )hcpu;struct cpu_workqueue_struct *cwq;struct workqueue_struct *wq; +int ret =NOTIFY_OK ; action &= ~CPU_TASKS_FROZEN ;switch (action) { -case CPU_LOCK_ACQUIRE : -mutex_lock (&workqueue_mutex); -return NOTIFY_OK ; - -case CPU_LOCK_RELEASE : -mutex_unlock (&workqueue_mutex); -return NOTIFY_OK ;case CPU_UP_PREPARE :cpu_set (cpu, cpu_populated_map); } +mutex_lock (&workqueue_mutex);list_for_each_entry (wq, &workqueues, list) { cwq =per_cpu_ptr (wq->cpu_wq, cpu);
@@ -853,8 +850,10 @@ static int __devinit workqueue_cpu_callbcase CPU_UP_PREPARE :if (!create_workqueue_thread (cwq, cpu))break ; -printk (KERN_ERR "workqueue for %i failed\n" , cpu); -return NOTIFY_BAD ; +printk (KERN_ERR "workqueue [%s] for %i failed\n" , + wq->name, cpu); + ret =NOTIFY_BAD ; + goto out_unlock;case CPU_ONLINE :start_workqueue_thread (cwq, cpu);
@@ -868,7 +867,9 @@ static int __devinit workqueue_cpu_callb } } -return NOTIFY_OK ; +out_unlock: +mutex_unlock (&workqueue_mutex); +return ret; }void __init init_workqueues (void )
kernel/cpu.c +0 -4
@@ -185,7 +185,6 @@ static int _cpu_down(unsigned int cpu, ireturn -EINVAL ;cpu_hotplug_begin (); -raw_notifier_call_chain (&cpu_chain,CPU_LOCK_ACQUIRE , hcpu); err =__raw_notifier_call_chain (&cpu_chain,CPU_DOWN_PREPARE | mod, hcpu,-1 , &nr_calls);if (err ==NOTIFY_BAD ) {
@@ -238,7 +237,6 @@ out_thread: out_allowed:set_cpus_allowed (current, old_allowed); out_release: -raw_notifier_call_chain (&cpu_chain,CPU_LOCK_RELEASE , hcpu);cpu_hotplug_done ();return err; }
@@ -269,7 +267,6 @@ static int __cpuinit _cpu_up(unsigned inreturn -EINVAL ;cpu_hotplug_begin (); -raw_notifier_call_chain (&cpu_chain,CPU_LOCK_ACQUIRE , hcpu); ret =__raw_notifier_call_chain (&cpu_chain,CPU_UP_PREPARE | mod, hcpu,-1 , &nr_calls);if (ret ==NOTIFY_BAD ) {
@@ -293,7 +290,6 @@ out_notify:if (ret !=0 )__raw_notifier_call_chain (&cpu_chain,CPU_UP_CANCELED | mod, hcpu, nr_calls,NULL ); -raw_notifier_call_chain (&cpu_chain,CPU_LOCK_RELEASE , hcpu);cpu_hotplug_done ();return ret;
include/linux/notifier.h +1 -3
@@ -207,9 +207,7 @@ static inline int notifier_to_errno(int#define CPU_DOWN_PREPARE 0x0005/* CPU (unsigned)v going down */ #define CPU_DOWN_FAILED 0x0006/* CPU (unsigned)v NOT going down */ #define CPU_DEAD 0x0007/* CPU (unsigned)v dead */ -#define CPU_LOCK_ACQUIRE 0x0008/* Acquire all hotcpu locks */ -#define CPU_LOCK_RELEASE 0x0009/* Release all hotcpu locks */ -#define CPU_DYING 0x000A/* CPU (unsigned)v not running any task, +#define CPU_DYING 0x0008/* CPU (unsigned)v not running any task, * not handling interrupts, soon dead */ /* Used for CPU hotplug events occuring while tasks are frozen due to a suspend
Signature · 5 lines
Gautham R Shenoy Linux Technology Center IBM India. "Freedom comes with a price tag of responsibility, which is still a bargain, because Freedom is priceless!"
Re: [RFC PATCH 3/4] Replace per-subsystem mutexes with get_online_cpus
Quoted 5 lines · Gautham R Shenoy
This patch converts the known per-subsystem
cpu_hotplugmutexes toget_online_cpusput_online_cpus.
It also eliminates theCPU_LOCK_ACQUIREandCPU_LOCK_RELEASEhotplug notification events.
Personally, I like the changes in workqueue.c very much, a couple of minor nits below.
kernel/workqueue.c EXPORT_SYMBOL(schedule_delayed_work_on); · line 596
Quoted 8 lines
kernel/workqueue.c +0
-2@@ -592,8 +592,6 @@ EXPORT_SYMBOL(schedule_delayed_work_on);* Returns zero on success. * Returns -ve errno on failure. * -* Appears to be racy against CPU hotplug. -*
see below,
Quoted 13 lines
kernel/workqueue.c +1
-1* schedule_on_each_cpu() is very slow. */ int schedule_on_each_cpu (work_func_t func)@@ -605,7 +603,7 @@ int schedule_on_each_cpu(work_func_t funif (!works)return -ENOMEM ; -preempt_disable ();/* CPU hotplug */ +get_online_cpus ();/* CPU hotplug */ for_each_online_cpu (cpu) {struct work_struct *work =per_cpu_ptr (works, cpu);
kernel/workqueue.c int schedule_on_each_cpu(work_func_t fun · line 615
Quoted 6 lines
kernel/workqueue.c +1
-1@@ -613,7 +611,7 @@ int schedule_on_each_cpu(work_func_t funset_bit (WORK_STRUCT_PENDING ,work_data_bits (work));__queue_work (per_cpu_ptr (keventd_wq->cpu_wq, cpu), work); } -preempt_enable (); +put_online_cpus ();flush_workqueue (keventd_wq);
Still racy. To kill the race, please move flush_workqueue() up, before put_online_cpus().
kernel/workqueue.c void destroy_workqueue(struct workqueue_ · line 821
Quoted 13 lines
kernel/workqueue.c +2
-0@@ -809,6 +809,7 @@ void destroy_workqueue(struct workqueue_struct cpu_workqueue_struct *cwq;int cpu; +get_online_cpus ();mutex_lock (&workqueue_mutex);list_del (&wq->list);mutex_unlock (&workqueue_mutex);@@ -817,6 +818,7 @@ void destroy_workqueue(struct workqueue_ cwq =per_cpu_ptr (wq->cpu_wq, cpu);cleanup_workqueue_thread (cwq, cpu); } +put_online_cpus ();
Correct, but I'd suggest to do put_online_cpus() earlier, right after mutex_unlock(&workqueue_mutex).
Quoted 7 lines
kernel/workqueue.c +1
-0@@ -830,22 +832,17 @@ static int __devinit workqueue_cpu_callbunsigned int cpu = (unsigned long )hcpu;struct cpu_workqueue_struct *cwq;struct workqueue_struct *wq; +int ret =NOTIFY_OK ; action &= ~CPU_TASKS_FROZEN ;
kernel/workqueue.c static int __devinit workqueue_cpu_callb · line 840
Quoted 10 lines
kernel/workqueue.c +0
-7switch (action) { -case CPU_LOCK_ACQUIRE : -mutex_lock (&workqueue_mutex); -return NOTIFY_OK ; - -case CPU_LOCK_RELEASE : -mutex_unlock (&workqueue_mutex); -return NOTIFY_OK ;
please remove this emtpy line
kernel/workqueue.c static int __devinit workqueue_cpu_callb · line 845
Quoted 5 lines
kernel/workqueue.c +1
-0case CPU_UP_PREPARE :cpu_set (cpu, cpu_populated_map); } +mutex_lock (&workqueue_mutex);
We don't need workqueue_mutex here. With your patch workqueue_mutex protects list_head, nothing more. But all other callers (create/destroy) should take get_online_cpus() anyway. This means that we can convert workqueue_mutex to spinlock_t.
Oleg.
Re: [RFC PATCH 3/4] Replace per-subsystem mutexes with get_online_cpus
Quoted 85 lines · Oleg Nesterov
Quoted 5 lines · Gautham R Shenoy
This patch converts the known per-subsystem
cpu_hotplugmutexes toget_online_cpusput_online_cpus.
It also eliminates theCPU_LOCK_ACQUIREandCPU_LOCK_RELEASEhotplug notification events.Personally, I like the changes in
workqueue.cvery much, a couple of minor nits below.kernel/workqueue.c EXPORT_SYMBOL(schedule_delayed_work_on); · line 596
Quoted 8 lines
kernel/workqueue.c +0
-2@@ -592,8 +592,6 @@ EXPORT_SYMBOL(schedule_delayed_work_on);* Returns zero on success. * Returns -ve errno on failure. * -* Appears to be racy against CPU hotplug. -* see below,
Quoted 13 lines
kernel/workqueue.c +1
-1* schedule_on_each_cpu() is very slow. */ int schedule_on_each_cpu (work_func_t func)@@ -605,7 +603,7 @@ int schedule_on_each_cpu(work_func_t funif (!works)return -ENOMEM ; -preempt_disable ();/* CPU hotplug */ +get_online_cpus ();/* CPU hotplug */ for_each_online_cpu (cpu) {struct work_struct *work =per_cpu_ptr (works, cpu);kernel/workqueue.c int schedule_on_each_cpu(work_func_t fun · line 615
Quoted 6 lines
kernel/workqueue.c +1
-1@@ -613,7 +611,7 @@ int schedule_on_each_cpu(work_func_t funset_bit (WORK_STRUCT_PENDING ,work_data_bits (work));__queue_work (per_cpu_ptr (keventd_wq->cpu_wq, cpu), work); } -preempt_enable (); +put_online_cpus ();flush_workqueue (keventd_wq);Still racy. To kill the race, please move
flush_workqueue()up, beforeput_online_cpus().kernel/workqueue.c void destroy_workqueue(struct workqueue_ · line 821
Quoted 13 lines
kernel/workqueue.c +2
-0@@ -809,6 +809,7 @@ void destroy_workqueue(struct workqueue_struct cpu_workqueue_struct *cwq;int cpu; +get_online_cpus ();mutex_lock (&workqueue_mutex);list_del (&wq->list);mutex_unlock (&workqueue_mutex);@@ -817,6 +818,7 @@ void destroy_workqueue(struct workqueue_ cwq =per_cpu_ptr (wq->cpu_wq, cpu);cleanup_workqueue_thread (cwq, cpu); } +put_online_cpus ();Correct, but I'd suggest to do
put_online_cpus()earlier, right aftermutex_unlock(&workqueue_mutex).Quoted 7 lines
kernel/workqueue.c +1
-0@@ -830,22 +832,17 @@ static int __devinit workqueue_cpu_callbunsigned int cpu = (unsigned long )hcpu;struct cpu_workqueue_struct *cwq;struct workqueue_struct *wq; +int ret =NOTIFY_OK ; action &= ~CPU_TASKS_FROZEN ;kernel/workqueue.c static int __devinit workqueue_cpu_callb · line 840
Quoted 10 lines
kernel/workqueue.c +0
-7switch (action) { -case CPU_LOCK_ACQUIRE : -mutex_lock (&workqueue_mutex); -return NOTIFY_OK ; - -case CPU_LOCK_RELEASE : -mutex_unlock (&workqueue_mutex); -return NOTIFY_OK ;please remove this emtpy line
Quoted 4 lines
kernel/workqueue.c +0
-0case CPU_UP_PREPARE :cpu_set (cpu, cpu_populated_map); }
Quoted 6 lines
kernel/workqueue.c static int __devinit workqueue_cpu_callb · line 845
Quoted 1 line
kernel/workqueue.c +1
-0+mutex_lock (&workqueue_mutex);We don't need
workqueue_mutexhere. With your patchworkqueue_mutexprotectslist_head, nothing more. But all other callers (create/destroy) should takeget_online_cpus()anyway. This means that we can convertworkqueue_mutextospinlock_t.
Thanks for the review!
Will code these changes up in the next version and post them sometime soon.
Quoted 3 lines
Oleg.
Thanks and Regards
gautham.
Signature · 5 lines
Gautham R Shenoy Linux Technology Center IBM India. "Freedom comes with a price tag of responsibility, which is still a bargain, because Freedom is priceless!"
RFC PATCH 4/4 Remove CPU_DEAD/CPU_UP_CANCELLED handling from workqueue.c
In reply to Gautham R Shenoycleanup_workqueue_thread() in the CPU_DEAD and CPU_UP_CANCELLED path will cause a deadlock if the worker thread is executing a work item which is blocked on get_online_cpus(). This will lead to a irrecoverable hang.
Solution is not to cleanup the worker thread. Instead let it remain even after the cpu goes offline. Since no one can queue any work on an offlined cpu, this thread will be forever sleeping, untill someone onlines the cpu.
--- kernel/workqueue.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
kernel/workqueue.c +6 -7
@@ -30,6 +30,7 @@#include <linux/hardirq.h> #include <linux/mempolicy.h> #include <linux/freezer.h> +#include <linux/cpumask.h> #include <linux/kallsyms.h> #include <linux/debug_locks.h> #include <linux/lockdep.h>
@@ -679,6 +680,7 @@ init_cpu_workqueue(struct workqueue_struspin_lock_init (&cwq->lock);INIT_LIST_HEAD (&cwq->worklist);init_waitqueue_head (&cwq->more_work); + cwq->thread =NULL ;return cwq; }
@@ -712,7 +714,7 @@ static void start_workqueue_thread(strucif (p !=NULL ) {if (cpu >=0 ) -kthread_bind (p, cpu); +set_cpus_allowed (p,cpumask_of_cpu (cpu));wake_up_process (p); } }
@@ -848,6 +850,9 @@ static int __devinit workqueue_cpu_callbswitch (action) {case CPU_UP_PREPARE : +if (likely (cwq->thread !=NULL && + !IS_ERR (cwq->thread))) +break ;if (!create_workqueue_thread (cwq, cpu))break ; printk(KERN_ERR "workqueue [%s] for %i failed\n",
@@ -858,12 +863,6 @@ static int __devinit workqueue_cpu_callbcase CPU_ONLINE :start_workqueue_thread (cwq, cpu);break ; - -case CPU_UP_CANCELED : -start_workqueue_thread (cwq,-1 ); -case CPU_DEAD : -cleanup_workqueue_thread (cwq, cpu); -break ; } }
Signature · 5 lines
Gautham R Shenoy Linux Technology Center IBM India. "Freedom comes with a price tag of responsibility, which is still a bargain, because Freedom is priceless!"
Re: [RFC PATCH 4/4] Remove CPU_DEAD/CPU_UP_CANCELLED handling from workqueue.c
Quoted 10 lines · Gautham R Shenoy
cleanup_workqueue_thread()in theCPU_DEADandCPU_UP_CANCELLEDpath will cause a deadlock if the worker thread is executing a work item which is blocked onget_online_cpus(). This will lead to a irrecoverable hang.Solution is not to cleanup the worker thread. Instead let it remain even after the cpu goes offline. Since no one can queue any work on an offlined cpu, this thread will be forever sleeping, untill someone onlines the cpu.
Imho: not perfect, but acceptable. We can re-introduce cwq->should_stop flag, so that cwq->thread eventually exits after it flushed ->worklist. But see below.
kernel/workqueue.c init_cpu_workqueue(struct workqueue_stru · line 683
Quoted 5 lines
kernel/workqueue.c +1
-0@@ -679,6 +680,7 @@ init_cpu_workqueue(struct workqueue_struspin_lock_init (&cwq->lock);INIT_LIST_HEAD (&cwq->worklist);init_waitqueue_head (&cwq->more_work); + cwq->thread =NULL ;
Not needed, cwq->thread == NULL because alloc_percpu() uses GFP_ZERO.
kernel/workqueue.c static void start_workqueue_thread(struc · line 717
Quoted 8 lines
kernel/workqueue.c +1
-1return cwq; }@@ -712,7 +714,7 @@ static void start_workqueue_thread(strucif (p !=NULL ) {if (cpu >=0 ) -kthread_bind (p, cpu); +set_cpus_allowed (p,cpumask_of_cpu (cpu));
OK, this is understandable, but see below...
kernel/workqueue.c static int __devinit workqueue_cpu_callb · line 854
Quoted 9 lines
kernel/workqueue.c +2
-0wake_up_process (p); } }@@ -848,6 +850,9 @@ static int __devinit workqueue_cpu_callbswitch (action) {case CPU_UP_PREPARE : +if (likely (cwq->thread !=NULL && + !IS_ERR (cwq->thread)))
IS_ERR(cwq->thread) is not possible. cwq->thread is either NULL, or a valid task_struct.
Quoted 6 lines
kernel/workqueue.c +1
-0+break ;if (!create_workqueue_thread (cwq, cpu))break ; printk(KERN_ERR "workqueue [%s] for %i failed\n",@@ -858,12 +863,6 @@ static int __devinit workqueue_cpu_callbcase CPU_ONLINE :
kernel/workqueue.c static int __devinit workqueue_cpu_callb · line 867
Quoted 10 lines
kernel/workqueue.c +0
-6start_workqueue_thread (cwq, cpu);break ; - -case CPU_UP_CANCELED : -start_workqueue_thread (cwq,-1 ); -case CPU_DEAD : -cleanup_workqueue_thread (cwq, cpu); -break ; } }
Unfortunately, this approach seem to have a sublte problem.
Suppose that CPU 0 goes down. Let's denote the corresponding cwq->thread as T. When CPU goes offline, T is not bound to CPU 0 any longer. So far this is OK.
Now suppose that CPU 0 goes online again. There is a window before CPU_ONLINE (note that CPU 0 is already online at this time) binds T back to CPU 0. It is possible that another thread running on CPU 0 does schedule_work() in that window, or it can run on any CPU but calls schedule_delayed_work_on(0).
In this case the work_struct may run on the wrong CPU because T is ready to execute the work_struct, this is bug. work->func() has all rights to expect that cwq->thread is bound to the right CPU.
Let's take cache_reap() as an example. It is critical that this function is really "per cpu", otherwise it can use the wrong per-cpu data, and even the last schedule_delayed_work() is not reliable.
(Sorry, have no time to read the other patches now, will do on weekend).
Oleg.
Quoted 10 lines · Gautham R Shenoy
Patch 1/4: Implements the core refcount + waitqueue model. Patch 2/4: Replaces all the lock_cpu_hotplug/unlock_cpu_hotplug instances
with get_online_cpus()/put_online_cpus()
Patch 3/4: Replaces the subsystem mutexes (we do have three of them now,
insched.c,slab.candworkqueue.c) withget_online_cpus,put_online_cpus.
Patch 4/4: Eliminates theCPU_DEADandCPU_UP_CANCELLEDevent handling
fromworkqueue.c
Quoted 4 lines
The patch series has survived an overnight test with kernbench on i386. and has been tested with Paul Mckenney's latest preemptible rcu code.
Awaiting thy feedback!
Well, afaik, the patch series is fairly clean, and I'm obviously perfectly happy with the approach, so I have no objections.
But it looks buggy. This:
+static void cpu_hotplug_begin(void)
+{
+ mutex_lock(&cpu_hotplug.lock);
+ cpu_hotplug.active_writer = current;
+ while (cpu_hotplug.refcount) {
+ mutex_unlock(&cpu_hotplug.lock);
+ wait_for_completion(&cpu_hotplug.readers_done);
+ mutex_lock(&cpu_hotplug.lock);
+ }
+
+}
drops the cpu_hotplug.lock, which - as far as I can see - means that another process can come in and do the same, and mess up the "active_writer" thing. The oerson that actually *gets* the lock may not be the same one that has "active_writer" set to itself. No? Am I missing something.
So I think this needs (a) more people looking at it (I think I found a bug, who knows if there are more subtle ones lurking) and (b) lots of testing.
Linus
Quoted 19 lines · Linus Torvalds
On Tue, 16 Oct 2007, Gautham R Shenoy wrote:
Well, afaik, the patch series is fairly clean, and I'm obviously perfectly happy with the approach, so I have no objections.
But it looks buggy. This:
+static void
cpu_hotplug_begin(void)
+{
+mutex_lock(&cpu_hotplug.lock);
+ cpu_hotplug.active_writer = current;
+ while (cpu_hotplug.refcount) {
+mutex_unlock(&cpu_hotplug.lock);
+wait_for_completion(&cpu_hotplug.readers_done);
+mutex_lock(&cpu_hotplug.lock);
+ }
+
+}
Quoted 5 lines
drops the cpu_hotplug.lock, which - as far as I can see - means that another process can come in and do the same, and mess up the "
active_writer" thing. The oerson that actually *gets* the lock may not be the same one that has "active_writer" set to itself. No? Am I missing something.
Unless I am reading the patch wrongly, it seems cpu_hotplug_begin() is called while holding the cpu_add_remove_lock mutex. So, another CPU cannot come in and do the same until _cpu_down() is over.
Thanks
Dipankar
Quoted 4 lines · Dipankar Sarma
Unless I am reading the patch wrongly, it seems
cpu_hotplug_begin()is called while holding thecpu_add_remove_lockmutex. So, another CPU cannot come in and do the same until_cpu_down()is over.
Ahh, in that case I take back that objection, although maybe a comment about the required nesting within the other mutex is in order? Or maybe it's there and I just missed it (blush).
Linus
Quoted 11 lines · Linus Torvalds
Quoted 4 lines · Dipankar Sarma
Unless I am reading the patch wrongly, it seems
cpu_hotplug_begin()is called while holding thecpu_add_remove_lockmutex. So, another CPU cannot come in and do the same until_cpu_down()is over.Ahh, in that case I take back that objection, although maybe a comment about the required nesting within the other mutex is in order? Or maybe it's there and I just missed it (blush).
Good point! I'll add the comment before cpu_hotplug_begin().
Quoted 2 lines
Linus
Thanks and Regards
gautham.
Signature · 5 lines
Gautham R Shenoy Linux Technology Center IBM India. "Freedom comes with a price tag of responsibility, which is still a bargain, because Freedom is priceless!"