On 18 March 2014 03:23, Srivatsa S. Bhat
<srivatsa.bhat@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxx>
> Cc: Russell King <linux@xxxxxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: kvmarm@xxxxxxxxxxxxxxxxxxxxx
> Cc: kvm@xxxxxxxxxxxxxxx
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Acked-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Acked-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@xxxxxxxxxxxxxxxxxx>
> ---
>
> 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;
> }
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
