[PATCHv4 0/4] evtchn: Improve scalebility

[PATCHv4 0/4] evtchn: Improve scalebility

[David Vrabel]

The per-domain event channel lock limits scalability when many VCPUs
are trying to send interdomain events.  A per-channel lock is
introduced eliminating any lock contention when sending an event.

See this graph for the performance improvements:

  http://xenbits.xen.org/people/dvrabel/evtchn-scalability.png

A different test (using Linux's evtchn device which masks/unmasks
event channels) showed the following lock profile improvements:

Per-domain lock:
(XEN)   lock:    69267976(00000004:19830041), block:    27777407(00000002:3C7C5C96)

Per-event channel lock 
(XEN)   lock:      686530(00000000:076AF5F6), block:        1787(00000000:000B4D22)

Locking removed from evtchn_unmask():
(XEN)   lock:       10769(00000000:00512999), block:          99(00000000:00009491)

v4:
- consumer_is_xen() check in __evtchn_close() is only valid when guest
  requested the close.
- Re-add BUG_ON(!port_is_valid()) to free_xen_event_channel().

v3:
- Clear xen_consumer when clearing state.
- Defer freeing struct evtchn's until evtchn_destroy_final().
- Remove redundant d->evtchn test from port_is_valid().
- Use port_is_valid() again.
- Drop event lock from notify_via_xen_event_channel().

v2:
- Use unsigned int for d->valid_evtchns.
- Compare channel pointers in double_evtchn_lock().

David

[PATCHv4 1/4] evtchn: clear xen_consumer when clearing state

[David Vrabel]

Freeing a xen event channel would clear xen_consumer before clearing
the channel state, leaving a window where the channel is in a funny
state (still bound but no consumer).

Move the clear of xen_consumer into free_evtchn() where the state is
also cleared.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
v4:
- consumer_is_xen() check is only valid when guest requested the close.
---
 xen/common/event_channel.c |   17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 2208de0..26870b6 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -202,6 +202,7 @@ static void free_evtchn(struct domain *d, struct evtchn *chn)
     /* Reset binding to vcpu0 when the channel is freed. */
     chn->state          = ECS_FREE;
     chn->notify_vcpu_id = 0;
+    chn->xen_consumer   = 0;
 
     xsm_evtchn_close_post(chn);
 }
@@ -468,7 +469,7 @@ static long evtchn_bind_pirq(evtchn_bind_pirq_t *bind)
 }
 
 
