* [PATCH v3 09/52] arm, hw-breakpoint: Fix CPU hotplug callback registration [not found] <20140310203312.10746.310.stgit@srivatsabhat.in.ibm.com> @ 2014-03-10 20:35 ` Srivatsa S. Bhat 2014-03-10 20:35 ` [PATCH v3 10/52] arm, kvm: " Srivatsa S. Bhat ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Srivatsa S. Bhat @ 2014-03-10 20:35 UTC (permalink / raw) To: linux-arm-kernel 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). Instead, the correct and race-free way of performing the callback registration is: cpu_notifier_register_begin(); for_each_online_cpu(cpu) init_cpu(cpu); /* Note the use of the double underscored version of the API */ __register_cpu_notifier(&foobar_cpu_notifier); cpu_notifier_register_done(); Fix the hw-breakpoint code in arm by using this latter form of callback registration. Cc: Russell King <linux@arm.linux.org.uk> Cc: Ingo Molnar <mingo@kernel.org> Cc: linux-arm-kernel at lists.infradead.org Acked-by: Will Deacon <will.deacon@arm.com> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> --- arch/arm/kernel/hw_breakpoint.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c index 3d44660..3702de8 100644 --- a/arch/arm/kernel/hw_breakpoint.c +++ b/arch/arm/kernel/hw_breakpoint.c @@ -1072,6 +1072,8 @@ static int __init arch_hw_breakpoint_init(void) core_num_brps = get_num_brps(); core_num_wrps = get_num_wrps(); + cpu_notifier_register_begin(); + /* * We need to tread carefully here because DBGSWENABLE may be * driven low on this core and there isn't an architected way to @@ -1088,6 +1090,7 @@ static int __init arch_hw_breakpoint_init(void) if (!cpumask_empty(&debug_err_mask)) { core_num_brps = 0; core_num_wrps = 0; + cpu_notifier_register_done(); return 0; } @@ -1107,7 +1110,10 @@ static int __init arch_hw_breakpoint_init(void) TRAP_HWBKPT, "breakpoint debug exception"); /* Register hotplug and PM notifiers. */ - register_cpu_notifier(&dbg_reset_nb); + __register_cpu_notifier(&dbg_reset_nb); + + cpu_notifier_register_done(); + pm_init(); return 0; } ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 10/52] arm, kvm: Fix CPU hotplug callback registration [not found] <20140310203312.10746.310.stgit@srivatsabhat.in.ibm.com> 2014-03-10 20:35 ` [PATCH v3 09/52] arm, hw-breakpoint: Fix CPU hotplug callback registration Srivatsa S. Bhat @ 2014-03-10 20:35 ` Srivatsa S. Bhat 2014-03-12 23:21 ` Christoffer Dall 2014-03-10 20:39 ` [PATCH v3 30/52] arm64, hw_breakpoint.c: " Srivatsa S. Bhat 2014-03-10 20:39 ` [PATCH v3 31/52] arm64, debug-monitors: " Srivatsa S. Bhat 3 siblings, 1 reply; 9+ messages in thread From: Srivatsa S. Bhat @ 2014-03-10 20:35 UTC (permalink / raw) To: linux-arm-kernel 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). Instead, the correct and race-free way of performing the callback registration is: cpu_notifier_register_begin(); for_each_online_cpu(cpu) init_cpu(cpu); /* Note the use of the double underscored version of the API */ __register_cpu_notifier(&foobar_cpu_notifier); cpu_notifier_register_done(); Fix the kvm code in arm by using this latter form of callback registration. Cc: Christoffer Dall <christoffer.dall@linaro.org> Cc: Gleb Natapov <gleb@kernel.org> Cc: Russell King <linux@arm.linux.org.uk> Cc: Ingo Molnar <mingo@kernel.org> Cc: kvmarm at lists.cs.columbia.edu Cc: kvm at vger.kernel.org Cc: linux-arm-kernel at lists.infradead.org Acked-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> --- arch/arm/kvm/arm.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index bd18bb8..f0e50a0 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -1051,21 +1051,26 @@ int kvm_arch_init(void *opaque) } } + cpu_notifier_register_begin(); + err = init_hyp_mode(); if (err) goto out_err; - err = register_cpu_notifier(&hyp_init_cpu_nb); + err = __register_cpu_notifier(&hyp_init_cpu_nb); if (err) { kvm_err("Cannot register HYP init CPU notifier (%d)\n", err); goto out_err; } + cpu_notifier_register_done(); + hyp_cpu_pm_init(); kvm_coproc_table_init(); return 0; out_err: + cpu_notifier_register_done(); return err; } ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 10/52] arm, kvm: Fix CPU hotplug callback registration 2014-03-10 20:35 ` [PATCH v3 10/52] arm, kvm: " Srivatsa S. Bhat @ 2014-03-12 23:21 ` Christoffer Dall 2014-03-14 5:43 ` Srivatsa S. Bhat 0 siblings, 1 reply; 9+ messages in thread From: Christoffer Dall @ 2014-03-12 23:21 UTC (permalink / raw) To: linux-arm-kernel On Tue, Mar 11, 2014 at 02:05:38AM +0530, Srivatsa S. Bhat 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). > > Instead, the correct and race-free way of performing the callback > registration is: > > cpu_notifier_register_begin(); > > for_each_online_cpu(cpu) > init_cpu(cpu); > > /* Note the use of the double underscored version of the API */ > __register_cpu_notifier(&foobar_cpu_notifier); > > cpu_notifier_register_done(); > > > Fix the kvm code in arm by using this latter form of callback registration. > > Cc: Christoffer Dall <christoffer.dall@linaro.org> > Cc: Gleb Natapov <gleb@kernel.org> > Cc: Russell King <linux@arm.linux.org.uk> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: kvmarm at lists.cs.columbia.edu > Cc: kvm at vger.kernel.org > Cc: linux-arm-kernel at lists.infradead.org > Acked-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> > --- > > arch/arm/kvm/arm.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c > index bd18bb8..f0e50a0 100644 > --- a/arch/arm/kvm/arm.c > +++ b/arch/arm/kvm/arm.c > @@ -1051,21 +1051,26 @@ int kvm_arch_init(void *opaque) > } > } > > + cpu_notifier_register_begin(); > + > err = init_hyp_mode(); > if (err) > goto out_err; > > - err = register_cpu_notifier(&hyp_init_cpu_nb); > + err = __register_cpu_notifier(&hyp_init_cpu_nb); > if (err) { > kvm_err("Cannot register HYP init CPU notifier (%d)\n", err); > goto out_err; > } > > + cpu_notifier_register_done(); > + > hyp_cpu_pm_init(); > > kvm_coproc_table_init(); > return 0; > out_err: > + cpu_notifier_register_done(); > return err; > } > > Just so we're clear, the existing code was simply racy as not prone to deadlocks, right? This makes it clear that the test above for compatible CPUs can be quite easily evaded by using CPU hotplug, but we don't really have a good solution for handling that yet... Hmmm, grumble grumble, I guess if you hotplug unsupported CPUs on a KVM/ARM system for now, stuff will break. In any case: Acked-by: Christoffer Dall <christoffer.dall@linaro.org> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 10/52] arm, kvm: Fix CPU hotplug callback registration 2014-03-12 23:21 ` Christoffer Dall @ 2014-03-14 5:43 ` Srivatsa S. Bhat 2014-03-14 19:10 ` Christoffer Dall 0 siblings, 1 reply; 9+ messages in thread From: Srivatsa S. Bhat @ 2014-03-14 5:43 UTC (permalink / raw) To: linux-arm-kernel On 03/13/2014 04:51 AM, Christoffer Dall wrote: > On Tue, Mar 11, 2014 at 02:05:38AM +0530, Srivatsa S. Bhat 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). >> >> Instead, the correct and race-free way of performing the callback >> registration is: >> >> cpu_notifier_register_begin(); >> >> for_each_online_cpu(cpu) >> init_cpu(cpu); >> >> /* Note the use of the double underscored version of the API */ >> __register_cpu_notifier(&foobar_cpu_notifier); >> >> cpu_notifier_register_done(); >> >> >> Fix the kvm code in arm by using this latter form of callback registration. >> >> Cc: Christoffer Dall <christoffer.dall@linaro.org> >> Cc: Gleb Natapov <gleb@kernel.org> >> Cc: Russell King <linux@arm.linux.org.uk> >> Cc: Ingo Molnar <mingo@kernel.org> >> Cc: kvmarm at lists.cs.columbia.edu >> Cc: kvm at vger.kernel.org >> Cc: linux-arm-kernel at lists.infradead.org >> Acked-by: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> >> --- >> >> arch/arm/kvm/arm.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c >> index bd18bb8..f0e50a0 100644 >> --- a/arch/arm/kvm/arm.c >> +++ b/arch/arm/kvm/arm.c >> @@ -1051,21 +1051,26 @@ int kvm_arch_init(void *opaque) >> } >> } >> >> + cpu_notifier_register_begin(); >> + >> err = init_hyp_mode(); >> if (err) >> goto out_err; >> >> - err = register_cpu_notifier(&hyp_init_cpu_nb); >> + err = __register_cpu_notifier(&hyp_init_cpu_nb); >> if (err) { >> kvm_err("Cannot register HYP init CPU notifier (%d)\n", err); >> goto out_err; >> } >> >> + cpu_notifier_register_done(); >> + >> hyp_cpu_pm_init(); >> >> kvm_coproc_table_init(); >> return 0; >> out_err: >> + cpu_notifier_register_done(); >> return err; >> } >> >> > > Just so we're clear, the existing code was simply racy as not prone to > deadlocks, right? > > This makes it clear that the test above for compatible CPUs can be quite > easily evaded by using CPU hotplug, but we don't really have a good > solution for handling that yet... Hmmm, grumble grumble, I guess if you > hotplug unsupported CPUs on a KVM/ARM system for now, stuff will break. > In this particular case, there was no deadlock possibility, rather the existing code had insufficient synchronization against CPU hotplug. init_hyp_mode() would invoke cpu_init_hyp_mode() on currently online CPUs using on_each_cpu(). If a CPU came online after this point and before calling register_cpu_notifier(), that CPU would remain uninitialized because this subsystem would miss the hot-online event. This patch fixes this bug and also uses the new synchronization method (instead of get/put_online_cpus()) to ensure that we don't deadlock with CPU hotplug. > In any case: > Acked-by: Christoffer Dall <christoffer.dall@linaro.org> > Thanks a lot! Regards, Srivatsa S. Bhat ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 10/52] arm, kvm: Fix CPU hotplug callback registration 2014-03-14 5:43 ` Srivatsa S. Bhat @ 2014-03-14 19:10 ` Christoffer Dall 2014-03-18 10:23 ` [UPDATED PATCH " Srivatsa S. Bhat 0 siblings, 1 reply; 9+ messages in thread From: Christoffer Dall @ 2014-03-14 19:10 UTC (permalink / raw) To: linux-arm-kernel On Fri, Mar 14, 2014 at 11:13:29AM +0530, Srivatsa S. Bhat wrote: > On 03/13/2014 04:51 AM, Christoffer Dall wrote: > > On Tue, Mar 11, 2014 at 02:05:38AM +0530, Srivatsa S. Bhat 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). > >> > >> Instead, the correct and race-free way of performing the callback > >> registration is: > >> > >> cpu_notifier_register_begin(); > >> > >> for_each_online_cpu(cpu) > >> init_cpu(cpu); > >> > >> /* Note the use of the double underscored version of the API */ > >> __register_cpu_notifier(&foobar_cpu_notifier); > >> > >> cpu_notifier_register_done(); > >> > >> > >> Fix the kvm code in arm by using this latter form of callback registration. > >> > >> Cc: Christoffer Dall <christoffer.dall@linaro.org> > >> Cc: Gleb Natapov <gleb@kernel.org> > >> Cc: Russell King <linux@arm.linux.org.uk> > >> Cc: Ingo Molnar <mingo@kernel.org> > >> Cc: kvmarm at lists.cs.columbia.edu > >> Cc: kvm at vger.kernel.org > >> Cc: linux-arm-kernel at lists.infradead.org > >> Acked-by: Paolo Bonzini <pbonzini@redhat.com> > >> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> > >> --- > >> > >> arch/arm/kvm/arm.c | 7 ++++++- > >> 1 file changed, 6 insertions(+), 1 deletion(-) > >> > >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c > >> index bd18bb8..f0e50a0 100644 > >> --- a/arch/arm/kvm/arm.c > >> +++ b/arch/arm/kvm/arm.c > >> @@ -1051,21 +1051,26 @@ int kvm_arch_init(void *opaque) > >> } > >> } > >> > >> + cpu_notifier_register_begin(); > >> + > >> err = init_hyp_mode(); > >> if (err) > >> goto out_err; > >> > >> - err = register_cpu_notifier(&hyp_init_cpu_nb); > >> + err = __register_cpu_notifier(&hyp_init_cpu_nb); > >> if (err) { > >> kvm_err("Cannot register HYP init CPU notifier (%d)\n", err); > >> goto out_err; > >> } > >> > >> + cpu_notifier_register_done(); > >> + > >> hyp_cpu_pm_init(); > >> > >> kvm_coproc_table_init(); > >> return 0; > >> out_err: > >> + cpu_notifier_register_done(); > >> return err; > >> } > >> > >> > > > > Just so we're clear, the existing code was simply racy as not prone to > > deadlocks, right? > > > > This makes it clear that the test above for compatible CPUs can be quite > > easily evaded by using CPU hotplug, but we don't really have a good > > solution for handling that yet... Hmmm, grumble grumble, I guess if you > > hotplug unsupported CPUs on a KVM/ARM system for now, stuff will break. > > > > In this particular case, there was no deadlock possibility, rather the > existing code had insufficient synchronization against CPU hotplug. > > init_hyp_mode() would invoke cpu_init_hyp_mode() on currently online CPUs > using on_each_cpu(). If a CPU came online after this point and before calling > register_cpu_notifier(), that CPU would remain uninitialized because this > subsystem would miss the hot-online event. This patch fixes this bug and > also uses the new synchronization method (instead of get/put_online_cpus()) > to ensure that we don't deadlock with CPU hotplug. > Yes, that was my conclusion as well. Thanks for clarifying. (It could be noted in the commit message as well if you should feel so inclined). > > In any case: > > Acked-by: Christoffer Dall <christoffer.dall@linaro.org> > > > > Thanks a lot! > Thanks, -Christoffer ^ permalink raw reply [flat|nested] 9+ messages in thread
* [UPDATED PATCH v3 10/52] arm, kvm: Fix CPU hotplug callback registration 2014-03-14 19:10 ` Christoffer Dall @ 2014-03-18 10:23 ` Srivatsa S. Bhat 2014-03-18 22:08 ` Christoffer Dall 0 siblings, 1 reply; 9+ messages in thread From: Srivatsa S. Bhat @ 2014-03-18 10:23 UTC (permalink / raw) To: linux-arm-kernel On 03/15/2014 12:40 AM, Christoffer Dall wrote: > On Fri, Mar 14, 2014 at 11:13:29AM +0530, Srivatsa S. Bhat wrote: >> On 03/13/2014 04:51 AM, Christoffer Dall wrote: >>> On Tue, Mar 11, 2014 at 02:05:38AM +0530, Srivatsa S. Bhat 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: >>>> [...] >>> Just so we're clear, the existing code was simply racy as not prone to >>> deadlocks, right? >>> >>> This makes it clear that the test above for compatible CPUs can be quite >>> easily evaded by using CPU hotplug, but we don't really have a good >>> solution for handling that yet... Hmmm, grumble grumble, I guess if you >>> hotplug unsupported CPUs on a KVM/ARM system for now, stuff will break. >>> >> >> In this particular case, there was no deadlock possibility, rather the >> existing code had insufficient synchronization against CPU hotplug. >> >> init_hyp_mode() would invoke cpu_init_hyp_mode() on currently online CPUs >> using on_each_cpu(). If a CPU came online after this point and before calling >> register_cpu_notifier(), that CPU would remain uninitialized because this >> subsystem would miss the hot-online event. This patch fixes this bug and >> also uses the new synchronization method (instead of get/put_online_cpus()) >> to ensure that we don't deadlock with CPU hotplug. >> > > Yes, that was my conclusion as well. Thanks for clarifying. (It could > be noted in the commit message as well if you should feel so inclined). > Please find the patch with updated changelog (and your Ack) below. (No changes in code). From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> Subject: [PATCH] arm, kvm: Fix CPU hotplug callback registration 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). Instead, the correct and race-free way of performing the callback registration is: cpu_notifier_register_begin(); for_each_online_cpu(cpu) init_cpu(cpu); /* Note the use of the double underscored version of the API */ __register_cpu_notifier(&foobar_cpu_notifier); cpu_notifier_register_done(); In the existing arm kvm code, there is no synchronization with CPU hotplug to avoid missing the hotplug events that might occur after invoking init_hyp_mode() and before calling register_cpu_notifier(). Fix this bug and also use the new synchronization method (instead of get/put_online_cpus()) to ensure that we don't deadlock with CPU hotplug. Cc: Gleb Natapov <gleb@kernel.org> Cc: Russell King <linux@arm.linux.org.uk> Cc: Ingo Molnar <mingo@kernel.org> Cc: kvmarm at lists.cs.columbia.edu Cc: kvm at vger.kernel.org Cc: linux-arm-kernel at lists.infradead.org Acked-by: Paolo Bonzini <pbonzini@redhat.com> Acked-by: Christoffer Dall <christoffer.dall@linaro.org> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> --- arch/arm/kvm/arm.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index bd18bb8..f0e50a0 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -1051,21 +1051,26 @@ int kvm_arch_init(void *opaque) } } + cpu_notifier_register_begin(); + err = init_hyp_mode(); if (err) goto out_err; - err = register_cpu_notifier(&hyp_init_cpu_nb); + err = __register_cpu_notifier(&hyp_init_cpu_nb); if (err) { kvm_err("Cannot register HYP init CPU notifier (%d)\n", err); goto out_err; } + cpu_notifier_register_done(); + hyp_cpu_pm_init(); kvm_coproc_table_init(); return 0; out_err: + cpu_notifier_register_done(); return err; } ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [UPDATED PATCH v3 10/52] arm, kvm: Fix CPU hotplug callback registration 2014-03-18 10:23 ` [UPDATED PATCH " Srivatsa S. Bhat @ 2014-03-18 22:08 ` Christoffer Dall 0 siblings, 0 replies; 9+ messages in thread From: Christoffer Dall @ 2014-03-18 22:08 UTC (permalink / raw) To: linux-arm-kernel On 18 March 2014 03:23, Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> wrote: > On 03/15/2014 12:40 AM, Christoffer Dall wrote: >> On Fri, Mar 14, 2014 at 11:13:29AM +0530, Srivatsa S. Bhat wrote: >>> On 03/13/2014 04:51 AM, Christoffer Dall wrote: >>>> On Tue, Mar 11, 2014 at 02:05:38AM +0530, Srivatsa S. Bhat 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: >>>>> > [...] >>>> Just so we're clear, the existing code was simply racy as not prone to >>>> deadlocks, right? >>>> >>>> This makes it clear that the test above for compatible CPUs can be quite >>>> easily evaded by using CPU hotplug, but we don't really have a good >>>> solution for handling that yet... Hmmm, grumble grumble, I guess if you >>>> hotplug unsupported CPUs on a KVM/ARM system for now, stuff will break. >>>> >>> >>> In this particular case, there was no deadlock possibility, rather the >>> existing code had insufficient synchronization against CPU hotplug. >>> >>> init_hyp_mode() would invoke cpu_init_hyp_mode() on currently online CPUs >>> using on_each_cpu(). If a CPU came online after this point and before calling >>> register_cpu_notifier(), that CPU would remain uninitialized because this >>> subsystem would miss the hot-online event. This patch fixes this bug and >>> also uses the new synchronization method (instead of get/put_online_cpus()) >>> to ensure that we don't deadlock with CPU hotplug. >>> >> >> Yes, that was my conclusion as well. Thanks for clarifying. (It could >> be noted in the commit message as well if you should feel so inclined). >> > > Please find the patch with updated changelog (and your Ack) below. > (No changes in code). > perfect, thanks! > From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> > Subject: [PATCH] arm, kvm: Fix CPU hotplug callback registration > > 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). > > Instead, the correct and race-free way of performing the callback > registration is: > > cpu_notifier_register_begin(); > > for_each_online_cpu(cpu) > init_cpu(cpu); > > /* Note the use of the double underscored version of the API */ > __register_cpu_notifier(&foobar_cpu_notifier); > > cpu_notifier_register_done(); > > > In the existing arm kvm code, there is no synchronization with CPU hotplug > to avoid missing the hotplug events that might occur after invoking > init_hyp_mode() and before calling register_cpu_notifier(). Fix this bug > and also use the new synchronization method (instead of get/put_online_cpus()) > to ensure that we don't deadlock with CPU hotplug. > > Cc: Gleb Natapov <gleb@kernel.org> > Cc: Russell King <linux@arm.linux.org.uk> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: kvmarm at lists.cs.columbia.edu > Cc: kvm at vger.kernel.org > Cc: linux-arm-kernel at lists.infradead.org > Acked-by: Paolo Bonzini <pbonzini@redhat.com> > Acked-by: Christoffer Dall <christoffer.dall@linaro.org> > Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> > --- > > arch/arm/kvm/arm.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c > index bd18bb8..f0e50a0 100644 > --- a/arch/arm/kvm/arm.c > +++ b/arch/arm/kvm/arm.c > @@ -1051,21 +1051,26 @@ int kvm_arch_init(void *opaque) > } > } > > + cpu_notifier_register_begin(); > + > err = init_hyp_mode(); > if (err) > goto out_err; > > - err = register_cpu_notifier(&hyp_init_cpu_nb); > + err = __register_cpu_notifier(&hyp_init_cpu_nb); > if (err) { > kvm_err("Cannot register HYP init CPU notifier (%d)\n", err); > goto out_err; > } > > + cpu_notifier_register_done(); > + > hyp_cpu_pm_init(); > > kvm_coproc_table_init(); > return 0; > out_err: > + cpu_notifier_register_done(); > return err; > } > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 30/52] arm64, hw_breakpoint.c: Fix CPU hotplug callback registration [not found] <20140310203312.10746.310.stgit@srivatsabhat.in.ibm.com> 2014-03-10 20:35 ` [PATCH v3 09/52] arm, hw-breakpoint: Fix CPU hotplug callback registration Srivatsa S. Bhat 2014-03-10 20:35 ` [PATCH v3 10/52] arm, kvm: " Srivatsa S. Bhat @ 2014-03-10 20:39 ` Srivatsa S. Bhat 2014-03-10 20:39 ` [PATCH v3 31/52] arm64, debug-monitors: " Srivatsa S. Bhat 3 siblings, 0 replies; 9+ messages in thread From: Srivatsa S. Bhat @ 2014-03-10 20:39 UTC (permalink / raw) To: linux-arm-kernel 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). Instead, the correct and race-free way of performing the callback registration is: cpu_notifier_register_begin(); for_each_online_cpu(cpu) init_cpu(cpu); /* Note the use of the double underscored version of the API */ __register_cpu_notifier(&foobar_cpu_notifier); cpu_notifier_register_done(); Fix the hw-breakpoint code in arm64 by using this latter form of callback registration. Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com> Cc: Ingo Molnar <mingo@kernel.org> Cc: linux-arm-kernel at lists.infradead.org Acked-by: Will Deacon <will.deacon@arm.com> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> --- arch/arm64/kernel/hw_breakpoint.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c index f17f581..bee7897 100644 --- a/arch/arm64/kernel/hw_breakpoint.c +++ b/arch/arm64/kernel/hw_breakpoint.c @@ -913,6 +913,8 @@ static int __init arch_hw_breakpoint_init(void) pr_info("found %d breakpoint and %d watchpoint registers.\n", core_num_brps, core_num_wrps); + cpu_notifier_register_begin(); + /* * Reset the breakpoint resources. We assume that a halting * debugger will leave the world in a nice state for us. @@ -927,7 +929,10 @@ static int __init arch_hw_breakpoint_init(void) TRAP_HWBKPT, "hw-watchpoint handler"); /* Register hotplug notifier. */ - register_cpu_notifier(&hw_breakpoint_reset_nb); + __register_cpu_notifier(&hw_breakpoint_reset_nb); + + cpu_notifier_register_done(); + /* Register cpu_suspend hw breakpoint restore hook */ cpu_suspend_set_dbg_restorer(hw_breakpoint_reset); ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 31/52] arm64, debug-monitors: Fix CPU hotplug callback registration [not found] <20140310203312.10746.310.stgit@srivatsabhat.in.ibm.com> ` (2 preceding siblings ...) 2014-03-10 20:39 ` [PATCH v3 30/52] arm64, hw_breakpoint.c: " Srivatsa S. Bhat @ 2014-03-10 20:39 ` Srivatsa S. Bhat 3 siblings, 0 replies; 9+ messages in thread From: Srivatsa S. Bhat @ 2014-03-10 20:39 UTC (permalink / raw) To: linux-arm-kernel 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). Instead, the correct and race-free way of performing the callback registration is: cpu_notifier_register_begin(); for_each_online_cpu(cpu) init_cpu(cpu); /* Note the use of the double underscored version of the API */ __register_cpu_notifier(&foobar_cpu_notifier); cpu_notifier_register_done(); Fix the debug-monitors code in arm64 by using this latter form of callback registration. Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Russell King <rmk+kernel@arm.linux.org.uk> Cc: Ingo Molnar <mingo@kernel.org> Cc: linux-arm-kernel at lists.infradead.org Acked-by: Will Deacon <will.deacon@arm.com> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> --- arch/arm64/kernel/debug-monitors.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c index 636ba8b..c985531 100644 --- a/arch/arm64/kernel/debug-monitors.c +++ b/arch/arm64/kernel/debug-monitors.c @@ -155,12 +155,16 @@ static struct notifier_block os_lock_nb = { static int debug_monitors_init(void) { + cpu_notifier_register_begin(); + /* Clear the OS lock. */ smp_call_function(clear_os_lock, NULL, 1); clear_os_lock(NULL); /* Register hotplug handler. */ - register_cpu_notifier(&os_lock_nb); + __register_cpu_notifier(&os_lock_nb); + + cpu_notifier_register_done(); return 0; } postcore_initcall(debug_monitors_init); ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-03-18 22:08 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20140310203312.10746.310.stgit@srivatsabhat.in.ibm.com>
2014-03-10 20:35 ` [PATCH v3 09/52] arm, hw-breakpoint: Fix CPU hotplug callback registration Srivatsa S. Bhat
2014-03-10 20:35 ` [PATCH v3 10/52] arm, kvm: " Srivatsa S. Bhat
2014-03-12 23:21 ` Christoffer Dall
2014-03-14 5:43 ` Srivatsa S. Bhat
2014-03-14 19:10 ` Christoffer Dall
2014-03-18 10:23 ` [UPDATED PATCH " Srivatsa S. Bhat
2014-03-18 22:08 ` Christoffer Dall
2014-03-10 20:39 ` [PATCH v3 30/52] arm64, hw_breakpoint.c: " Srivatsa S. Bhat
2014-03-10 20:39 ` [PATCH v3 31/52] arm64, debug-monitors: " 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; as well as URLs for NNTP newsgroup(s).