[PATCH 08/51] arm, hw-breakpoint: Fix CPU hotplug callback registration

[PATCH 08/51] arm, hw-breakpoint: Fix CPU hotplug callback registration

[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).

Instead, the correct and race-free way of performing the callback
registration is:

	cpu_maps_update_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_maps_update_done();


Fix the hw-breakpoint code in arm by using this latter form of callback
registration.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: linux-arm-kernel at lists.infradead.org
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..eaa7fcf 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_maps_update_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_maps_update_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_maps_update_done();
+
 	pm_init();
 	return 0;
 }

[PATCH 08/51] arm, hw-breakpoint: Fix CPU hotplug callback registration

[Will Deacon]

Hi Srivatsa,

On Wed, Feb 05, 2014 at 10:06:04PM +0000, 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).

Hmm, the code in question (for this patch) runs from an arch_initcall. How
can you generate CPU hotplug operations at that stage?

> Instead, the correct and race-free way of performing the callback
> registration is:
> 
> 	cpu_maps_update_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_maps_update_done();
> 
> 
> Fix the hw-breakpoint code in arm by using this latter form of callback
> registration.

I guess you introduce __register_cpu_notifier somewhere earlier in the
series, so it's best if you take this all via your tree.

Will

[PATCH 08/51] arm, hw-breakpoint: Fix CPU hotplug callback registration

[Srivatsa S. Bhat]

Hi Will,

On 02/06/2014 04:27 PM, Will Deacon wrote:
> Hi Srivatsa,
> 
> On Wed, Feb 05, 2014 at 10:06:04PM +0000, 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).
> 
> Hmm, the code in question (for this patch) runs from an arch_initcall. How
> can you generate CPU hotplug operations at that stage?
> 

You are right - in today's design of the init sequence, CPU hotplug
operations can't be triggered at that time during boot.

However, there have been proposals to boot CPUs in parallel along with the
rest of the kernel initialization [1] (and that would need full synchronization
with CPU hotplug even at the initcall stage). Of course this needs a lot of
auditing and modifications to the CPU hotplug notifiers of various subsystems
to make them robust enough to handle the parallel boot; so its not going to
happen very soon. But I felt that it would be a good idea to ensure that we
get the locking/synchronization right, even if the registrations happen very
early during boot today.. you know, just to be on the safer side and also to
make the job easier for whoever that is, who tries to implement parallel
CPU booting again in the future ;-)

[1]. http://thread.gmane.org/gmane.linux.kernel/1246209


>> Instead, the correct and race-free way of performing the callback
>> registration is:
>>
>> 	cpu_maps_update_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_maps_update_done();
>>
>>
>> Fix the hw-breakpoint code in arm by using this latter form of callback
>> registration.
> 
> I guess you introduce __register_cpu_notifier somewhere earlier in the
> series,

Yes, patch 1 adds that API..

> so it's best if you take this all via your tree.
>

Hmm.. I'm not a maintainer myself, so I'm hoping that either Oleg or Rusty
or any of the other CPU hotplug maintainers (Thomas/Peter/Ingo) would be
willing to take these patches through their tree.

Regards,
Srivatsa S. Bhat

[PATCH 08/51] arm, hw-breakpoint: Fix CPU hotplug callback registration

[Will Deacon]

On Thu, Feb 06, 2014 at 11:25:46AM +0000, Srivatsa S. Bhat wrote:
> Hi Will,

Hello,

> On 02/06/2014 04:27 PM, Will Deacon wrote:
> > On Wed, Feb 05, 2014 at 10:06:04PM +0000, 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).
> > 
> > Hmm, the code in question (for this patch) runs from an arch_initcall. How
> > can you generate CPU hotplug operations at that stage?
> > 
> 
> You are right - in today's design of the init sequence, CPU hotplug
> operations can't be triggered at that time during boot.

Phew, so we don't have a bug as the code stands today.

> However, there have been proposals to boot CPUs in parallel along with the
> rest of the kernel initialization [1] (and that would need full synchronization
> with CPU hotplug even at the initcall stage). Of course this needs a lot of
> auditing and modifications to the CPU hotplug notifiers of various subsystems
> to make them robust enough to handle the parallel boot; so its not going to
> happen very soon. But I felt that it would be a good idea to ensure that we
> get the locking/synchronization right, even if the registrations happen very
> early during boot today.. you know, just to be on the safer side and also to
> make the job easier for whoever that is, who tries to implement parallel
> CPU booting again in the future ;-)
> 
> [1]. http://thread.gmane.org/gmane.linux.kernel/1246209

Makes sense, and this seems like a good start.

> > so it's best if you take this all via your tree.
> >
> 
> Hmm.. I'm not a maintainer myself, so I'm hoping that either Oleg or Rusty
> or any of the other CPU hotplug maintainers (Thomas/Peter/Ingo) would be
> willing to take these patches through their tree.

Well, you can have my ack for this patch:

  Acked-by: Will Deacon <will.deacon@arm.com>

Cheers,

Will

[PATCH 08/51] arm, hw-breakpoint: Fix CPU hotplug callback registration

[Srivatsa S. Bhat]

On 02/06/2014 05:09 PM, Will Deacon wrote:
> On Thu, Feb 06, 2014 at 11:25:46AM +0000, Srivatsa S. Bhat wrote:
>> Hi Will,
> 
> Hello,
> 
>> On 02/06/2014 04:27 PM, Will Deacon wrote:
>>> On Wed, Feb 05, 2014 at 10:06:04PM +0000, 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).
>>>
>>> Hmm, the code in question (for this patch) runs from an arch_initcall. How
>>> can you generate CPU hotplug operations at that stage?
>>>
>>
>> You are right - in today's design of the init sequence, CPU hotplug
>> operations can't be triggered at that time during boot.
> 
> Phew, so we don't have a bug as the code stands today.

Yes, that's right.

> 
>> However, there have been proposals to boot CPUs in parallel along with the
>> rest of the kernel initialization [1] (and that would need full synchronization
>> with CPU hotplug even at the initcall stage). Of course this needs a lot of
>> auditing and modifications to the CPU hotplug notifiers of various subsystems
>> to make them robust enough to handle the parallel boot; so its not going to
>> happen very soon. But I felt that it would be a good idea to ensure that we
>> get the locking/synchronization right, even if the registrations happen very
>> early during boot today.. you know, just to be on the safer side and also to
>> make the job easier for whoever that is, who tries to implement parallel
>> CPU booting again in the future ;-)
>>
>> [1]. http://thread.gmane.org/gmane.linux.kernel/1246209
> 
> Makes sense, and this seems like a good start.
> 
>>> so it's best if you take this all via your tree.
>>>
>>
>> Hmm.. I'm not a maintainer myself, so I'm hoping that either Oleg or Rusty
>> or any of the other CPU hotplug maintainers (Thomas/Peter/Ingo) would be
>> willing to take these patches through their tree.
> 
> Well, you can have my ack for this patch:
> 
>   Acked-by: Will Deacon <will.deacon@arm.com>
>

Great! Thanks a lot Will!

Regards,
Srivatsa S. Bhat

[PATCH 09/51] arm, kvm: Fix CPU hotplug callback registration

[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).

Instead, the correct and race-free way of performing the callback
registration is:

	cpu_maps_update_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_maps_update_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: Paolo Bonzini <pbonzini@redhat.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: kvmarm at lists.cs.columbia.edu
Cc: kvm at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.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 1d8248e..e2ef4c4 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -1050,21 +1050,26 @@ int kvm_arch_init(void *opaque)
 		}
 	}
 
+	cpu_maps_update_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_maps_update_done();
+
 	hyp_cpu_pm_init();
 
 	kvm_coproc_table_init();
 	return 0;
 out_err:
+	cpu_maps_update_done();
 	return err;
 }
 

[PATCH 28/51] arm64, hw_breakpoint.c: Fix CPU hotplug callback registration

[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).

Instead, the correct and race-free way of performing the callback
registration is:

	cpu_maps_update_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_maps_update_done();


Fix the hw-breakpoint code in arm64 by using this latter form of callback
registration.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
Cc: linux-arm-kernel at lists.infradead.org
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..24e88d0 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_maps_update_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_maps_update_done();
+
 	/* Register cpu_suspend hw breakpoint restore hook */
 	cpu_suspend_set_dbg_restorer(hw_breakpoint_reset);
 

[PATCH 28/51] arm64, hw_breakpoint.c: Fix CPU hotplug callback registration

[Will Deacon]

On Wed, Feb 05, 2014 at 10:09:45PM +0000, 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:

[...]

> Fix the hw-breakpoint code in arm64 by using this latter form of callback
> registration.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
> Cc: linux-arm-kernel at lists.infradead.org
> 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..24e88d0 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_maps_update_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_maps_update_done();
> +
>  	/* Register cpu_suspend hw breakpoint restore hook */
>  	cpu_suspend_set_dbg_restorer(hw_breakpoint_reset);

  Acked-by: Will Deacon <will.deacon@arm.com>

Will

[PATCH 29/51] arm64, debug-monitors: Fix CPU hotplug callback registration

[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).

Instead, the correct and race-free way of performing the callback
registration is:

	cpu_maps_update_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_maps_update_done();


Fix the debug-monitors code in arm64 by using this latter form of callback
registration.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: linux-arm-kernel at lists.infradead.org
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..959a16b 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_maps_update_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_maps_update_done();
 	return 0;
 }
 postcore_initcall(debug_monitors_init);

[PATCH 29/51] arm64, debug-monitors: Fix CPU hotplug callback registration

[Will Deacon]

On Wed, Feb 05, 2014 at 10:09:58PM +0000, 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:

[...]

> Fix the debug-monitors code in arm64 by using this latter form of callback
> registration.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: linux-arm-kernel at lists.infradead.org
> 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..959a16b 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_maps_update_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_maps_update_done();
>  	return 0;
>  }
>  postcore_initcall(debug_monitors_init);

  Acked-by: Will Deacon <will.deacon@arm.com>

Will