-static long __evtchn_close(struct domain *d1, int port1)
+static long __evtchn_close(struct domain *d1, int port1, bool_t guest)
 {
     struct domain *d2 = NULL;
     struct vcpu   *v;
@@ -488,7 +489,7 @@ static long __evtchn_close(struct domain *d1, int port1)
     chn1 = evtchn_from_port(d1, port1);
 
     /* Guest cannot close a Xen-attached event channel. */
-    if ( unlikely(consumer_is_xen(chn1)) )
+    if ( unlikely(guest && consumer_is_xen(chn1)) )
     {
         rc = -EINVAL;
         goto out;
@@ -600,7 +601,7 @@ static long __evtchn_close(struct domain *d1, int port1)
 
 static long evtchn_close(evtchn_close_t *close)
 {
-    return __evtchn_close(current->domain, close->port);
+    return __evtchn_close(current->domain, close->port, 1);
 }
 
 int evtchn_send(struct domain *ld, unsigned int lport)
@@ -952,7 +953,7 @@ static long evtchn_reset(evtchn_reset_t *r)
         goto out;
 
     for ( i = 0; port_is_valid(d, i); i++ )
-        (void)__evtchn_close(d, i);
+        (void)__evtchn_close(d, i, 1);
 
     spin_lock(&d->event_lock);
 
@@ -1185,11 +1186,10 @@ void free_xen_event_channel(struct domain *d, int port)
     BUG_ON(!port_is_valid(d, port));
     chn = evtchn_from_port(d, port);
     BUG_ON(!consumer_is_xen(chn));
-    chn->xen_consumer = 0;
 
     spin_unlock(&d->event_lock);
 
-    (void)__evtchn_close(d, port);
+    (void)__evtchn_close(d, port, 0);
 }
 
 
@@ -1286,10 +1286,7 @@ void evtchn_destroy(struct domain *d)
 
     /* Close all existing event channels. */
     for ( i = 0; port_is_valid(d, i); i++ )
-    {
-        evtchn_from_port(d, i)->xen_consumer = 0;
-        (void)__evtchn_close(d, i);
-    }
+        (void)__evtchn_close(d, i, 0);
 
     /* Free all event-channel buckets. */
     spin_lock(&d->event_lock);
-- 
1.7.10.4

[PATCHv4 2/4] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()

[David Vrabel]

notify_via_xen_event_channel() and free_xen_event_channel() had to
check if the domain was dying because they may be called while the
domain is being destroyed and the struct evtchn's are being freed.

By deferring the freeing of the struct evtchn's until all references
to the domain are dropped, these functions can rely on the channel
state being present and valid.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
v4:
- Re-add BUG_ON(!port_is_valid()).
---
 xen/common/event_channel.c |   46 ++++++++++++--------------------------------
 1 file changed, 12 insertions(+), 34 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 26870b6..4ee65e6 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -1173,21 +1173,7 @@ int alloc_unbound_xen_event_channel(
 
 void free_xen_event_channel(struct domain *d, int port)
 {
-    struct evtchn *chn;
-
-    spin_lock(&d->event_lock);
-
-    if ( unlikely(d->is_dying) )
-    {
-        spin_unlock(&d->event_lock);
-        return;
-    }
-
     BUG_ON(!port_is_valid(d, port));
-    chn = evtchn_from_port(d, port);
-    BUG_ON(!consumer_is_xen(chn));
-
-    spin_unlock(&d->event_lock);
 
     (void)__evtchn_close(d, port, 0);
 }
@@ -1200,18 +1186,12 @@ void notify_via_xen_event_channel(struct domain *ld, int lport)
 
     spin_lock(&ld->event_lock);
 
-    if ( unlikely(ld->is_dying) )
-    {
-        spin_unlock(&ld->event_lock);
-        return;
-    }
-
     ASSERT(port_is_valid(ld, lport));
     lchn = evtchn_from_port(ld, lport);
-    ASSERT(consumer_is_xen(lchn));
 
     if ( likely(lchn->state == ECS_INTERDOMAIN) )
     {
+        ASSERT(consumer_is_xen(lchn));
         rd    = lchn->u.interdomain.remote_dom;
         rchn  = evtchn_from_port(rd, lchn->u.interdomain.remote_port);
         evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
@@ -1278,7 +1258,7 @@ int evtchn_init(struct domain *d)
 
 void evtchn_destroy(struct domain *d)
 {
-    unsigned int i, j;
+    unsigned int i;
 
     /* After this barrier no new event-channel allocations can occur. */
     BUG_ON(!d->is_dying);
@@ -1288,8 +1268,17 @@ void evtchn_destroy(struct domain *d)
     for ( i = 0; port_is_valid(d, i); i++ )
         (void)__evtchn_close(d, i, 0);
 
+    clear_global_virq_handlers(d);
+
+    evtchn_fifo_destroy(d);
+}
+
+
+void evtchn_destroy_final(struct domain *d)
+{
+    unsigned int i, j;
+
     /* Free all event-channel buckets. */
-    spin_lock(&d->event_lock);
     for ( i = 0; i < NR_EVTCHN_GROUPS; i++ )
     {
         if ( !d->evtchn_group[i] )
@@ -1297,20 +1286,9 @@ void evtchn_destroy(struct domain *d)
         for ( j = 0; j < BUCKETS_PER_GROUP; j++ )
             free_evtchn_bucket(d, d->evtchn_group[i][j]);
         xfree(d->evtchn_group[i]);
-        d->evtchn_group[i] = NULL;
     }
     free_evtchn_bucket(d, d->evtchn);
-    d->evtchn = NULL;
-    spin_unlock(&d->event_lock);
-
-    clear_global_virq_handlers(d);
 
-    evtchn_fifo_destroy(d);
-}
-
-
-void evtchn_destroy_final(struct domain *d)
-{
 #if MAX_VIRT_CPUS > BITS_PER_LONG
     xfree(d->poll_mask);
     d->poll_mask = NULL;
-- 
1.7.10.4

[PATCHv4 3/4] evtchn: use a per-event channel lock for sending events

[David Vrabel]

When sending an event, use a new per-event channel lock to safely
validate the event channel state.

This new lock must be held when changing event channel state.  Note
that the event channel lock must also be held when changing state from
ECS_FREE or it will race with a concurrent get_free_port() call.

To avoid having to take the remote event channel locks when sending to
an interdomain event channel, the local and remote channel locks are
both held when binding or closing an interdomain event channel.

This significantly  increases the  number of events  that can  be sent
from multiple  VCPUs.  But struct  evtchn increases in  size, reducing
the number that fit into a single page to 64 (instead of 128).

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
v3:
- Use port_is_valid in evtchn_send().
- Drop event lock from notify_via_xen_event_channel().

v2:
- Compare channel pointers in double_evtchn_lock().
---
 xen/common/event_channel.c |   81 +++++++++++++++++++++++++++++++++++++-------
 xen/include/xen/sched.h    |    1 +
 2 files changed, 69 insertions(+), 13 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 4ee65e6..193e565 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -139,6 +139,7 @@ static struct evtchn *alloc_evtchn_bucket(struct domain *d, unsigned int port)
             return NULL;
         }
         chn[i].port = port + i;
+        spin_lock_init(&chn[i].lock);
     }
     return chn;
 }
@@ -229,11 +230,15 @@ static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
     if ( rc )
         goto out;
 
+    spin_lock(&chn->lock);
+
     chn->state = ECS_UNBOUND;
     if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF )
         chn->u.unbound.remote_domid = current->domain->domain_id;
     evtchn_port_init(d, chn);
 
+    spin_unlock(&chn->lock);
+
     alloc->port = port;
 
  out:
@@ -244,6 +249,28 @@ static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
 }
 
 
+static void double_evtchn_lock(struct evtchn *lchn, struct evtchn *rchn)
+{
+    if ( lchn < rchn )
+    {
+        spin_lock(&lchn->lock);
+        spin_lock(&rchn->lock);
+    }
+    else
+    {
+        if ( lchn != rchn )
+            spin_lock(&rchn->lock);
+        spin_lock(&lchn->lock);
+    }
+}
+
+static void double_evtchn_unlock(struct evtchn *lchn, struct evtchn *rchn)
+{
+    spin_unlock(&lchn->lock);
+    if ( lchn != rchn )
+        spin_unlock(&rchn->lock);
+}
+
 static long evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
 {
     struct evtchn *lchn, *rchn;
@@ -286,6 +313,8 @@ static long evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
     if ( rc )
         goto out;
 
+    double_evtchn_lock(lchn, rchn);
+
     lchn->u.interdomain.remote_dom  = rd;
     lchn->u.interdomain.remote_port = rport;
     lchn->state                     = ECS_INTERDOMAIN;
@@ -301,6 +330,8 @@ static long evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
      */
     evtchn_port_set_pending(ld, lchn->notify_vcpu_id, lchn);
 
+    double_evtchn_unlock(lchn, rchn);
+
     bind->local_port = lport;
 
  out:
@@ -341,11 +372,16 @@ static long evtchn_bind_virq(evtchn_bind_virq_t *bind)
         ERROR_EXIT(port);
 
     chn = evtchn_from_port(d, port);
+
+    spin_lock(&chn->lock);
+
     chn->state          = ECS_VIRQ;
     chn->notify_vcpu_id = vcpu;
     chn->u.virq         = virq;
     evtchn_port_init(d, chn);
 
+    spin_unlock(&chn->lock);
+
     v->virq_to_evtchn[virq] = bind->port = port;
 
  out:
@@ -372,10 +408,15 @@ static long evtchn_bind_ipi(evtchn_bind_ipi_t *bind)
         ERROR_EXIT(port);
 
     chn = evtchn_from_port(d, port);
+
+    spin_lock(&chn->lock);
+
     chn->state          = ECS_IPI;
     chn->notify_vcpu_id = vcpu;
     evtchn_port_init(d, chn);
 
+    spin_unlock(&chn->lock);
+
     bind->port = port;
 
  out:
@@ -450,11 +491,15 @@ static long evtchn_bind_pirq(evtchn_bind_pirq_t *bind)
         goto out;
     }
 
+    spin_lock(&chn->lock);
+
     chn->state  = ECS_PIRQ;
     chn->u.pirq.irq = pirq;
     link_pirq_port(port, chn, v);
     evtchn_port_init(d, chn);
 
+    spin_unlock(&chn->lock);
+
     bind->port = port;
 
 #ifdef CONFIG_X86
@@ -575,15 +620,24 @@ static long __evtchn_close(struct domain *d1, int port1, bool_t guest)
         BUG_ON(chn2->state != ECS_INTERDOMAIN);
         BUG_ON(chn2->u.interdomain.remote_dom != d1);
 
