* Re: [PATCH 44/51] xen, balloon: Fix CPU hotplug callback registration
@ 2014-02-06 1:18 Boris Ostrovsky
0 siblings, 0 replies; 2+ messages in thread
From: Boris Ostrovsky @ 2014-02-06 1:18 UTC (permalink / raw)
To: srivatsa.bhat
Cc: konrad.wilk, rusty, ego, tglx, mingo, xen-devel, paulus, akpm,
linux, oleg, paulmck, david.vrabel, tj, walken, linux-kernel,
peterz
----- srivatsa.bhat@linux.vnet.ibm.com wrote:
> Subsystems that want to register CPU hotplug callbacks, as well as
> perform
> initialization for the CPUs that are already online, often do it as
> shown
> below:
>
> get_online_cpus();
>
> for_each_online_cpu(cpu)
> init_cpu(cpu);
>
> register_cpu_notifier(&foobar_cpu_notifier);
>
> put_online_cpus();
>
> This is wrong, since it is prone to ABBA deadlocks involving the
> cpu_add_remove_lock and the cpu_hotplug.lock (when running
> concurrently
> with CPU hotplug operations).
>
> Interestingly, the balloon code in xen can actually prevent double
> initialization and hence can use the following simplified form of
> callback
> registration:
>
> register_cpu_notifier(&foobar_cpu_notifier);
>
> get_online_cpus();
>
> for_each_online_cpu(cpu)
> init_cpu(cpu);
>
> put_online_cpus();
>
> A hotplug operation that occurs between registering the notifier and
> calling
> get_online_cpus(), won't disrupt anything, because the code takes care
> to
> perform the memory allocations only once.
>
> So reorganize the balloon code in xen this way to fix the deadlock
> with
> callback registration.
>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
> Cc: xen-devel@lists.xenproject.org
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
>
> drivers/xen/balloon.c | 35 +++++++++++++++++++++++------------
> 1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 37d06ea..afe1a3f 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -592,19 +592,29 @@ static void __init balloon_add_region(unsigned
> long start_pfn,
> }
> }
>
> +static int alloc_balloon_scratch_page(int cpu)
> +{
> + if (per_cpu(balloon_scratch_page, cpu) != NULL)
> + return 0;
> +
> + per_cpu(balloon_scratch_page, cpu) = alloc_page(GFP_KERNEL);
> + if (per_cpu(balloon_scratch_page, cpu) == NULL) {
> + pr_warn("Failed to allocate balloon_scratch_page for cpu %d\n",
> cpu);
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +
> static int balloon_cpu_notify(struct notifier_block *self,
> unsigned long action, void *hcpu)
> {
> int cpu = (long)hcpu;
> switch (action) {
> case CPU_UP_PREPARE:
> - if (per_cpu(balloon_scratch_page, cpu) != NULL)
> - break;
> - per_cpu(balloon_scratch_page, cpu) = alloc_page(GFP_KERNEL);
> - if (per_cpu(balloon_scratch_page, cpu) == NULL) {
> - pr_warn("Failed to allocate balloon_scratch_page for cpu %d\n",
> cpu);
> + if (alloc_balloon_scratch_page(cpu))
> return NOTIFY_BAD;
> - }
> break;
> default:
> break;
> @@ -624,15 +634,16 @@ static int __init balloon_init(void)
> return -ENODEV;
>
> if (!xen_feature(XENFEAT_auto_translated_physmap)) {
> - for_each_online_cpu(cpu)
> - {
> - per_cpu(balloon_scratch_page, cpu) = alloc_page(GFP_KERNEL);
> - if (per_cpu(balloon_scratch_page, cpu) == NULL) {
> - pr_warn("Failed to allocate balloon_scratch_page for cpu %d\n",
> cpu);
> + register_cpu_notifier(&balloon_cpu_notifier);
> +
> + get_online_cpus();
> + for_each_online_cpu(cpu) {
> + if (alloc_balloon_scratch_page(cpu)) {
> + put_online_cpus();
> return -ENOMEM;
Not that original code was doing a particularly thorough job of cleaning up on allocation failure but if it couldn't get memory it would not register the notifier. So perhaps you should unregister it before returning here.
I am also not sure how we were susceptible to the deadlock here since we didn't call get_online_cpus(). (We probably should have but then commit description should say it).
-boris
> }
> }
> - register_cpu_notifier(&balloon_cpu_notifier);
> + put_online_cpus();
> }
>
> pr_info("Initialising balloon driver\n");
^ permalink raw reply [flat|nested] 2+ messages in thread* [PATCH 00/51] CPU hotplug: Fix issues with callback registration
@ 2014-02-05 22:04 Srivatsa S. Bhat
2014-02-05 22:12 ` [PATCH 44/51] xen, balloon: Fix CPU hotplug " Srivatsa S. Bhat
0 siblings, 1 reply; 2+ messages in thread
From: Srivatsa S. Bhat @ 2014-02-05 22:04 UTC (permalink / raw)
To: paulus, oleg, rusty, peterz, tglx, akpm
Cc: mingo, paulmck, tj, walken, ego, linux, linux-kernel, srivatsa.bhat
Hi,
Many subsystems and drivers have the need to register CPU hotplug callbacks
from their init routines and also perform initialization for the CPUs that are
already online. But unfortunately there is no race-free way to achieve this
today.
For example, consider this piece of code:
get_online_cpus();
for_each_online_cpu(cpu)
init_cpu(cpu);
register_cpu_notifier(&foobar_cpu_notifier);
put_online_cpus();
This is not safe because there is a possibility of an ABBA deadlock involving
the cpu_add_remove_lock and the cpu_hotplug.lock.
CPU 0 CPU 1
----- -----
Acquire cpu_hotplug.lock
[via get_online_cpus()]
CPU online/offline operation
takes cpu_add_remove_lock
[via cpu_maps_update_begin()]
Try to acquire
cpu_add_remove_lock
[via register_cpu_notifier()]
CPU online/offline operation
tries to acquire cpu_hotplug.lock
[via cpu_hotplug_begin()]
*** DEADLOCK! ***
Other combinations of callback registration also don't work correctly.
Examples:
register_cpu_notifier(&foobar_cpu_notifier);
get_online_cpus();
for_each_online_cpu(cpu)
init_cpu(cpu);
put_online_cpus();
This can lead to double initialization if a hotplug operation occurs after
registering the notifier and before invoking get_online_cpus().
On the other hand, the following piece of code can miss hotplug events
altogether:
get_online_cpus();
for_each_online_cpu(cpu)
init_cpu(cpu);
put_online_cpus();
^
| Race window; Can miss hotplug events here
v
register_cpu_notifier(&foobar_cpu_notifier);
To solve these issues and provide a race-free method to register CPU hotplug
callbacks, this patchset introduces new variants of the callback registration
APIs that don't hold the cpu_add_remove_lock, and exports the
cpu_add_remove_lock via cpu_maps_update_begin/done() for use by various
subsystems. With this in place, the following code snippet will register a
hotplug callback as well as initialize already online CPUs without any race
conditions.
cpu_maps_update_begin();
for_each_online_cpu(cpu)
init_cpu(cpu);
/* This doesn't take the cpu_add_remove_lock */
__register_cpu_notifier(&foobar_cpu_notifier);
cpu_maps_update_done();
This patchset introduces this infrastructure in patch 1, and performs
tree-wide conversions (to use this model) in the remaining patches.
This patchset has been hosted in the below git tree. It applies cleanly on
v3.14-rc1.
git://github.com/srivatsabhat/linux.git cpuhp-registration-fixes-v1
Oleg, I have incorporated your fix for raid5 in this patchset, and modified
the patch a bit to handle the unregister_cpu_notifier() case as well. If you
are fine with that patch (patch 45), can you kindly provide your Signed-off-by
for that? Thank you!
Oleg Nesterov (1):
md, raid5: Fix CPU hotplug callback registration
Srivatsa S. Bhat (50):
CPU hotplug: Provide lockless versions of callback registration functions
Doc/cpu-hotplug: Specify race-free way to register CPU hotplug callbacks
CPU hotplug, perf: Fix CPU hotplug callback registration
ia64, salinfo: Fix hotplug callback registration
ia64, palinfo: Fix CPU hotplug callback registration
ia64, topology: Fix CPU hotplug callback registration
ia64, err-inject: Fix CPU hotplug callback registration
arm, hw-breakpoint: Fix CPU hotplug callback registration
arm, kvm: Fix CPU hotplug callback registration
s390, cacheinfo: Fix CPU hotplug callback registration
s390, smp: Fix CPU hotplug callback registration
sparc, sysfs: Fix CPU hotplug callback registration
powerpc, sysfs: Fix CPU hotplug callback registration
x86, msr: Fix CPU hotplug callback registration
x86, cpuid: Fix CPU hotplug callback registration
x86, vsyscall: Fix CPU hotplug callback registration
x86, intel, uncore: Fix CPU hotplug callback registration
x86, mce: Fix CPU hotplug callback registration
x86, therm_throt.c: Fix CPU hotplug callback registration
x86, amd, ibs: Fix CPU hotplug callback registration
x86, intel, cacheinfo: Fix CPU hotplug callback registration
x86, intel, rapl: Fix CPU hotplug callback registration
x86, amd, uncore: Fix CPU hotplug callback registration
x86, hpet: Fix CPU hotplug callback registration
x86, pci, amd-bus: Fix CPU hotplug callback registration
x86, oprofile, nmi: Fix CPU hotplug callback registration
x86, kvm: Fix CPU hotplug callback registration
arm64, hw_breakpoint.c: Fix CPU hotplug callback registration
arm64, debug-monitors: Fix CPU hotplug callback registration
powercap, intel-rapl: Fix CPU hotplug callback registration
scsi, bnx2i: Fix CPU hotplug callback registration
scsi, bnx2fc: Fix CPU hotplug callback registration
scsi, fcoe: Fix CPU hotplug callback registration
zsmalloc: Fix CPU hotplug callback registration
acpi-cpufreq: Fix CPU hotplug callback registration
drivers/base/topology.c: Fix CPU hotplug callback registration
clocksource, dummy-timer: Fix CPU hotplug callback registration
intel-idle: Fix CPU hotplug callback registration
oprofile, nmi-timer: Fix CPU hotplug callback registration
octeon, watchdog: Fix CPU hotplug callback registration
thermal, x86-pkg-temp: Fix CPU hotplug callback registration
hwmon, coretemp: Fix CPU hotplug callback registration
hwmon, via-cputemp: Fix CPU hotplug callback registration
xen, balloon: Fix CPU hotplug callback registration
trace, ring-buffer: Fix CPU hotplug callback registration
profile: Fix CPU hotplug callback registration
mm, vmstat: Fix CPU hotplug callback registration
mm, zswap: Fix CPU hotplug callback registration
net/core/flow.c: Fix CPU hotplug callback registration
net/iucv/iucv.c: Fix CPU hotplug callback registration
Documentation/cpu-hotplug.txt | 45 +++++++++
arch/arm/kernel/hw_breakpoint.c | 8 +-
arch/arm/kvm/arm.c | 7 +
arch/arm64/kernel/debug-monitors.c | 6 +
arch/arm64/kernel/hw_breakpoint.c | 7 +
arch/ia64/kernel/err_inject.c | 15 +++
arch/ia64/kernel/palinfo.c | 6 +
arch/ia64/kernel/salinfo.c | 6 +
arch/ia64/kernel/topology.c | 6 +
arch/powerpc/kernel/sysfs.c | 8 +-
arch/s390/kernel/cache.c | 5 +
arch/s390/kernel/smp.c | 13 ++-
arch/sparc/kernel/sysfs.c | 6 +
arch/x86/kernel/cpu/intel_cacheinfo.c | 13 ++-
arch/x86/kernel/cpu/mcheck/mce.c | 8 +-
arch/x86/kernel/cpu/mcheck/therm_throt.c | 5 +
arch/x86/kernel/cpu/perf_event_amd_ibs.c | 6 +
arch/x86/kernel/cpu/perf_event_amd_uncore.c | 7 +
arch/x86/kernel/cpu/perf_event_intel_rapl.c | 9 +-
arch/x86/kernel/cpu/perf_event_intel_uncore.c | 6 +
arch/x86/kernel/cpuid.c | 15 ++-
arch/x86/kernel/hpet.c | 4 +
arch/x86/kernel/msr.c | 16 ++-
arch/x86/kernel/vsyscall_64.c | 6 +
arch/x86/kvm/x86.c | 7 +
arch/x86/oprofile/nmi_int.c | 15 +++
arch/x86/pci/amd_bus.c | 5 +
drivers/base/topology.c | 12 ++
drivers/clocksource/dummy_timer.c | 11 ++
drivers/cpufreq/acpi-cpufreq.c | 7 +
drivers/hwmon/coretemp.c | 14 +--
drivers/hwmon/via-cputemp.c | 14 +--
drivers/idle/intel_idle.c | 12 ++
drivers/md/raid5.c | 90 +++++++++----------
drivers/oprofile/nmi_timer_int.c | 23 +++--
drivers/powercap/intel_rapl.c | 10 ++
drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 12 ++
drivers/scsi/bnx2i/bnx2i_init.c | 12 ++
drivers/scsi/fcoe/fcoe.c | 15 +++
drivers/thermal/x86_pkg_temp_thermal.c | 14 +--
drivers/watchdog/octeon-wdt-main.c | 11 ++
drivers/xen/balloon.c | 35 +++++--
include/linux/cpu.h | 36 +++++++
include/linux/perf_event.h | 16 +++
kernel/cpu.c | 20 ++++
kernel/profile.c | 20 +++-
kernel/trace/ring_buffer.c | 19 ++--
mm/vmstat.c | 6 +
mm/zsmalloc.c | 17 +++-
mm/zswap.c | 8 +-
net/core/flow.c | 8 +-
net/iucv/iucv.c | 121 ++++++++++++-------------
52 files changed, 564 insertions(+), 259 deletions(-)
Regards,
Srivatsa S. Bhat
IBM Linux Technology Center
^ permalink raw reply [flat|nested] 2+ messages in thread* [PATCH 44/51] xen, balloon: Fix CPU hotplug callback registration
2014-02-05 22:04 [PATCH 00/51] CPU hotplug: Fix issues with " Srivatsa S. Bhat
@ 2014-02-05 22:12 ` Srivatsa S. Bhat
0 siblings, 0 replies; 2+ messages in thread
From: Srivatsa S. Bhat @ 2014-02-05 22:12 UTC (permalink / raw)
To: paulus, oleg, rusty, peterz, tglx, akpm
Cc: mingo, paulmck, tj, walken, ego, linux, linux-kernel,
srivatsa.bhat, Konrad Rzeszutek Wilk, Boris Ostrovsky,
David Vrabel, xen-devel, Srivatsa S. Bhat
Subsystems that want to register CPU hotplug callbacks, as well as perform
initialization for the CPUs that are already online, often do it as shown
below:
get_online_cpus();
for_each_online_cpu(cpu)
init_cpu(cpu);
register_cpu_notifier(&foobar_cpu_notifier);
put_online_cpus();
This is wrong, since it is prone to ABBA deadlocks involving the
cpu_add_remove_lock and the cpu_hotplug.lock (when running concurrently
with CPU hotplug operations).
Interestingly, the balloon code in xen can actually prevent double
initialization and hence can use the following simplified form of callback
registration:
register_cpu_notifier(&foobar_cpu_notifier);
get_online_cpus();
for_each_online_cpu(cpu)
init_cpu(cpu);
put_online_cpus();
A hotplug operation that occurs between registering the notifier and calling
get_online_cpus(), won't disrupt anything, because the code takes care to
perform the memory allocations only once.
So reorganize the balloon code in xen this way to fix the deadlock with
callback registration.
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: David Vrabel <david.vrabel@citrix.com>
Cc: xen-devel@lists.xenproject.org
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---
drivers/xen/balloon.c | 35 +++++++++++++++++++++++------------
1 file changed, 23 insertions(+), 12 deletions(-)
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 37d06ea..afe1a3f 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -592,19 +592,29 @@ static void __init balloon_add_region(unsigned long start_pfn,
}
}
+static int alloc_balloon_scratch_page(int cpu)
+{
+ if (per_cpu(balloon_scratch_page, cpu) != NULL)
+ return 0;
+
+ per_cpu(balloon_scratch_page, cpu) = alloc_page(GFP_KERNEL);
+ if (per_cpu(balloon_scratch_page, cpu) == NULL) {
+ pr_warn("Failed to allocate balloon_scratch_page for cpu %d\n", cpu);
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+
static int balloon_cpu_notify(struct notifier_block *self,
unsigned long action, void *hcpu)
{
int cpu = (long)hcpu;
switch (action) {
case CPU_UP_PREPARE:
- if (per_cpu(balloon_scratch_page, cpu) != NULL)
- break;
- per_cpu(balloon_scratch_page, cpu) = alloc_page(GFP_KERNEL);
- if (per_cpu(balloon_scratch_page, cpu) == NULL) {
- pr_warn("Failed to allocate balloon_scratch_page for cpu %d\n", cpu);
+ if (alloc_balloon_scratch_page(cpu))
return NOTIFY_BAD;
- }
break;
default:
break;
@@ -624,15 +634,16 @@ static int __init balloon_init(void)
return -ENODEV;
if (!xen_feature(XENFEAT_auto_translated_physmap)) {
- for_each_online_cpu(cpu)
- {
- per_cpu(balloon_scratch_page, cpu) = alloc_page(GFP_KERNEL);
- if (per_cpu(balloon_scratch_page, cpu) == NULL) {
- pr_warn("Failed to allocate balloon_scratch_page for cpu %d\n", cpu);
+ register_cpu_notifier(&balloon_cpu_notifier);
+
+ get_online_cpus();
+ for_each_online_cpu(cpu) {
+ if (alloc_balloon_scratch_page(cpu)) {
+ put_online_cpus();
return -ENOMEM;
}
}
- register_cpu_notifier(&balloon_cpu_notifier);
+ put_online_cpus();
}
pr_info("Initialising balloon driver\n");
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2014-02-06 1:19 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-06 1:18 [PATCH 44/51] xen, balloon: Fix CPU hotplug callback registration Boris Ostrovsky
-- strict thread matches above, loose matches on Subject: below --
2014-02-05 22:04 [PATCH 00/51] CPU hotplug: Fix issues with " Srivatsa S. Bhat
2014-02-05 22:12 ` [PATCH 44/51] xen, balloon: Fix CPU hotplug " Srivatsa S. Bhat
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®