[PATCH] KVM breaks CPU hotplug

[PATCH] KVM breaks CPU hotplug

[Shaohua Li]

When testing CPU hotplug, I found cpu can't be onlined with kvm enabled
sometimes. The reason is smp_call_function_single is a nop if the thread
is running on the target cpu. I think CPU_ONLINE case doesn't require
the fix as the online CPU isn't plugged into sheduler yet.

Signed-off-by: Shaohua Li <shaohua.li-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index dc7a8c7..806a931 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -2360,6 +2360,7 @@ static int kvm_cpu_hotplug(struct notifier_block *notifier, unsigned long val,
 			   void *v)
 {
 	int cpu = (long)v;
+	int this_cpu;
 
 	switch (val) {
 	case CPU_DOWN_PREPARE:
@@ -2367,8 +2368,13 @@ static int kvm_cpu_hotplug(struct notifier_block *notifier, unsigned long val,
 		printk(KERN_INFO "kvm: disabling virtualization on CPU%d\n",
 		       cpu);
 		decache_vcpus_on_cpu(cpu);
-		smp_call_function_single(cpu, kvm_arch_ops->hardware_disable,
-					 NULL, 0, 1);
+		this_cpu = get_cpu();
+		if (this_cpu == cpu)
+			kvm_arch_ops->hardware_disable(NULL);
+		else
+			smp_call_function_single(cpu,
+				kvm_arch_ops->hardware_disable,  NULL, 0, 1);
+		put_cpu();
 		break;
 	case CPU_ONLINE:
 		printk(KERN_INFO "kvm: enabling virtualization on CPU%d\n",

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] KVM breaks CPU hotplug

[Avi Kivity]

Shaohua Li wrote:
> When testing CPU hotplug, I found cpu can't be onlined with kvm enabled
> sometimes. The reason is smp_call_function_single is a nop if the thread
> is running on the target cpu. I think CPU_ONLINE case doesn't require
> the fix as the online CPU isn't plugged into sheduler yet.
>
>   

I think this is not enough, because:

- this path is preemptible code, so the test (this_cpu == cpu) can run 
one on cpu and execute later code on another
- a virtual machine might be scheduled later on this cpu and oops when 
executing a vmx/svm instruction

I'm not sure how to fix.  Perhaps have _cpu_down() remove the cpu it is 
downing from its affinity mask earlier?  the code is already there, a 
few lines below.

> Signed-off-by: Shaohua Li <shaohua.li-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
> diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
> index dc7a8c7..806a931 100644
> --- a/drivers/kvm/kvm_main.c
> +++ b/drivers/kvm/kvm_main.c
> @@ -2360,6 +2360,7 @@ static int kvm_cpu_hotplug(struct notifier_block *notifier, unsigned long val,
>  			   void *v)
>  {
>  	int cpu = (long)v;
> +	int this_cpu;
>  
>  	switch (val) {
>  	case CPU_DOWN_PREPARE:
> @@ -2367,8 +2368,13 @@ static int kvm_cpu_hotplug(struct notifier_block *notifier, unsigned long val,
>  		printk(KERN_INFO "kvm: disabling virtualization on CPU%d\n",
>  		       cpu);
>  		decache_vcpus_on_cpu(cpu);
> -		smp_call_function_single(cpu, kvm_arch_ops->hardware_disable,
> -					 NULL, 0, 1);
> +		this_cpu = get_cpu();
> +		if (this_cpu == cpu)
> +			kvm_arch_ops->hardware_disable(NULL);
> +		else
> +			smp_call_function_single(cpu,
> +				kvm_arch_ops->hardware_disable,  NULL, 0, 1);
> +		put_cpu();
>  		break;
>  	case CPU_ONLINE:
>  		printk(KERN_INFO "kvm: enabling virtualization on CPU%d\n",
>   


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] KVM breaks CPU hotplug

[Avi Kivity]

Avi Kivity wrote:
> Shaohua Li wrote:
>> When testing CPU hotplug, I found cpu can't be onlined with kvm enabled
>> sometimes. The reason is smp_call_function_single is a nop if the thread
>> is running on the target cpu. I think CPU_ONLINE case doesn't require
>> the fix as the online CPU isn't plugged into sheduler yet.
>>
>>   
>
> I think this is not enough, because:
>
> - this path is preemptible code, so the test (this_cpu == cpu) can run 
> one on cpu and execute later code on another
>> +        this_cpu = get_cpu();

...

>> +        put_cpu();

Sorry, I see you handled that.  But the other argument (scheduling vcpus 
later on same cpu) still holds, I think.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] KVM breaks CPU hotplug

[Li, Shaohua]

>-----Original Message-----
>From: Avi Kivity [mailto:avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org]
>Sent: Monday, March 26, 2007 6:58 PM
>To: Li, Shaohua
>Cc: kvm-devel; Andrew Morton
>Subject: Re: [PATCH] KVM breaks CPU hotplug
>
>Avi Kivity wrote:
>> Shaohua Li wrote:
>>> When testing CPU hotplug, I found cpu can't be onlined with kvm
enabled
>>> sometimes. The reason is smp_call_function_single is a nop if the
thread
>>> is running on the target cpu. I think CPU_ONLINE case doesn't
require
>>> the fix as the online CPU isn't plugged into sheduler yet.
>>>
>>>
>>
>> I think this is not enough, because:
>>
>> - this path is preemptible code, so the test (this_cpu == cpu) can
run
>> one on cpu and execute later code on another
>>> +        this_cpu = get_cpu();
>
>...
>
>>> +        put_cpu();
>
>Sorry, I see you handled that.  But the other argument (scheduling
vcpus
>later on same cpu) still holds, I think.
How about take cpu hotplug lock for kvm_vm_ioctl and kvm_vcpu_ioctl?

Thanks,
Shaohua

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] KVM breaks CPU hotplug

[Avi Kivity]

Li, Shaohua wrote:
> How about take cpu hotplug lock for kvm_vm_ioctl and kvm_vcpu_ioctl?
>
>   

That will serialize calls to these functions, and we want parallel 
execution of kvm_vcpu_ioctl().  We could make it a reader-writer lock, 
but...

I think that adding a

    raw_notifier_call_chain(&cpu_chain, CPU_DYING, NULL);

in take_cpu_down() solve the problem.  The semantics of that would be 
that the chain is executed on the dying cpu, without being allowed to 
fail, and requiring atomic execution.  I don't know whether that would 
acceptable to the cpu hotplug maintainers.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV