Re: [PATCH 1/2] vm_event: sync domctl

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 12/23/2015 08:11 PM, Tamas K Lengyel wrote:
> 
> 
> On Wed, Dec 23, 2015 at 6:17 PM, Andrew Cooper
> <andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>> wrote:
> 
>     On 23/12/2015 15:41, Razvan Cojocaru wrote:
>     > On 12/23/2015 04:53 PM, Tamas K Lengyel wrote:
>     >> Introduce new vm_event domctl option which allows an event subscriber
>     >> to request all vCPUs not currently pending a vm_event request to be paused,
>     >> thus allowing the subscriber to sync up on the state of the domain. This
>     >> is especially useful when the subscribed wants to disable certain events
>     >> from being delivered and wants to ensure no more requests are pending on the
>     >> ring before doing so.
>     >>
>     >> Cc: Ian Jackson <ian.jackson@eu.citrix.com <mailto:ian.jackson@eu.citrix.com>>
>     >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com
>     <mailto:stefano.stabellini@eu.citrix.com>>
>     >> Cc: Ian Campbell <ian.campbell@citrix.com <mailto:ian.campbell@citrix.com>>
>     >> Cc: Wei Liu <wei.liu2@citrix.com <mailto:wei.liu2@citrix.com>>
>     >> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>>
>     >> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com <mailto:tamas@tklengyel.com>>
>     > This certainly looks very interesting. Would xc_domain_pause() not be
>     > enough for your use case then?
> 
>     I second this query.  I would have thought xc_domain_pause() does
>     exactly what you want in this case.
> 
> 
> The problem is in what order the responses are processed. I may not be
> correct about the logic but here is what my impression was:
> xc_domain_unpause resumes all vCPUs even if there is still a vm_event
> response that has not been processed. Now, if the subscriber set
> response flags (altp2m switch, singlestep toggle, etc) those actions
> would not be properly performed on the vCPU before it's resumed. If the
> subscriber processes all requests and signals via the event channel that
> the responses are on the ring, then calls xc_domain_unpause, we can
> still have a race between processing the responses from the ring and
> unpausing the vCPU.
>  
> 
>     The code provided is racy, as it is liable to alter which pause
>     references it takes/releases depending on what other pause/unpause
>     actions are being made.
> 
> 
> It's understood that the user would not use xc_domain_pause/unpause
> while using vm_event responses with response flags specified. Even then,
> it was already racy IMHO if the user called xc_domain_unpause before
> processing requests from the vm_event ring that originally paused the
> vCPU, so this doesn't change that situation.

There are a bunch of checks in vcpu_wake() (xen/common/schedule.c) that
I've always assumed guard against the problem you're describing. I may
be wrong (I don't have any experience with the scheduling code), but
even if I am, I still think having xc_domain_pause() /
xc_domain_unpause() behave correctly is better than adding a new libxc
function. Is that an unreasonable goal?


Thanks,
Razvan