Skip to contentSkip to conversation

Mem event ring management overhaul

PATCH In review Andres Lagar-Cavilla 3 participants 8 messages Dec 5, 2011 – Dec 7, 2011

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Mem event ring management overhaul

Ensure no guest events are ever lost in the mem event ring.

This is one of two outstanding proposals to solve this issue. One key difference between them being that ours does not necessitate wait queues.

Instead, we rely on foreign domain retry (already in place), preempting hypercalls that may cause unbounded guest events (such as decrease_reservation), and ensuring there is always space left in the ring for each guest vcpu to place at least one event.

The patch has been refreshed to apply on top of 62ff6a318c5d, and untangled from other mem event modifications that are essentially orthogonal and can go in independently.

 xen/common/memory.c             |   29 +++++-
 xen/arch/x86/hvm/hvm.c          |   21 ++-
 xen/arch/x86/mm/mem_event.c     |  203 +++++++++++++++++++++++++++++----------
 xen/arch/x86/mm/mem_sharing.c   |   17 ++-
 xen/arch/x86/mm/p2m.c           |   47 +++++----
 xen/common/memory.c             |    7 +-
 xen/include/asm-x86/mem_event.h |   16 ++-
 xen/include/asm-x86/p2m.h       |    6 +-
 xen/include/xen/mm.h            |    2 +
 xen/include/xen/sched.h         |    5 +-
 10 files changed, 257 insertions(+), 96 deletions(-)
#1 lore raw

PATCH 1/1 Allow decrease_reservation to be preempted if remove_page returns negative

 xen/common/memory.c |  29 ++++++++++++++++++++++++++++-
 1 files changed, 28 insertions(+), 1 deletions(-)

This will allow for handling of full paging rings in a more graceful manner. ATM, remove_page does not return negative values, so this code is not yet exercised.

xen/common/memory.c +28 -1
diff -r 790cd814bee8 -r 4c4a9ed0728c xen/common/memory.c
@@ -226,6 +226,8 @@ static void decrease_reservation(struct 
 
     for ( i = a->nr_done; i < a->nr_extents; i++ )
     {
+        int one_remove_succeeded = 0;
+
         if ( hypercall_preempt_check() )
         {
             a->preempted = 1;
@@ -255,8 +257,33 @@ static void decrease_reservation(struct 
             continue;
 
         for ( j = 0; j < (1 << a->extent_order); j++ )
-            if ( !guest_remove_page(a->domain, gmfn + j) )
+        {
+            int rv = guest_remove_page(a->domain, gmfn + j);
+            /* negative rv is a result of a mem event ring full
+             * in the presence of paging. We preempt the hypercall */
+            if ( rv < 0 )
+            {
+                /* Indicate we're done with this extent */
+                if ((j+1) == (1 << a->extent_order))
+                    i++;
+                a->preempted = 1;
+                raise_softirq(SCHEDULE_SOFTIRQ);
                 goto out;
+            }
+            /* rv of zero means we tried to remove a gfn with no backing
+             * mfn. It can be a bad argument, or, a continuation in the midst
+             * of an extent. Heuristically determine second case. */
+            if ( !rv )
+            {
+                /* Has to be first extent */
+                if ( i != a->nr_done )
+                    goto out;
+                /* No previous remove in this extent must have succeeded */
+                if ( one_remove_succeeded )
+                    goto out;
+            } else
+                one_remove_succeeded = 1;
+        }
     }
 
  out:
#3 lore raw

PATCH 1/1 Improve ring management for memory events. Do not lose guest events

In reply to Andres Lagar-Cavilla
 xen/arch/x86/hvm/hvm.c          |   21 ++-
 xen/arch/x86/mm/mem_event.c     |  203 +++++++++++++++++++++++++++++----------
 xen/arch/x86/mm/mem_sharing.c   |   17 ++-
 xen/arch/x86/mm/p2m.c           |   47 +++++----
 xen/common/memory.c             |    7 +-
 xen/include/asm-x86/mem_event.h |   16 ++-
 xen/include/asm-x86/p2m.h       |    6 +-
 xen/include/xen/mm.h            |    2 +
 xen/include/xen/sched.h         |    5 +-
 9 files changed, 229 insertions(+), 95 deletions(-)

The memevent code currently has a mechanism for reserving space in the ring before putting an event, but each caller must individually ensure that the vCPUs are correctly paused if no space is available.

This fixes that issue by reversing the semantics: we ensure that enough space is always left for one event per vCPU in the ring. If, after putting the current request, this constraint will be violated by the current vCPU when putting putting another request in the ring, we pause the vCPU.

For mem events caused by foreign vcpus, we simply drop the event if the space constraint will be violated. Foreign vcpus are expected to retry mapping operations (and thus the associated paging populate mem events will be re issued).

Finally, guests can cause an unbound number of paging drop events via the balloon -> decrease_reservation hypercall. Thus, notify the hypercall there is no more space in the ring so a continuation is scheduled.

With all these mechanisms, no guest events are lost and there is no need for wait-queues. Our testing includes 1. ballooning down 512 MiBs; 2. a mem event mode in which every page access in a four-vCPU guest results in an event, with no vCPU pausing, and the four vCPUs touching all RAM. No guest events were lost in either case, and qemu-dm had no mapping problems.

Finally, mark_and_pause_vcpus was misleading, beacause it wasn't strictly pausing the vcpu's, rather sending them to sleep. Change to actual vcpu_pause, which protects wakeups via an atomic count. This is useful when an event pauses a vcpu and the vcpu gets mark and paused due to ring congestion.

xen/arch/x86/hvm/hvm.c +15 -6
diff -r 4c4a9ed0728c -r d1d238616c0c xen/arch/x86/hvm/hvm.c
@@ -4119,7 +4119,6 @@ static int hvm_memory_event_traps(long p
     struct vcpu* v = current;
     struct domain *d = v->domain;
     mem_event_request_t req;
-    int rc;
 
     if ( !(p & HVMPME_MODE_MASK) ) 
         return 0;
@@ -4127,10 +4126,7 @@ static int hvm_memory_event_traps(long p
     if ( (p & HVMPME_onchangeonly) && (value == old) )
         return 1;
     
-    rc = mem_event_check_ring(d, &d->mem_event->access);
-    if ( rc )
-        return rc;
-    
+  
     memset(&req, 0, sizeof(req));
     req.type = MEM_EVENT_TYPE_ACCESS;
     req.reason = reason;
@@ -4150,7 +4146,20 @@ static int hvm_memory_event_traps(long p
         req.gla_valid = 1;
     }
     
-    mem_event_put_request(d, &d->mem_event->access, &req);
+    if ( mem_event_put_request(d, &d->mem_event->access, &req) == -ENOSYS )
+    {
+        /* rc == -ENOSYS
+         * If there was no ring to send the event, then simply unpause the
+         * vcpu (if we had previously paused it). It will retry the 
+         * instruction (with the exception "handled") and go on */
+        if ( req.flags & MEM_EVENT_FLAG_VCPU_PAUSED ) 
+            vcpu_unpause(v);
+        /* rc == -EBUSY
+         * If the ring is full, the vcpu has been marked and paused,
+         * and the exception has been communicated to the consumer.
+         * Once there is room in the ring, the vcpu will be woken up 
+         * and will retry. */
+    }
     
     return 1;
 }
xen/arch/x86/mm/mem_event.c +149 -54

270 lines not on this page · raw message

xen/arch/x86/mm/mem_sharing.c +14 -3
diff -r 4c4a9ed0728c -r d1d238616c0c xen/arch/x86/mm/mem_sharing.c
@@ -281,12 +281,20 @@ static struct page_info* mem_sharing_all
     vcpu_pause_nosync(v);
     req.flags |= MEM_EVENT_FLAG_VCPU_PAUSED;
 
-    if(mem_event_check_ring(d, &d->mem_event->share)) return page;
-
     req.gfn = gfn;
     req.p2mt = p2m_ram_shared;
     req.vcpu_id = v->vcpu_id;
-    mem_event_put_request(d, &d->mem_event->share, &req);
+
+    /* If there is no ring, and we're out of memory, then
+     * there is no way out. */
+    if ( mem_event_put_request(d, &d->mem_event->share, 
+                                &req) == -ENOSYS )
+    {
+        gdprintk(XENLOG_ERR, 
+                 "Failed alloc on unshare path for %hu and no ring "
+                 "to upcall\n", d->domain_id);
+        domain_crash(d);
+    }
 
     return page;
 }
@@ -310,6 +318,9 @@ int mem_sharing_sharing_resume(struct do
             vcpu_unpause(d->vcpu[rsp.vcpu_id]);
     }
 
+    /* Unpause any domains that were paused because the ring was full */
+    mem_event_unpause_vcpus(d, &d->mem_event->paging);
+
     return 0;
 }
 
xen/arch/x86/mm/p2m.c +26 -21

125 lines not on this page · raw message

xen/common/memory.c +5 -2
diff -r 4c4a9ed0728c -r d1d238616c0c xen/common/memory.c
@@ -165,10 +165,13 @@ int guest_remove_page(struct domain *d, 
     mfn = mfn_x(get_gfn(d, gmfn, &p2mt)); 
     if ( unlikely(p2m_is_paging(p2mt)) )
     {
+        int rc; 
         guest_physmap_remove_page(d, gmfn, mfn, 0);
-        p2m_mem_paging_drop_page(d, gmfn);
+        rc = p2m_mem_paging_drop_page(d, gmfn);
         put_gfn(d, gmfn);
-        return 1;
+        /* drop_page returns zero on success, we return 1 on success.
+         * drop_page returns negative on error, never returns 1. */
+        return rc ? rc : 1;
     }
 #else
     mfn = gmfn_to_mfn(d, gmfn);
xen/include/asm-x86/mem_event.h +11 -5
diff -r 4c4a9ed0728c -r d1d238616c0c xen/include/asm-x86/mem_event.h
@@ -25,12 +25,18 @@
 #define __MEM_EVENT_H__
 
 /* Pauses VCPU while marking pause flag for mem event */
-void mem_event_mark_and_pause(struct vcpu *v);
+void mem_event_mark_and_pause(struct vcpu *v, struct mem_event_domain *med);
 int mem_event_check_ring(struct domain *d, struct mem_event_domain *med);
-void mem_event_put_req_producers(struct mem_event_domain *med);
-void mem_event_put_request(struct domain *d, struct mem_event_domain *med, mem_event_request_t *req);
-int mem_event_get_response(struct mem_event_domain *med, mem_event_response_t *rsp);
-void mem_event_unpause_vcpus(struct domain *d);
+void mem_event_unpause_vcpus(struct domain *d, struct mem_event_domain *med);
+
+/* Returns 0 on success, -ENOSYS if there is no ring, -EBUSY if there is 
+ * a ring or no space. FOr success or -EBUSY. the vCPU is left blocked to 
+ * ensure that the ring does not lose events.  In general, put_request should 
+ * not fail, as it attempts to ensure that each vCPU can put at least one req. */
+int mem_event_put_request(struct domain *d, struct mem_event_domain *med,
+                            mem_event_request_t *req);
+int mem_event_get_response(struct mem_event_domain *med,
+                            mem_event_response_t *rsp);
 
 int mem_event_domctl(struct domain *d, xen_domctl_mem_event_op_t *mec,
                      XEN_GUEST_HANDLE(void) u_domctl);
xen/include/asm-x86/p2m.h +3 -3
diff -r 4c4a9ed0728c -r d1d238616c0c xen/include/asm-x86/p2m.h
@@ -475,7 +475,7 @@ int p2m_mem_paging_nominate(struct domai
 /* Evict a frame */
 int p2m_mem_paging_evict(struct domain *d, unsigned long gfn);
 /* Tell xenpaging to drop a paged out frame */
-void p2m_mem_paging_drop_page(struct domain *d, unsigned long gfn);
+int p2m_mem_paging_drop_page(struct domain *d, unsigned long gfn);
 /* Start populating a paged out frame */
 void p2m_mem_paging_populate(struct domain *d, unsigned long gfn);
 /* Prepare the p2m for paging a frame in */
@@ -483,8 +483,8 @@ int p2m_mem_paging_prep(struct domain *d
 /* Resume normal operation (in case a domain was paused) */
 void p2m_mem_paging_resume(struct domain *d);
 #else
-static inline void p2m_mem_paging_drop_page(struct domain *d, unsigned long gfn)
-{ }
+static inline int p2m_mem_paging_drop_page(struct domain *d, unsigned long gfn)
+{ return 0; }
 static inline void p2m_mem_paging_populate(struct domain *d, unsigned long gfn)
 { }
 #endif
xen/include/xen/mm.h +2 -0
diff -r 4c4a9ed0728c -r d1d238616c0c xen/include/xen/mm.h
@@ -318,6 +318,8 @@ page_list_splice(struct page_list_head *
 
 void scrub_one_page(struct page_info *);
 
+/* Returns 1 on success, 0 on error, negative if the ring
+ * for event propagation is full in the presence of paging */
 int guest_remove_page(struct domain *d, unsigned long gmfn);
 
 #define RAM_TYPE_CONVENTIONAL 0x00000001
xen/include/xen/sched.h +4 -1
diff -r 4c4a9ed0728c -r d1d238616c0c xen/include/xen/sched.h
@@ -183,13 +183,16 @@ struct mem_event_domain
 {
     /* ring lock */
     spinlock_t ring_lock;
-    unsigned int req_producers;
     /* shared page */
     mem_event_shared_page_t *shared_page;
     /* shared ring page */
     void *ring_page;
     /* front-end ring */
     mem_event_front_ring_t front_ring;
+    /* the number of vCPUs blocked */
+    unsigned int blocked;
+    /* The last vcpu woken up */
+    unsigned int last_vcpu_wake_up;
     /* event channel port (vcpu0 only) */
     int xen_port;
 };
#2 lore raw

Re: [PATCH 1 of 2] Allow decrease_reservation to be preempted if remove_page returns negative

In reply to Andres Lagar-Cavilla
Quoted 4 lines
Quoted 1 line
Quoted 1 line · Andres Lagar-Cavilla

This will allow for handling of full paging rings in a more graceful manner. ATM, remove_page does not return negative values, so this code is not yet exercised.

Which makes the patch on its own be of questionable value...

Quoted 18 lines

diff -r 790cd814bee8 -r 4c4a9ed0728c xen/common/memory.c

xen/common/memory.c +2 -0
@@ -226,6 +226,8 @@ static void decrease_reservation(struct 
 
     for ( i = a->nr_done; i < a->nr_extents; i++ )
     {
+        int one_remove_succeeded = 0;
+
         if ( hypercall_preempt_check() )
         {
             a->preempted = 1;
@@ -255,8 +257,33 @@ static void decrease_reservation(struct 
             continue;
 
         for ( j = 0; j < (1 << a->extent_order); j++ )

xen/common/memory.c static void decrease_reservation(struct · line 268

Quoted 10 lines
xen/common/memory.c +9 -1
-            if ( !guest_remove_page(a->domain, gmfn + j) )
+        {
+            int rv = guest_remove_page(a->domain, gmfn + j);
+            /* negative rv is a result of a mem event ring full
+             * in the presence of paging. We preempt the hypercall */
+            if ( rv < 0 )
+            {
+                /* Indicate we're done with this extent */
+                if ((j+1) == (1 << a->extent_order))
+                    i++;

So this implies that the page was actually acted upon? How that, if the event ring was full? (That's part of the reason why doing the change here without the change to guest_remove_page() is bogus - one can't really review it in one step.)

xen/common/memory.c static void decrease_reservation(struct · line 270

Quoted 2 lines
xen/common/memory.c +2 -0
+                a->preempted = 1;
+                raise_softirq(SCHEDULE_SOFTIRQ);

Do you really need to open code this *here* (rather than, if at all, in guest_remove_page() or wherever else the event ring full condition is actually being determined)?

xen/common/memory.c static void decrease_reservation(struct · line 275

Quoted 5 lines
xen/common/memory.c +4 -0
                 goto out;
+            }
+            /* rv of zero means we tried to remove a gfn with no backing
+             * mfn. It can be a bad argument, or, a continuation in the midst
+             * of an extent. Heuristically determine second case. */

Heuristically? What if this goes wrong (I can't really judge this from the code below)?

Jan

Quoted 19 lines
xen/common/memory.c +11 -0
+            if ( !rv )
+            {
+                /* Has to be first extent */
+                if ( i != a->nr_done )
+                    goto out;
+                /* No previous remove in this extent must have succeeded */
+                if ( one_remove_succeeded )
+                    goto out;
+            } else
+                one_remove_succeeded = 1;
+        }
     }
 
  out:

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#4 lore raw
In reply to Andres Lagar-Cavilla
Quoted 10 lines · Andres Lagar-Cavilla

Ensure no guest events are ever lost in the mem event ring.

This is one of two outstanding proposals to solve this issue. One key difference between them being that ours does not necessitate wait queues.

Instead, we rely on foreign domain retry (already in place), preempting hypercalls that may cause unbounded guest events (such as decrease_reservation), and ensuring there is always space left in the ring for each guest vcpu to place at least one event.

Thats not enough. Cases like hvm_copy and the emulator do currently no retry, instead they get an invalid mfn and crash the guest. Its possible to code around that in some places, like shown in the URL below, but wouldnt it make sense to just stop execution until the expected condition is met?
Its not clear to me how to properly handle a full ring in get_gfn_type_access() with your proposal.

Olaf

http://old-list-archives.xen.org/archives/html/xen-devel/2011-01/msg01121.html

#5 lore raw
Quoted 13 lines · Andres Lagar-Cavilla
Quoted 11 lines

Ensure no guest events are ever lost in the mem event ring.

This is one of two outstanding proposals to solve this issue. One key difference between them being that ours does not necessitate wait queues.

Instead, we rely on foreign domain retry (already in place), preempting hypercalls that may cause unbounded guest events (such as decrease_reservation), and ensuring there is always space left in the ring for each guest vcpu to place at least one event.

Quoted 5 lines

Thats not enough. Cases like hvm_copy and the emulator do currently no retry, instead they get an invalid mfn and crash the guest. Its possible to code around that in some places, like shown in the URL below, but wouldnt it make sense to just stop execution until the expected condition is met?

These things are completely unrelated. *Foreign* domains retry. With our patch, events caused by the guest itself are guaranteed to go in, no retry.

The fact that hvm_copy et al. currently crash the guest is independent of ring management. They crash the guest after placing the event in the ring. And that's where the wait queues are expected to save the day.

Quoted 2 lines

Its not clear to me how to properly handle a full ring in get_gfn_type_access() with your proposal.

If the request comes from a foreign domain, and there is no space in the ring, ENOENT goes upwards.

If the request comes from the guest itself, and it's p2m_query, no event needs to be generated.

If the request comes from the guest itself, and it requires paging_populate (!= p2m_query), the event is guaranteed to be put in the ring, and then the vcpu goes to sleep.

Easy-peasy
Andres

Quoted 5 lines

Olaf

http://old-list-archives.xen.org/archives/html/xen-devel/2011-01/msg01121.html

#6 lore raw
Quoted 5 lines · Andres Lagar-Cavilla

If the request comes from the guest itself, and it requires paging_populate (!= p2m_query), the event is guaranteed to be put in the ring, and then the vcpu goes to sleep.

Easy-peasy

If everything were so easy. ;-)

I will try to combine your patch with my paging waitqueue change. Perhaps your change should disallow paging if max_cpus is larger than RING_SIZE().

Olaf

#7 lore raw
Quoted 6 lines · Andres Lagar-Cavilla
Quoted 5 lines

If the request comes from the guest itself, and it requires paging_populate (!= p2m_query), the event is guaranteed to be put in the ring, and then the vcpu goes to sleep.

Quoted 7 lines
Quoted 1 line

Easy-peasy

If everything were so easy. ;-)

I will try to combine your patch with my paging waitqueue change. Perhaps your change should disallow paging if max_cpus is larger than RING_SIZE().

That sounds excellent. I can do those changes as well, if you don't want to be burdened by that.

I think it's wise though, to suspend forward-motion on either "branch" of ring management until we get a verdict. The fundamentals of each philosophy have been exposed. Further code has a 50% of being throw-away.

Thanks
Andres

Quoted 4 lines

Olaf