+        double_evtchn_lock(chn1, chn2);
+
+        free_evtchn(d1, chn1);
+
         chn2->state = ECS_UNBOUND;
         chn2->u.unbound.remote_domid = d1->domain_id;
-        break;
+
+        double_evtchn_unlock(chn1, chn2);
+
+        goto out;
 
     default:
         BUG();
     }
 
+    spin_lock(&chn1->lock);
     free_evtchn(d1, chn1);
+    spin_unlock(&chn1->lock);
 
  out:
     if ( d2 != NULL )
@@ -610,21 +664,18 @@ int evtchn_send(struct domain *ld, unsigned int lport)
     struct domain *rd;
     int            rport, ret = 0;
 
-    spin_lock(&ld->event_lock);
-
-    if ( unlikely(!port_is_valid(ld, lport)) )
-    {
-        spin_unlock(&ld->event_lock);
+    if ( !port_is_valid(ld, lport) )
         return -EINVAL;
-    }
 
     lchn = evtchn_from_port(ld, lport);
 
+    spin_lock(&lchn->lock);
+
     /* Guest cannot send via a Xen-attached event channel. */
     if ( unlikely(consumer_is_xen(lchn)) )
     {
-        spin_unlock(&ld->event_lock);
-        return -EINVAL;
+        ret = -EINVAL;
+        goto out;
     }
 
     ret = xsm_evtchn_send(XSM_HOOK, ld, lchn);
@@ -653,7 +704,7 @@ int evtchn_send(struct domain *ld, unsigned int lport)
     }
 
 out:
-    spin_unlock(&ld->event_lock);
+    spin_unlock(&lchn->lock);
 
     return ret;
 }
@@ -1160,11 +1211,15 @@ int alloc_unbound_xen_event_channel(
     if ( rc )
         goto out;
 
+    spin_lock(&chn->lock);
+
     chn->state = ECS_UNBOUND;
     chn->xen_consumer = get_xen_consumer(notification_fn);
     chn->notify_vcpu_id = lvcpu;
     chn->u.unbound.remote_domid = remote_domid;
 
+    spin_unlock(&chn->lock);
+
  out:
     spin_unlock(&ld->event_lock);
 
@@ -1184,11 +1239,11 @@ void notify_via_xen_event_channel(struct domain *ld, int lport)
     struct evtchn *lchn, *rchn;
     struct domain *rd;
 
-    spin_lock(&ld->event_lock);
-
     ASSERT(port_is_valid(ld, lport));
     lchn = evtchn_from_port(ld, lport);
 
+    spin_lock(&lchn->lock);
+
     if ( likely(lchn->state == ECS_INTERDOMAIN) )
     {
         ASSERT(consumer_is_xen(lchn));
@@ -1197,7 +1252,7 @@ void notify_via_xen_event_channel(struct domain *ld, int lport)
         evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
     }
 
-    spin_unlock(&ld->event_lock);
+    spin_unlock(&lchn->lock);
 }
 
 void evtchn_check_pollers(struct domain *d, unsigned int port)
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 604d047..44ea92d 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -79,6 +79,7 @@ extern domid_t hardware_domid;
 
 struct evtchn
 {
+    spinlock_t lock;
 #define ECS_FREE         0 /* Channel is available for use.                  */
 #define ECS_RESERVED     1 /* Channel is reserved.                           */
 #define ECS_UNBOUND      2 /* Channel is waiting to bind to a remote domain. */
-- 
1.7.10.4

[PATCHv4 4/4] evtchn: pad struct evtchn to 64 bytes

[David Vrabel]

The number of struct evtchn in a page must be a power of two.  Under
some workloads performance is improved slightly by padding struct
evtchn to 64 bytes (a typical cache line size), thus putting the fewer
per-channel locks into each cache line.

This does not decrease the number of struct evtchn's per-page.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
v2:
- Use __attribute__((aligned(64))) for the padding.
---
 xen/include/xen/sched.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 44ea92d..a0ff9d2 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -129,7 +129,7 @@ struct evtchn
 #endif
     } ssid;
 #endif
-};
+} __attribute__((aligned(64)));
 
 int  evtchn_init(struct domain *d); /* from domain_create */
 void evtchn_destroy(struct domain *d); /* from domain_kill */
-- 
1.7.10.4