Mem event ring management overhaul
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(-)
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
@@ -226,6 +226,8 @@ static void decrease_reservation(structfor ( 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(structcontinue ;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:
PATCH 1/1 Improve ring management for memory events. Do not lose guest events
In reply to Andres Lagar-Cavillaxen/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
@@ -4119,7 +4119,6 @@ static int hvm_memory_event_traps(long pstruct 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 pif ( (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
@@ -281,12 +281,20 @@ static struct page_info* mem_sharing_allvcpu_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 dovcpu_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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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; };
Re: [PATCH 1 of 2] Allow decrease_reservation to be preempted if remove_page returns negative
In reply to Andres Lagar-CavillaQuoted 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_pagedoes 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
Signed-off-by Andres Lagar-Cavilla <[email protected]>diff -r
790cd814bee8-r4c4a9ed0728cxen/common/memory.cxen/common/memory.c +2
-0@@ -226,6 +226,8 @@ static void decrease_reservation(structfor ( 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(structcontinue ;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
-0goto 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
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
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_copyand 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 · 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
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_cpusis larger thanRING_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