RE: [Xen-devel] [Patch] continue_hypercall_on_cpu rework using tasklets

["Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx>]

>-----Original Message-----
>From: Keir Fraser [mailto:keir.fraser@xxxxxxxxxxxxx]
>Sent: Thursday, April 15, 2010 7:07 PM
>To: Jiang, Yunhong; Juergen Gross
>Cc: xen-devel@xxxxxxxxxxxxxxxxxxx; Yu, Ke
>Subject: Re: [Xen-devel] [Patch] continue_hypercall_on_cpu rework using 
>tasklets
>
>On 15/04/2010 10:59, "Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx> wrote:
>
>>> Actually that's a good example because it now won't work, but for other
>>> reasons! The hypercall continuation can interrupt another vcpu's execution,
>>> and then try to synchronously pause that vcpu. Which will deadlock.
>>>
>>> Luckily I think we can re-jig this code to freeze_domains() before doing the
>>> continue_hypercall_on_cpu(). I've cc'ed one of the CPU RAS guys. :-)
>>
>> Hmm, I have cc'ed one of the PM guys because it is enter_state :-)
>> Can we add check in vcpu_sleep_sync() for current? It is meaningless to
>> cpu_relax for current vcpu in that situation, especially if we are not in irq
>> context.
>> I'm not sure why in freeze_domains it only checkes dom0's vcpu for current,
>> instead of all domains.
>
>Well actually pausing any vcpu from within the hypercall continuation is
>dangerous. The softirq handler running the hypercall continuation may have
>interrupted some running VCPU X. And the VCPU Y that the continuation is
>currently trying to pause may itself be trying to pause X. So we can get a
>deadlock that way. The freeze_domains() *has* to be pulled outside of the
>hypercall continuation.
>
>It's a little bit similar to the super-subtle stop_machine_run deadlock
>possibility I just emailed to you a second ago. :-)

Thanks for pointing out the stop_machine_run deadlock issue.

After more consideration and internally discussion, seems the key point is, the 
tasklet softirq is something like getting a lock for the current vcpu's 
state(i.e. no one else could change that state unless this softirq is 
finished). So any block action in softirq context, not just vcpu_pause_sync, is 
dangerous and should be avoided (we can't get a lock and do block action per my 
understanding).
This is because vcpu's state can only be changed by schedule softirq (am I 
right on this?), while schedule softirq can't prempt other softirq. So, more 
generally, anything that will be updated by a softirq context, and will be 
syncrhozed/blocking waitied in xen's vcpu context is in fact a implicit lock 
held by the softirq.

To the tricky bug on the stop_machine_run(), I think it is in fact similar to 
the cpu_add_remove_lock. The stop_machine_run() is a block action, so we must 
make sure no one will be blocking to get the lock that is held by 
stop_machine_run() already. At that time, we change all components that try to 
get the cpu_add_remove_lock to be try_lock.

The changes caused by the tasklet is, a new implicit lock is added, i.e. the 
vcpu's state.
The straightforward method is like the cpu_add_remove_lock: make everything 
that waiting for the vcpu state change to do softirq between the checking. 
Maybe the cleaner way is your previous suggestion, that is, put the 
stop_machine_run() in the idle_vcpu(), another way is, turn back to the 
original method, i.e. do it in the schedule_tail.

Also We are not sure why the continue_hypercall_on_cpu is changed to use 
tasklet. What's the benifit for it? At least I think this is quite confusing, 
because per our understanding, usually hypercall is assumed to execut in 
current context, while this change break the assumption. So any hypercall that 
may use this _c_h_o_c, and any function called by that hypercall, should be 
aware of this, I'm not sure if this is really so correct, at least it may cause 
trouble if someone use this without realize the limitation. From Juergen 
Gross's mail, it seems for cpupool, but I have no idea of the cpupool :-(

--jyh


>
> -- Keir
>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel