Message-ID: <CAMJs5B9kVX9urC2PCXY2kbH-j1fA8bge0KQa1z0nK+A689SNYg@mail.gmail.com>
Date:	Tue, 18 Mar 2014 15:08:06 -0700
From:	Christoffer Dall <christoffer.dall@...aro.org>
To:	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
Cc:	ego@...ux.vnet.ibm.com,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>, peterz@...radead.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linuxppc-dev@...abs.org, Paul Mackerras <paulus@...ba.org>,
	Michel Lespinasse <walken@...gle.com>,
	"kvmarm@...ts.cs.columbia.edu" <kvmarm@...ts.cs.columbia.edu>,
	linux-arch@...r.kernel.org, Russell King <linux@....linux.org.uk>,
	mingo@...nel.org, Marc Zyngier <marc.zyngier@....com>,
	Paul McKenney <paulmck@...ux.vnet.ibm.com>,
	linux-pm@...r.kernel.org, Gleb Natapov <gleb@...nel.org>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Thomas Gleixner <tglx@...utronix.de>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>, rjw@...ysocki.net,
	oleg@...hat.com, tj@...nel.org,
	Paolo Bonzini <pbonzini@...hat.com>, akpm@...ux-foundation.org
Subject: Re: [UPDATED PATCH v3 10/52] arm, kvm: Fix CPU hotplug callback registration

On 18 March 2014 03:23, Srivatsa S. Bhat
<srivatsa.bhat@...ux.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@...ux.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@...nel.org>
> Cc: Russell King <linux@....linux.org.uk>
> Cc: Ingo Molnar <mingo@...nel.org>
> Cc: kvmarm@...ts.cs.columbia.edu
> Cc: kvm@...r.kernel.org
> Cc: linux-arm-kernel@...ts.infradead.org
> Acked-by: Paolo Bonzini <pbonzini@...hat.com>
> Acked-by: Christoffer Dall <christoffer.dall@...aro.org>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@...ux.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;
>  }
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists