Re: [Xen-devel] VM_EVENT_FLAG_SET_REGISTERS and sync_vcpu_execstate()

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 08/08/2016 09:01 PM, Tamas K Lengyel wrote:
> On Mon, Aug 8, 2016 at 10:29 AM, Razvan Cojocaru
> <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> On 08/08/16 18:52, Tamas K Lengyel wrote:
>>>> I think the issue might be that vm_event_vcpu_pause() uses
>>>>> vcpu_pause_nosync(), and it's being used everywhere we pause the VCPU in
>>>>> the course of sending out a vm_event.
>>>>>
>>>>> So this creates the premise for a race condition: either some more
>>>>> things happen between sending out the vm_event and replying to it that
>>>>> cause v->arch.user_regs to be synchronized - in which case everything
>>>>> works (this has been the case when I was reading the VCPU context via a
>>>>> domctl that did vcpu_pause()) -, or not, in which case all bets are off.
>>>>>
>>>>> In this case, we should either drop vm_event_vcpu_pause() completely and
>>>>> just use vcpu_pause() everywhere, modify it to use vcpu_sleep_sync()
>>>>> (and basically turn it into vcpu_pause()), or only sync in
>>>>> vm_event_set_registers() as I've suggested.
>>>>>
>>> I would say just using vcpu_pause() would make sense as it's the least
>>> complicated route. Is there any downside of doing the sync() in all
>>> cases? Was the current way implemented perhaps the way it is for
>>> performance reasons? If so, is it noticeable at all?
>>
>> I was hoping you'd know why it's implemented like this :), I think maybe
>> Tim Deegan (CCd, hopefully the email address is not out of date) did the
>> original implementation?
>>
>> That's why I proposed to only sync in vm_event_set_registers() - for all
>> the other cases we can then keep the current performance (if
>> significant). The least complicated route (at least as far as the patch
>> changes go) I think is also this one, since it only requires a new line
>> of code in vm_event_set_registers() - using vcpu_pause() everywhere else
>> requires removing vm_event_pause_vcpu() as well as replacing the call
>> everywhere else.
>>
> 
> Using _nosync() predates me touching this code by a couple years,
> according to git blame it goes all the way back to when mem_access was
> introduced:
> 
> commit fbbedcae8c0c5374f8c0a869f49784b37baf04bb
> Joe Epstein <jepstein98@xxxxxxxxx>
> Date:   Fri Jan 7 11:54:40 2011 +0000
> 
>     mem_access: mem event additions for access
> 
> My only concern with changing to sync only in the set registers route
> is that we potentially keep a buggy interface where we might run into
> this problem in the future. IMHO just shifting all cases to do sync()
> is safer, provided the performance difference is unnoticeable.

Actually looking at the code again, this is vcpu_pause():

 875 void vcpu_pause(struct vcpu *v)
 876 {
 877     ASSERT(v != current);
 878     atomic_inc(&v->pause_count);
 879     vcpu_sleep_sync(v);
 880 }
 881
 882 void vcpu_pause_nosync(struct vcpu *v)
 883 {
 884     atomic_inc(&v->pause_count);
 885     vcpu_sleep_nosync(v);
 886 }

and this is vm_event_vcpu_pause():

751 void vm_event_vcpu_pause(struct vcpu *v)
752 {
753     ASSERT(v == current);
754
755     atomic_inc(&v->vm_event_pause_count);
756     vcpu_pause_nosync(v);
757 }

If we want to preserve the vm_event-specific reference counter
(v->vm_event_pause_count) we'd use vcpu_pause() instead of
vcpu_pause_nosync(). But vcpu_pause() wants to be used on a VCPU !=
current (see the ASSERT()). This is possibly why vcpu_pause_nosync() has
been chosen over vcpu_pause().

I'll try to remove the ASSERT() and see if anything explodes, but it's
looking increasingly like the smallest change is the one I've initially
proposed.


Thanks,
Razvan

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