Re: [Xen-devel] [PATCH V3] common/vm_event: Prevent guest locking with large max_vcpus

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Feb 9, 2017 at 10:11 AM, Razvan Cojocaru
<rcojocaru@xxxxxxxxxxxxxxx> wrote:
> It is currently possible for the guest to lock when subscribing
> to synchronous vm_events if max_vcpus is larger than the
> number of available ring buffer slots. This patch no longer
> blocks already paused VCPUs, fixing the issue for this use
> case, and wakes up as many blocked VCPUs as there are slots
> available in the ring buffer, eliminating the blockage for
> asynchronous events.
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>

Acked-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>

>
> ---
> Changes since V2:
>  - Fixed typo: re-enabled commented-out code (for testing).
> ---
>  xen/common/vm_event.c | 27 +++++++--------------------
>  1 file changed, 7 insertions(+), 20 deletions(-)
>
> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
> index 82ce8f1..45046d1 100644
> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -127,26 +127,11 @@ static unsigned int vm_event_ring_available(struct 
> vm_event_domain *ved)
>  static void vm_event_wake_blocked(struct domain *d, struct vm_event_domain 
> *ved)
>  {
>      struct vcpu *v;
> -    int online = d->max_vcpus;
>      unsigned int avail_req = vm_event_ring_available(ved);
>
>      if ( avail_req == 0 || ved->blocked == 0 )
>          return;
>
> -    /*
> -     * We ensure that we only have vCPUs online if there are enough free 
> slots
> -     * for their memory events to be processed.  This will ensure that no
> -     * memory events are lost (due to the fact that certain types of events
> -     * cannot be replayed, we need to ensure that there is space in the ring
> -     * for when they are hit).
> -     * See comment below in vm_event_put_request().
> -     */
> -    for_each_vcpu ( d, v )
> -        if ( test_bit(ved->pause_flag, &v->pause_flags) )
> -            online--;
> -
> -    ASSERT(online == (d->max_vcpus - ved->blocked));
> -
>      /* We remember which vcpu last woke up to avoid scanning always linearly
>       * from zero and starving higher-numbered vcpus under high load */
>      if ( d->vcpu )
> @@ -160,13 +145,13 @@ static void vm_event_wake_blocked(struct domain *d, 
> struct vm_event_domain *ved)
>              if ( !v )
>                  continue;
>
> -            if ( !(ved->blocked) || online >= avail_req )
> +            if ( !(ved->blocked) || avail_req == 0 )
>                 break;
>
>              if ( test_and_clear_bit(ved->pause_flag, &v->pause_flags) )
>              {
>                  vcpu_unpause(v);
> -                online++;
> +                avail_req--;
>                  ved->blocked--;
>                  ved->last_vcpu_wake_up = k;
>              }
> @@ -280,8 +265,9 @@ void vm_event_put_request(struct domain *d,
>      int free_req;
>      unsigned int avail_req;
>      RING_IDX req_prod;
> +    struct vcpu *curr = current;
>
> -    if ( current->domain != d )
> +    if ( curr->domain != d )
>      {
>          req->flags |= VM_EVENT_FLAG_FOREIGN;
>  #ifndef NDEBUG
> @@ -316,8 +302,9 @@ void vm_event_put_request(struct domain *d,
>       * See the comments above wake_blocked() for more information
>       * on how this mechanism works to avoid waiting. */
>      avail_req = vm_event_ring_available(ved);
> -    if( current->domain == d && avail_req < d->max_vcpus )
> -        vm_event_mark_and_pause(current, ved);
> +    if( curr->domain == d && avail_req < d->max_vcpus &&
> +        !atomic_read(&curr->vm_event_pause_count) )
> +        vm_event_mark_and_pause(curr, ved);
>
>      vm_event_ring_unlock(ved);
>
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel

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