diff mbox

[V3] common/vm_event: Prevent guest locking with large max_vcpus

Message ID 1486660298-11961-1-git-send-email-rcojocaru@bitdefender.com (mailing list archive)
State New, archived
Headers show

Commit Message

Razvan Cojocaru Feb. 9, 2017, 5:11 p.m. UTC
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@bitdefender.com>

---
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(-)

Comments

Tamas K Lengyel Feb. 9, 2017, 5:45 p.m. UTC | #1
On Thu, Feb 9, 2017 at 10:11 AM, Razvan Cojocaru
<rcojocaru@bitdefender.com> 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@bitdefender.com>

Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

>
> ---
> 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@lists.xen.org
> https://lists.xen.org/xen-devel
diff mbox

Patch

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);