Re: [Xen-devel] [PATCH 0/3] VMX: Properly handle pi descriptor and per-cpu blocking list

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Fri, 2016-06-24 at 06:11 +0000, Wu, Feng wrote:
> > -----Original Message-----
> > From: Dario Faggioli [mailto:dario.faggioli@xxxxxxxxxx]
> > No, because we call cpu_disable_scheduler() from __cpu_disable(),
> > only
> > when system state is SYS_STATE_suspend already, and hence we take
> > the
> > then branch of the 'if', which does never return an error.
> Thanks for the elaboration. I find __cpu_disable() can be called with
> system state not being SYS_STATE_suspend. Here is my experiment:
> 
> 1. Launch a guest and pin vCPU 3 to pCPU 3 (which makes the
> experiment simpler)
> 2. offline pCPU 3 via "xen-hptool cpu-offline 3"
> 
Ah, yes, of course. I should have included cpu-hot(un)plug in my
analysis in the first place, sorry for not doing so.

> The call path of the above steps is:
> arch_do_sysctl()
>   -> cpu_down_helper()
>     -> cpu_down()
>       -> take_cpu_down()
>         -> __cpu_disable()
>           -> cpu_disable_scheduler() (enter the 'else'  part)
> 
Right, and the important part is this one:

> int stop_machine_run(int (*fn)(void *), void *data, unsigned int cpu)
> {
> 
> ......
> 
> point 1
> 
>     for_each_cpu ( i, &allbutself )
>         tasklet_schedule_on_cpu(&per_cpu(stopmachine_tasklet, i), i);
> 
> point 2
> ......
> 
> }
> 
> at point 1 above, 
> vCPU3->runstate.state: 0, vCPU3->is_running: 1
> while at point 2 above:
> vCPU3->runstate.state: 1, vCPU3->is_running: 0
> 
This is exactly as you describe. cpu hotplug is done in stop machine
context. Check the comment close to the definition of stop_machine_run:

/**
 * stop_machine_run: freeze the machine on all CPUs and run this function
 * @fn: the function to run
 * @data: the data ptr for the @fn()
 * @cpu: the cpu to run @fn() on (or all, if @cpu == NR_CPUS).
 *
 * Description: This causes every other cpu to enter a safe point, with
 * each of which disables interrupts, and finally interrupts are disabled
 * on the current CPU.  The result is that none is holding a spinlock
 * or inside any other preempt-disabled region when @fn() runs.
 *
 * This can be thought of as a very heavy write lock, equivalent to
 * grabbing every spinlock in the kernel. */

As you discovered yourself, this is achieved by forcing the execution
of a tasklet on all the pcpus, which include pCPU 3 of your example.

So, vCPU 3 was running, but then some called stop_machine_run(), which
causes the descheduling of vCPU 3, and the execution of the stopmachine
tasklet.

Thet's why you find is_running to be 0, and that's why  we never return
EAGAIN.

> I tested it for many times and got the same result. I am not sure the
> vcpu
> state transition just happens to occurs here or not? If the
> transition doesn't
> happen and is_running is still 1 when we get vcpu_migrate() in
> cpu_disable_scheduler() in the above case, should it be a problem?
>
I'm not sure what you mean here (in particular with "just happen to
occur"). If you're wondering whether the fact that vCPU 3 gets
descheduled happens by chance or by design, it's indeed the latter, and
so, no, we don't have a problem with this code path.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel