Mem event handling improvements
PATCH 1/1 Mem event handling improvements
Several improvements to the mem event interface are included in this series. Most notably:
- The ability to trigger resume actions in the hypervisor via an event channel kick, as opposed to a domctl. Less locking and more batching.
- Improvements to the management of the mem event ring. Improvements on return codes, vcpu pause semantics, and handling of corner cases.
xen/arch/x86/hvm/hvm.c | 21 +++- xen/arch/x86/mm/mem_event.c | 173 +++++++++++++++++++++++++++++---------- xen/arch/x86/mm/mem_sharing.c | 27 ++++- xen/arch/x86/mm/p2m.c | 100 ++++++++++++---------- xen/include/asm-x86/mem_event.h | 16 ++- xen/include/xen/sched.h | 5 +- xen/arch/ia64/vmx/vmx_init.c | 2 +- xen/arch/x86/hvm/hvm.c | 5 +- xen/arch/x86/mm/mem_event.c | 3 +- xen/common/event_channel.c | 75 +++++++++++++--- xen/include/xen/event.h | 5 +- xen/include/xen/sched.h | 2 +- xen/arch/x86/mm/mem_access.c | 3 +- xen/arch/x86/mm/p2m.c | 3 +- xen/include/asm-x86/p2m.h | 2 +- xen/arch/x86/mm/mem_event.c | 26 ++++- 16 files changed, 324 insertions(+), 144 deletions(-)
PATCH 1/1 Improve ring management for memory events
xen/arch/x86/hvm/hvm.c | 21 +++- xen/arch/x86/mm/mem_event.c | 173 +++++++++++++++++++++++++++++---------- xen/arch/x86/mm/mem_sharing.c | 27 ++++- xen/arch/x86/mm/p2m.c | 100 ++++++++++++---------- xen/include/asm-x86/mem_event.h | 16 ++- xen/include/xen/sched.h | 5 +- 6 files changed, 232 insertions(+), 110 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 this constraint will be violated by the current vCPU putting another request in the ring, we pause it after putting the current event.
If no foreign mappings cause mem events, then no mem events will be lost. Because events caused by foreign mapping are effectively unbound, we cannot guarantee with certainty lack of event loss.
Additionally, we ensure that no events are lost by Xen as a consumer if multiple responses land on the ring for a single domctl. While the current domctl-based notifications to Xen disallow batching, this is required for later patches.
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
@@ -4077,7 +4077,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 ;
@@ -4085,10 +4084,6 @@ static int hvm_memory_event_traps(long pif ( (p & HVMPME_onchangeonly) && (value == old) )return 1 ; - rc =mem_event_check_ring (d, &d->mem_access); -if ( rc ) -return rc; -memset (&req,0 ,sizeof (req)); req.type =MEM_EVENT_TYPE_ACCESS ; req.reason = reason;
@@ -4108,7 +4103,21 @@ static int hvm_memory_event_traps(long p req.gla_valid =1 ; } -mem_event_put_request (d, &d->mem_access, &req); +if (mem_event_put_request (d, &d->mem_access, &req) ) + { +/* 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 +* rc == -EBUSY +* If the ring is full, the vcpu has been marked and paused, +* so we also need to unpause it (if we had previously paused +* it). Once the consumer depletes the ring, the vcpu will be +* woken up and will retry. The consumer may have lost this +* exception, though. */ +if ( req.flags &MEM_EVENT_FLAG_VCPU_PAUSED ) +vcpu_unpause (v); + }return 1 ; }
xen/arch/x86/mm/mem_event.c +127 -46
231 lines not on this page · raw message
xen/arch/x86/mm/mem_sharing.c +19 -8
@@ -281,12 +281,19 @@ 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_share))return page; - req.gfn = gfn; req.p2mt = p2m_ram_shared; req.vcpu_id = v->vcpu_id; -mem_event_put_request (d, &d->mem_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_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; }
@@ -300,12 +307,16 @@ int mem_sharing_sharing_resume(struct do {mem_event_response_t rsp; -/* Get request off the ring */ -mem_event_get_response (&d->mem_share, &rsp); +/* Get all requests off the ring */ +while (mem_event_get_response (d, &d->mem_share, &rsp) ) + { +/* Unpause domain/vcpu */ +if ( rsp.flags &MEM_EVENT_FLAG_VCPU_PAUSED ) +vcpu_unpause (d->vcpu[rsp.vcpu_id]); + } -/* Unpause domain/vcpu */ -if ( rsp.flags &MEM_EVENT_FLAG_VCPU_PAUSED ) -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_paging);return 0 ; }
xen/arch/x86/mm/p2m.c +56 -44
180 lines not on this page · raw message
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); -void 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 domain *d,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/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; };
PATCH 1/1 Create a generic callback mechanism for Xen-bound event channels
In reply to Andres Lagar-Cavillaxen/arch/ia64/vmx/vmx_init.c | 2 +- xen/arch/x86/hvm/hvm.c | 5 +- xen/arch/x86/mm/mem_event.c | 3 +- xen/common/event_channel.c | 75 ++++++++++++++++++++++++++++++++++--------- xen/include/xen/event.h | 5 ++- xen/include/xen/sched.h | 2 +- 6 files changed, 69 insertions(+), 23 deletions(-)
For event channels for which Xen is the consumer, there currently is a single action. With this patch, we allow event channel creators to specify a generic callback (or no callback). Because the expectation is that there will be few callbacks, they are stored in a small table.
xen/arch/ia64/vmx/vmx_init.c +1 -1
@@ -377,7 +377,7 @@ vmx_vcpu_initialise(struct vcpu *v) {struct vmx_ioreq_page *iorp = &v->domain->arch.hvm_domain.ioreq; -int rc =alloc_unbound_xen_event_channel (v,0 ); +int rc =alloc_unbound_xen_event_channel (v,0 ,NULL );if (rc <0 )return rc; v->arch.arch_vmx.xen_port = rc;
xen/arch/x86/hvm/hvm.c +3 -2
@@ -979,7 +979,7 @@ int hvm_vcpu_initialise(struct vcpu *v) goto fail3;/* Create ioreq event channel. */ - rc =alloc_unbound_xen_event_channel (v,0 ); + rc =alloc_unbound_xen_event_channel (v,0 ,NULL );if ( rc <0 ) goto fail4;
@@ -3531,7 +3531,8 @@ long do_hvm_op(unsigned long op, XEN_GUE for_each_vcpu ( d, v ) {int old_port, new_port; - new_port =alloc_unbound_xen_event_channel (v, a.value); + new_port =alloc_unbound_xen_event_channel ( + v, a.value,NULL );if ( new_port <0 ) { rc = new_port;
xen/arch/x86/mm/mem_event.c +2 -1
@@ -96,7 +96,8 @@ static int mem_event_enable(struct domai/* Allocate event channel */ rc =alloc_unbound_xen_event_channel (d->vcpu[0 ], - current->domain->domain_id); + current->domain->domain_id, +NULL );if ( rc <0 ) goto err;
xen/common/event_channel.c +58 -17
141 lines not on this page · raw message
xen/include/xen/event.h +4 -1
@@ -51,8 +51,11 @@ int evtchn_unmask(unsigned int port);void evtchn_move_pirqs (struct vcpu *v);/* Allocate/free a Xen-attached event channel port. */ +typedef void (*xen_event_channel_notification_t )( +struct vcpu *v,unsigned int port);int alloc_unbound_xen_event_channel ( -struct vcpu *local_vcpu,domid_t remote_domid); +struct vcpu *local_vcpu,domid_t remote_domid, +xen_event_channel_notification_t notification_fn);void free_xen_event_channel (struct vcpu *local_vcpu,int port);
xen/include/xen/sched.h +1 -1
@@ -47,7 +47,7 @@ struct evtchn#define ECS_VIRQ 5/* Channel is bound to a virtual IRQ line. */ #define ECS_IPI 6/* Channel is bound to a virtual IPI line. */ u8 state;/* ECS_* */ -u8 consumer_is_xen;/* Consumed by Xen or by guest? */ +u8 xen_consumer;/* Consumer in Xen, if any? (0 = send to guest) */ u16 notify_vcpu_id;/* VCPU for local delivery notification */ union { struct {
PATCH 1/1 Make the prototype of p2m_mem_access_resume consistent
In reply to Andres Lagar-Cavillaxen/arch/x86/mm/mem_access.c | 3 +-- xen/arch/x86/mm/p2m.c | 3 +-- xen/include/asm-x86/p2m.h | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-)
xen/arch/x86/mm/mem_access.c +1 -2
@@ -29,13 +29,12 @@ int mem_access_domctl(struct domain *d, XEN_GUEST_HANDLE(void) u_domctl) {int rc; -struct p2m_domain *p2m =p2m_get_hostp2m (d);switch ( mec->op ) {case XEN_DOMCTL_MEM_EVENT_OP_ACCESS_RESUME : { -p2m_mem_access_resume (p2m); +p2m_mem_access_resume (d); rc =0 ; }break ;
xen/arch/x86/mm/p2m.c +1 -2
@@ -1150,9 +1150,8 @@ void p2m_mem_access_check(unsigned long/* VCPU paused */ } -void p2m_mem_access_resume (struct p2m_domain *p2m) +void p2m_mem_access_resume (struct domain *d) { -struct domain *d = p2m->domain;mem_event_response_t rsp;/* Pull all responses off the ring */
xen/include/asm-x86/p2m.h +1 -1
@@ -493,7 +493,7 @@ static inline void p2m_mem_paging_populavoid p2m_mem_access_check (unsigned long gpa,bool_t gla_valid,unsigned long gla,bool_t access_r,bool_t access_w,bool_t access_x);/* Resumes the running of the VCPU, restarting the last instruction */ -void p2m_mem_access_resume (struct p2m_domain *p2m); +void p2m_mem_access_resume (struct domain *d);/* Set access type for a region of pfns. * If start_pfn == -1ul, sets the default access type */
PATCH 1/1 Allow memevent responses to be signaled via the event channel
In reply to Andres Lagar-Cavillaxen/arch/x86/mm/mem_event.c | 26 ++++++++++++++++++++------ 1 files changed, 20 insertions(+), 6 deletions(-)
Don't require a separate domctl to notify the memevent interface that an event has occured. This domctl can be taxing, particularly when you are scaling events and paging to many domains across a single system. Instead, we use the existing event channel to signal when we place something in the ring (as per normal ring operation).
xen/arch/x86/mm/mem_event.c +20 -6
@@ -37,9 +37,11 @@#define mem_event_ring_lock (_med) spin_lock(&(_med)->ring_lock)#define mem_event_ring_unlock (_med) spin_unlock(&(_med)->ring_lock) -static int mem_event_enable (struct domain *d, -xen_domctl_mem_event_op_t *mec, -struct mem_event_domain *med) +static int mem_event_enable ( +struct domain *d, +xen_domctl_mem_event_op_t *mec, +struct mem_event_domain *med, +xen_event_channel_notification_t notification_fn) {int rc;struct domain *dom_mem_event = current->domain;
@@ -97,7 +99,7 @@ static int mem_event_enable(struct domai/* Allocate event channel */ rc =alloc_unbound_xen_event_channel (d->vcpu[0 ], current->domain->domain_id, -NULL ); + notification_fn);if ( rc <0 ) goto err;
@@ -308,6 +310,18 @@ int mem_event_check_ring(struct domain *return 0 ; } +/* Registered with Xen-bound event channel for incoming notifications. */ +static void mem_paging_notification (struct vcpu *v,unsigned int port) +{ +p2m_mem_paging_resume (v->domain); +} + +/* Registered with Xen-bound event channel for incoming notifications. */ +static void mem_access_notification (struct vcpu *v,unsigned int port) +{ +p2m_mem_access_resume (v->domain); +} +int mem_event_domctl (struct domain *d,xen_domctl_mem_event_op_t *mec,XEN_GUEST_HANDLE (void ) u_domctl) {
@@ -369,7 +383,7 @@ int mem_event_domctl(struct domain *d, xif ( p2m->pod.entry_count )break ; - rc =mem_event_enable (d, mec, med); + rc =mem_event_enable (d, mec, med, mem_paging_notification); }break ;
@@ -408,7 +422,7 @@ int mem_event_domctl(struct domain *d, xif ( boot_cpu_data.x86_vendor !=X86_VENDOR_INTEL )break ; - rc =mem_event_enable (d, mec, med); + rc =mem_event_enable (d, mec, med, mem_access_notification); }break ;
Re: [PATCH 1 of 4] Improve ring management for memory events
In reply to Andres Lagar-CavillaOn Mon, Nov 14, Andres Lagar-Cavilla wrote:
I will do a more complete review later, a few comments below.
xen/arch/x86/mm/mem_event.c static int mem_event_disable(struct mem_ · line 148
Quoted 6 lines
xen/arch/x86/mm/mem_event.c +6
-0+ free_requests =RING_FREE_REQUESTS (&med->front_ring); +if (unlikely (free_requests < d->max_vcpus) ) + { +/* This may happen during normal operation (hopefully not often). */ +gdprintk (XENLOG_INFO ,"mem_event request slots for domain %d: %d\n" , + d->domain_id, free_requests);
What is the purpose of this printk? The ring can and does fillup quickly with paging.
xen/arch/x86/mm/mem_event.c static int mem_event_disable(struct mem_ · line 176
Quoted 5 lines
xen/arch/x86/mm/mem_event.c +5
-0+if (mem_event_ring_free (d, med) ==0 ) + { +/* This *may* happen, but only when foreign mappings generate events. */ +gdprintk (XENLOG_WARNING ,"possible lost mem_event for domain %d\n" , + d->domain_id);
This printk is not needed, callers have to try again.
Olaf
Re: [PATCH 1 of 4] Improve ring management for memory events
Olaf,
Quoted 14 lines · Andres Lagar-Cavilla
I will do a more complete review later, a few comments below.
Quoted 8 lines
xen/arch/x86/mm/mem_event.c +4
-0+ free_requests =RING_FREE_REQUESTS (&med->front_ring); +if (unlikely (free_requests < d->max_vcpus) ) + { +/* This may happen during normal operation (hopefully not often). */
+ gdprintk(XENLOG_INFO, "mem_eventrequest slots for domain %d:
%d\n",
+d->domain_id,free_requests);
Quoted 2 lines
What is the purpose of this printk? The ring can and does fillup quickly with paging.
Well, we can tone down printk's to be debug level. I don't think they're unnecessary if they're made an optional debug tool.
Question: I have one vcpu, how do I fill up the ring quickly? (outside of foreign mappings)
Andres
Quoted 14 lines
Quoted 9 lines
xen/arch/x86/mm/mem_event.c +3
-0+if (mem_event_ring_free (d, med) ==0 ) + { +/* This *may* happen, but only when foreign mappings generate events. */
+ gdprintk(XENLOG_WARNING, "possible lostmem_eventfor domain
%d\n",
+d->domain_id);This printk is not needed, callers have to try again.
Olaf
Re: [PATCH 1 of 4] Improve ring management for memory events
Quoted 2 lines · Andres Lagar-Cavilla
Well, we can tone down printk's to be debug level. I don't think they're unnecessary if they're made an optional debug tool.
There is nothing to debug here, since the callers have to retry anyway.
Quoted 2 lines
Question: I have one vcpu, how do I fill up the ring quickly? (outside of foreign mappings)
Have a balloon driver in the guest and balloon down more than 64*PAGE_SIZE. This is the default at least in my setup where the kernel driver releases some memory right away (I havent checked where this is actually configured).
Olaf
Re: [PATCH 1 of 4] Improve ring management for memory events
Quoted 7 lines · Andres Lagar-Cavilla
Quoted 3 lines
Well, we can tone down printk's to be debug level. I don't think they're unnecessary if they're made an optional debug tool.
There is nothing to debug here, since the callers have to retry anyway.
Quoted 8 lines
Quoted 3 lines
Question: I have one vcpu, how do I fill up the ring quickly? (outside of
foreign mappings)Have a balloon driver in the guest and balloon down more than 64*PAGE_SIZE. This is the default at least in my setup where the kernel driver releases some memory right away (I havent checked where this is actually configured).
I see, a guest can call decrease_reservation with an extent_order large enough that it will overflow the ring. No matter the size of the ring. Isn't preemption of this hypercall a better tactic than putting the vcpu on a wait-queue? This won't preclude the need for wait queues, but it feels like a much cleaner solution.
With retrying of foreign mappings in xc_map_foreign_bulk (and grants), I wonder if we should put events in the ring due to foreign mappings *at all*, in the case of congestion. Eventually a retry will get to kick the pager.
Andres
Quoted 3 lines
Olaf
Re: [PATCH 1 of 4] Improve ring management for memory events
Quoted 16 lines · Andres Lagar-Cavilla
Quoted 7 lines · Andres Lagar-Cavilla
Quoted 3 lines
Well, we can tone down printk's to be debug level. I don't think they're unnecessary if they're made an optional debug tool.
There is nothing to debug here, since the callers have to retry anyway.
Quoted 8 lines
Quoted 3 lines
Question: I have one vcpu, how do I fill up the ring quickly? (outside of
foreign mappings)Have a balloon driver in the guest and balloon down more than 64*PAGE_SIZE. This is the default at least in my setup where the kernel driver releases some memory right away (I havent checked where this is actually configured).
Quoted 5 lines
I see, a guest can call
decrease_reservationwith anextent_orderlarge enough that it will overflow the ring. No matter the size of the ring. Isn't preemption of this hypercall a better tactic than putting the vcpu on a wait-queue? This won't preclude the need for wait queues, but it feels like a much cleaner solution.
Yes, yesterday I was thinking about this as well. p2m_mem_paging_drop_page() should return -EBUSY. But currently not all callers of guest_remove_page() look at the exit code. Perhaps that can be fixed.
Quoted 4 lines
With retrying of foreign mappings in
xc_map_foreign_bulk(and grants), I wonder if we should put events in the ring due to foreign mappings *at all*, in the case of congestion. Eventually a retry will get to kick the pager.
What do you mean with that?
Olaf
Re: [PATCH 1 of 4] Improve ring management for memory events
Quoted 33 lines · Andres Lagar-Cavilla
Quoted 18 lines
Quoted 4 lines · Andres Lagar-Cavilla
Quoted 2 lines
Well, we can tone down printk's to be debug level. I don't think
they're
Quoted 3 lines
Quoted 1 line
unnecessary if they're made an optional debug tool.
There is nothing to debug here, since the callers have to retry
anyway.
Quoted 2 lines
Quoted 2 lines
Question: I have one vcpu, how do I fill up the ring quickly?
(outside
Quoted 5 lines
Quoted 2 lines
of
foreign mappings)Have a balloon driver in the guest and balloon down more than 64*PAGE_SIZE. This is the default at least in my setup where the
kernel
Quoted 8 lines
Quoted 2 lines
driver releases some memory right away (I havent checked where this is actually configured).
I see, a guest can call
decrease_reservationwith anextent_orderlarge enough that it will overflow the ring. No matter the size of the ring. Isn't preemption of this hypercall a better tactic than putting the vcpu on a wait-queue? This won't preclude the need for wait queues, but it feels like a much cleaner solution.Yes, yesterday I was thinking about this as well.
p2m_mem_paging_drop_page()should return-EBUSY. But currently not all callers ofguest_remove_page()look at the exit code. Perhaps that can be fixed.
Quoted 6 lines
Quoted 4 lines
With retrying of foreign mappings in
xc_map_foreign_bulk(and grants), I wonder if we should put events in the ring due to foreign mappings *at all*, in the case of congestion. Eventually a retry will get to kick the pager.What do you mean with that?
Thinking out loud here. In our ring management patch, we put a guest vcpu to sleep if space in the ring < d->max_vcpus.
In the case of a foreign mapping vcpu, we still allow the vcpu to put an event in the ring, as long as there is any space. This can eventually fill up the ring and prevent guest vcpus from placing events.
However, we could prevent this latter behaviour if it will result in a condition in whichspace_in_the_ring < (d->max_vcpus - ring->blocked)
This will ensure no event caused by a guest vcpu will ever be lost (we still need the preemption of decrease_reservation, though).
Correctness is preserved for the foreign mapping vcpu. It will retry its mapping and eventually there will be space in the ring.
With this, we won't need wait-queues for ring management.
Makes sense?
Andres
Quoted 3 lines
Olaf