[PATCHv2 0/5] evtchn: Improve scalebility

[PATCHv2 0/5] 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)

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

David

[PATCHv2 1/5] evtchn: factor out freeing an event channel

[David Vrabel]

We're going to want to free an event channel from two places.  Factor out
the code into a free_evtchn() function.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/common/event_channel.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index bf9b2f8..947880f 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -192,6 +192,17 @@ static int get_free_port(struct domain *d)
     return port;
 }
 
+static void free_evtchn(struct domain *d, struct evtchn *chn)
+{
+    /* Clear pending event to avoid unexpected behavior on re-bind. */
+    evtchn_port_clear_pending(d, chn);
+
+    /* Reset binding to vcpu0 when the channel is freed. */
+    chn->state          = ECS_FREE;
+    chn->notify_vcpu_id = 0;
+
+    xsm_evtchn_close_post(chn);
+}
 
 static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
 {
@@ -569,14 +580,7 @@ static long __evtchn_close(struct domain *d1, int port1)
         BUG();
     }
 
-    /* Clear pending event to avoid unexpected behavior on re-bind. */
-    evtchn_port_clear_pending(d1, chn1);
-
-    /* Reset binding to vcpu0 when the channel is freed. */
-    chn1->state          = ECS_FREE;
-    chn1->notify_vcpu_id = 0;
-
-    xsm_evtchn_close_post(chn1);
+    free_evtchn(d1, chn1);
 
  out:
     if ( d2 != NULL )
-- 
1.7.10.4

[PATCHv2 2/5] evtchn: simplify port_is_valid()

[David Vrabel]

By keeping a count of the number of currently valid event channels,
port_is_valid() can be simplified.

d->valid_evtchns can also be tested without holding d->event_lock which
will be useful later on.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
v2:
- Used unsigned int for d->valid_evtchns.
---
 xen/common/event_channel.c |    3 +++
 xen/include/xen/event.h    |    4 +---
 xen/include/xen/sched.h    |    5 +++--
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 947880f..fd48646 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -189,6 +189,8 @@ static int get_free_port(struct domain *d)
         return -ENOMEM;
     bucket_from_port(d, port) = chn;
 
+    write_atomic(&d->valid_evtchns, d->valid_evtchns + EVTCHNS_PER_BUCKET);
+
     return port;
 }
 
@@ -1254,6 +1256,7 @@ int evtchn_init(struct domain *d)
     d->evtchn = alloc_evtchn_bucket(d, 0);
     if ( !d->evtchn )
         return -ENOMEM;
+    d->valid_evtchns = EVTCHNS_PER_BUCKET;
 
     spin_lock_init_prof(d, event_lock);
     if ( get_free_port(d) != 0 )
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index 690f865..a08ecdb 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -91,9 +91,7 @@ static inline bool_t port_is_valid(struct domain *d, unsigned int p)
         return 0;
     if ( !d->evtchn )
         return 0;
-    if ( p < EVTCHNS_PER_BUCKET )
-        return 1;
-    return group_from_port(d, p) != NULL && bucket_from_port(d, p) != NULL;
+    return p < read_atomic(&d->valid_evtchns);
 }
 
 static inline struct evtchn *evtchn_from_port(struct domain *d, unsigned int p)
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 80c6f62..604d047 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -336,8 +336,9 @@ struct domain
     /* Event channel information. */
     struct evtchn   *evtchn;                         /* first bucket only */
     struct evtchn  **evtchn_group[NR_EVTCHN_GROUPS]; /* all other buckets */
-    unsigned int     max_evtchns;
-    unsigned int     max_evtchn_port;
+    unsigned int     max_evtchns;     /* number supported by ABI */
+    unsigned int     max_evtchn_port; /* max permitted port number */
+    unsigned int     valid_evtchns;   /* number of allocated event channels */
     spinlock_t       event_lock;
     const struct evtchn_port_ops *evtchn_port_ops;
     struct evtchn_fifo_domain *evtchn_fifo;
-- 
1.7.10.4

Re: [PATCHv2 2/5] evtchn: simplify port_is_valid()

[Ian Campbell]

On Mon, 2015-06-15 at 16:48 +0100, David Vrabel wrote:
> By keeping a count of the number of currently valid event channels,
> port_is_valid() can be simplified.
> 
> d->valid_evtchns can also be tested without holding d->event_lock which
> will be useful later on.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
> v2:
> - Used unsigned int for d->valid_evtchns.
> ---
>  xen/common/event_channel.c |    3 +++
>  xen/include/xen/event.h    |    4 +---
>  xen/include/xen/sched.h    |    5 +++--
>  3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> index 947880f..fd48646 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -189,6 +189,8 @@ static int get_free_port(struct domain *d)
>          return -ENOMEM;
>      bucket_from_port(d, port) = chn;
>  
> +    write_atomic(&d->valid_evtchns, d->valid_evtchns + EVTCHNS_PER_BUCKET);

Shouldn't this be atomic_add? Otherwise the result of what you have here
is actually a non-atomic read/add/write.

Ian.

Re: [PATCHv2 2/5] evtchn: simplify port_is_valid()

[David Vrabel]

On 15/06/15 16:57, Ian Campbell wrote:
> On Mon, 2015-06-15 at 16:48 +0100, David Vrabel wrote:
>> By keeping a count of the number of currently valid event channels,
>> port_is_valid() can be simplified.
>>
>> d->valid_evtchns can also be tested without holding d->event_lock which
>> will be useful later on.
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> ---
>> v2:
>> - Used unsigned int for d->valid_evtchns.
>> ---
>>  xen/common/event_channel.c |    3 +++
>>  xen/include/xen/event.h    |    4 +---
>>  xen/include/xen/sched.h    |    5 +++--
>>  3 files changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
>> index 947880f..fd48646 100644
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -189,6 +189,8 @@ static int get_free_port(struct domain *d)
>>          return -ENOMEM;
>>      bucket_from_port(d, port) = chn;
>>  
>> +    write_atomic(&d->valid_evtchns, d->valid_evtchns + EVTCHNS_PER_BUCKET);
> 
> Shouldn't this be atomic_add? Otherwise the result of what you have here
> is actually a non-atomic read/add/write.

This field is only updated while holding d->event_lock.  Only reads are
unlocked.

David

Re: [PATCHv2 2/5] evtchn: simplify port_is_valid()

[Ian Campbell]

On Mon, 2015-06-15 at 16:59 +0100, David Vrabel wrote:
> On 15/06/15 16:57, Ian Campbell wrote:
> > On Mon, 2015-06-15 at 16:48 +0100, David Vrabel wrote:
> >> By keeping a count of the number of currently valid event channels,
> >> port_is_valid() can be simplified.
> >>
> >> d->valid_evtchns can also be tested without holding d->event_lock which
> >> will be useful later on.
> >>
> >> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> >> ---
> >> v2:
> >> - Used unsigned int for d->valid_evtchns.
> >> ---
> >>  xen/common/event_channel.c |    3 +++
> >>  xen/include/xen/event.h    |    4 +---
> >>  xen/include/xen/sched.h    |    5 +++--
> >>  3 files changed, 7 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> >> index 947880f..fd48646 100644
> >> --- a/xen/common/event_channel.c
> >> +++ b/xen/common/event_channel.c
> >> @@ -189,6 +189,8 @@ static int get_free_port(struct domain *d)
> >>          return -ENOMEM;
> >>      bucket_from_port(d, port) = chn;
> >>  
> >> +    write_atomic(&d->valid_evtchns, d->valid_evtchns + EVTCHNS_PER_BUCKET);
> > 
> > Shouldn't this be atomic_add? Otherwise the result of what you have here
> > is actually a non-atomic read/add/write.
> 
> This field is only updated while holding d->event_lock.  Only reads are
> unlocked.

Oh good, thanks!

Ian.

[PATCHv2 3/5] 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.

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>
---
v2:
- Compare channel pointers in double_evtchn_lock().
---
 xen/common/event_channel.c |   80 ++++++++++++++++++++++++++++++++++++++------
 xen/include/xen/sched.h    |    1 +
 2 files changed, 70 insertions(+), 11 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index fd48646..416c76f 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;
 }
@@ -228,11 +229,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:
@@ -243,6 +248,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;
@@ -285,6 +312,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;
@@ -300,6 +329,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:
@@ -340,11 +371,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:
@@ -371,10 +407,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:
@@ -449,11 +490,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
@@ -467,7 +512,6 @@ static long evtchn_bind_pirq(evtchn_bind_pirq_t *bind)
     return rc;
 }
 
-
 static long __evtchn_close(struct domain *d1, int port1)
 {
     struct domain *d2 = NULL;
@@ -574,15 +618,24 @@ static long __evtchn_close(struct domain *d1, int port1)
         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 )
@@ -609,21 +662,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 ( unlikely(lport >= read_atomic(&ld->valid_evtchns)) )
         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);
@@ -652,7 +702,7 @@ int evtchn_send(struct domain *ld, unsigned int lport)
     }
 
 out:
-    spin_unlock(&ld->event_lock);
+    spin_unlock(&lchn->lock);
 
     return ret;
 }
@@ -1163,11 +1213,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);
 
@@ -1214,6 +1268,8 @@ void notify_via_xen_event_channel(struct domain *ld, int lport)
     lchn = evtchn_from_port(ld, lport);
     ASSERT(consumer_is_xen(lchn));
 
+    spin_lock(&lchn->lock);
+
     if ( likely(lchn->state == ECS_INTERDOMAIN) )
     {
         rd    = lchn->u.interdomain.remote_dom;
@@ -1221,6 +1277,8 @@ void notify_via_xen_event_channel(struct domain *ld, int lport)
         evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
     }
 
+    spin_unlock(&lchn->lock);
+
     spin_unlock(&ld->event_lock);
 }
 
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

Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events

[Jan Beulich]

>>> On 15.06.15 at 17:48, <david.vrabel@citrix.com> wrote:
> @@ -609,21 +662,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 ( unlikely(lport >= read_atomic(&ld->valid_evtchns)) )
>          return -EINVAL;
> -    }

I don't think you really want to open code part of port_is_valid()
(and avoid other parts of it) here? Or if really so, I think a comment
should be added to explain it.

> @@ -1163,11 +1213,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);

I don't see why you shouldn't be able to move up this unlock.

And then - no change to free_xen_event_channel()?

> @@ -1214,6 +1268,8 @@ void notify_via_xen_event_channel(struct domain *ld, int lport)
>      lchn = evtchn_from_port(ld, lport);
>      ASSERT(consumer_is_xen(lchn));
>  
> +    spin_lock(&lchn->lock);
> +
>      if ( likely(lchn->state == ECS_INTERDOMAIN) )
>      {
>          rd    = lchn->u.interdomain.remote_dom;
> @@ -1221,6 +1277,8 @@ void notify_via_xen_event_channel(struct domain *ld, int lport)
>          evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
>      }
>  
> +    spin_unlock(&lchn->lock);
> +
>      spin_unlock(&ld->event_lock);
>  }

Again I think the event lock can be dropped earlier now.

Jan

Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events

[David Vrabel]

On 16/06/15 10:18, Jan Beulich wrote:
>>>> On 15.06.15 at 17:48, <david.vrabel@citrix.com> wrote:
>> @@ -609,21 +662,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 ( unlikely(lport >= read_atomic(&ld->valid_evtchns)) )
>>          return -EINVAL;
>> -    }
> 
> I don't think you really want to open code part of port_is_valid()
> (and avoid other parts of it) here? Or if really so, I think a comment
> should be added to explain it.

The ld->valid_evtchns is the only field we can safely check without
ld->event_lock.

We do check the channel state and the code that set this state uses the
full port_is_valid() call.  I'll add a comment.

> And then - no change to free_xen_event_channel()?

It looked alright to me, but now I see the clear of xen_consumer needs
to be done under the channel lock.  Probably by moving it to
__evtchn_close().

David

Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events

[Jan Beulich]

>>> On 16.06.15 at 11:34, <david.vrabel@citrix.com> wrote:
> On 16/06/15 10:18, Jan Beulich wrote:
>>>>> On 15.06.15 at 17:48, <david.vrabel@citrix.com> wrote:
>>> @@ -609,21 +662,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 ( unlikely(lport >= read_atomic(&ld->valid_evtchns)) )
>>>          return -EINVAL;
>>> -    }
>> 
>> I don't think you really want to open code part of port_is_valid()
>> (and avoid other parts of it) here? Or if really so, I think a comment
>> should be added to explain it.
> 
> The ld->valid_evtchns is the only field we can safely check without
> ld->event_lock.
> 
> We do check the channel state and the code that set this state uses the
> full port_is_valid() call.  I'll add a comment.

Hmm, port_is_valid() also checks d->max_evtchns and d->evtchn.
The latter is involved in evtchn_from_port(), so I can't see how
you checking the channel's state _afterwards_ can leverage that
whoever set this state did a full check.

Another question is whether with the ->valid_evtchns check the
->evtchn check is necessary at all anymore. (The check against
->max_evtchns isn't wrong with the lock not held, i.e. could only
end up being too strict, and hence the open coding would then
still be questionable.)

Jan

Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events

[David Vrabel]

On 16/06/15 10:51, Jan Beulich wrote:
>>>> On 16.06.15 at 11:34, <david.vrabel@citrix.com> wrote:
>> On 16/06/15 10:18, Jan Beulich wrote:
>>>>>> On 15.06.15 at 17:48, <david.vrabel@citrix.com> wrote:
>>>> @@ -609,21 +662,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 ( unlikely(lport >= read_atomic(&ld->valid_evtchns)) )
>>>>          return -EINVAL;
>>>> -    }
>>>
>>> I don't think you really want to open code part of port_is_valid()
>>> (and avoid other parts of it) here? Or if really so, I think a comment
>>> should be added to explain it.
>>
>> The ld->valid_evtchns is the only field we can safely check without
>> ld->event_lock.
>>
>> We do check the channel state and the code that set this state uses the
>> full port_is_valid() call.  I'll add a comment.
> 
> Hmm, port_is_valid() also checks d->max_evtchns and d->evtchn.
> The latter is involved in evtchn_from_port(), so I can't see how
> you checking the channel's state _afterwards_ can leverage that
> whoever set this state did a full check.
> 
> Another question is whether with the ->valid_evtchns check the
> ->evtchn check is necessary at all anymore. (The check against
> ->max_evtchns isn't wrong with the lock not held, i.e. could only
> end up being too strict, and hence the open coding would then
> still be questionable.)

Ok.  I'll remove the d->evtchn check from port_is_valid() and use it.

David

Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events

[David Vrabel]

On 16/06/15 10:18, Jan Beulich wrote:
>>>> On 15.06.15 at 17:48, <david.vrabel@citrix.com> wrote:
>> @@ -1163,11 +1213,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);
> 
> I don't see why you shouldn't be able to move up this unlock.

Because we need to (also) hold ld->event_lock while changing the state
from ECS_FREE or a concurrent get_free_port() will find this port again.

>> @@ -1221,6 +1277,8 @@ void notify_via_xen_event_channel(struct domain *ld, int lport)
>>          evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
>>      }
>>  
>> +    spin_unlock(&lchn->lock);
>> +
>>      spin_unlock(&ld->event_lock);
>>  }
> 
> Again I think the event lock can be dropped earlier now.

Ditto.

David

Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events

[David Vrabel]

On 16/06/15 16:19, David Vrabel wrote:
>>> @@ -1221,6 +1277,8 @@ void notify_via_xen_event_channel(struct domain *ld, int lport)
>>>          evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
>>>      }
>>>  
>>> +    spin_unlock(&lchn->lock);
>>> +
>>>      spin_unlock(&ld->event_lock);
>>>  }
>>
>> Again I think the event lock can be dropped earlier now.
> 
> Ditto.

Uh, no. This is notify.  I've kept the locking like this because of the
ld->is_dying check.  I think we need the ld->event_lock in case
d->is_dying is set and evtchn_destroy(ld) is called.

David

Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events

[Jan Beulich]

>>> On 16.06.15 at 17:58, <david.vrabel@citrix.com> wrote:
> On 16/06/15 16:19, David Vrabel wrote:
>>>> @@ -1221,6 +1277,8 @@ void notify_via_xen_event_channel(struct domain *ld, 
> int lport)
>>>>          evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
>>>>      }
>>>>  
>>>> +    spin_unlock(&lchn->lock);
>>>> +
>>>>      spin_unlock(&ld->event_lock);
>>>>  }
>>>
>>> Again I think the event lock can be dropped earlier now.
>> 
>> Ditto.
> 
> Uh, no. This is notify.  I've kept the locking like this because of the
> ld->is_dying check.  I think we need the ld->event_lock in case
> d->is_dying is set and evtchn_destroy(ld) is called.

Right, but if evtchn_destroy() was a concern, then this wouldn't
apply just here, but also in the sending path you are relaxing.
Afaict due to the channel lock being taken in __evtchn_close()
you can drop the even lock here as the latest after you acquired
the channel one (I haven't been able to convince myself yet that
dropping it even before that would be okay).

Jan

Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events

[David Vrabel]

On 16/06/15 17:19, Jan Beulich wrote:
>>>> On 16.06.15 at 17:58, <david.vrabel@citrix.com> wrote:
>> On 16/06/15 16:19, David Vrabel wrote:
>>>>> @@ -1221,6 +1277,8 @@ void notify_via_xen_event_channel(struct domain *ld, 
>> int lport)
>>>>>          evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
>>>>>      }
>>>>>  
>>>>> +    spin_unlock(&lchn->lock);
>>>>> +
>>>>>      spin_unlock(&ld->event_lock);
>>>>>  }
>>>>
>>>> Again I think the event lock can be dropped earlier now.
>>>
>>> Ditto.
>>
>> Uh, no. This is notify.  I've kept the locking like this because of the
>> ld->is_dying check.  I think we need the ld->event_lock in case
>> d->is_dying is set and evtchn_destroy(ld) is called.
> 
> Right, but if evtchn_destroy() was a concern, then this wouldn't
> apply just here, but also in the sending path you are relaxing.
> Afaict due to the channel lock being taken in __evtchn_close()
> you can drop the even lock here as the latest after you acquired
> the channel one (I haven't been able to convince myself yet that
> dropping it even before that would be okay).

But in the evtchn_send() case, we're in a hypercall so we know
ld->is_dying is false and thus cannot be racing with evtchn_destroy(ld).

It would be good to remove event_lock from notify_xen_event_channel() as
well since this is heavily used for ioreqs and vm events.  Let me have a
more careful look.

David

Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events

[Jan Beulich]

>>> On 16.06.15 at 18:39, <david.vrabel@citrix.com> wrote:
> On 16/06/15 17:19, Jan Beulich wrote:
>>>>> On 16.06.15 at 17:58, <david.vrabel@citrix.com> wrote:
>>> On 16/06/15 16:19, David Vrabel wrote:
>>>>>> @@ -1221,6 +1277,8 @@ void notify_via_xen_event_channel(struct domain *ld, 
>>> int lport)
>>>>>>          evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
>>>>>>      }
>>>>>>  
>>>>>> +    spin_unlock(&lchn->lock);
>>>>>> +
>>>>>>      spin_unlock(&ld->event_lock);
>>>>>>  }
>>>>>
>>>>> Again I think the event lock can be dropped earlier now.
>>>>
>>>> Ditto.
>>>
>>> Uh, no. This is notify.  I've kept the locking like this because of the
>>> ld->is_dying check.  I think we need the ld->event_lock in case
>>> d->is_dying is set and evtchn_destroy(ld) is called.
>> 
>> Right, but if evtchn_destroy() was a concern, then this wouldn't
>> apply just here, but also in the sending path you are relaxing.
>> Afaict due to the channel lock being taken in __evtchn_close()
>> you can drop the even lock here as the latest after you acquired
>> the channel one (I haven't been able to convince myself yet that
>> dropping it even before that would be okay).
> 
> But in the evtchn_send() case, we're in a hypercall so we know
> ld->is_dying is false and thus cannot be racing with evtchn_destroy(ld).

That's only the common code path; there's an evtchn_send() call
from __domain_finalise_shutdown() which afaict doesn't make such
guarantees (in particular there clearly are d != current->domain
cases here).

And then - how is being in a hypercall a guard against is_dying not
getting set?

> It would be good to remove event_lock from notify_xen_event_channel() as
> well since this is heavily used for ioreqs and vm events.  Let me have a
> more careful look.

Indeed, thanks.

Jan

Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events

[Jan Beulich]

>>> On 16.06.15 at 17:19, <david.vrabel@citrix.com> wrote:
> On 16/06/15 10:18, Jan Beulich wrote:
>>>>> On 15.06.15 at 17:48, <david.vrabel@citrix.com> wrote:
>>> @@ -1163,11 +1213,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);
>> 
>> I don't see why you shouldn't be able to move up this unlock.
> 
> Because we need to (also) hold ld->event_lock while changing the state
> from ECS_FREE or a concurrent get_free_port() will find this port again.

I buy this one (and moving the unlock up after the state adjustment
is unlikely to be worth it), but ...

>>> @@ -1221,6 +1277,8 @@ void notify_via_xen_event_channel(struct domain *ld, int lport)
>>>          evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
>>>      }
>>>  
>>> +    spin_unlock(&lchn->lock);
>>> +
>>>      spin_unlock(&ld->event_lock);
>>>  }
>> 
>> Again I think the event lock can be dropped earlier now.
> 
> Ditto.

... there's no state change involved here.

Jan

[PATCHv2 4/5] evtchn: remove the locking when unmasking an event channel

[David Vrabel]

The event channel lock is no longer required to check if the port is
valid.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/common/event_channel.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 416c76f..aa821ea 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -977,9 +977,7 @@ int evtchn_unmask(unsigned int port)
     struct domain *d = current->domain;
     struct evtchn *evtchn;
 
-    ASSERT(spin_is_locked(&d->event_lock));
-
-    if ( unlikely(!port_is_valid(d, port)) )
+    if ( port >= read_atomic(&d->valid_evtchns) )
         return -EINVAL;
 
     evtchn = evtchn_from_port(d, port);
@@ -1145,9 +1143,7 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         struct evtchn_unmask unmask;
         if ( copy_from_guest(&unmask, arg, 1) != 0 )
             return -EFAULT;
-        spin_lock(&current->domain->event_lock);
         rc = evtchn_unmask(unmask.port);
-        spin_unlock(&current->domain->event_lock);
         break;
     }
 
-- 
1.7.10.4

Re: [PATCHv2 4/5] evtchn: remove the locking when unmasking an event channel

[Jan Beulich]

>>> On 15.06.15 at 17:48, <david.vrabel@citrix.com> wrote:
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -977,9 +977,7 @@ int evtchn_unmask(unsigned int port)
>      struct domain *d = current->domain;
>      struct evtchn *evtchn;
>  
> -    ASSERT(spin_is_locked(&d->event_lock));
> -
> -    if ( unlikely(!port_is_valid(d, port)) )
> +    if ( port >= read_atomic(&d->valid_evtchns) )
>          return -EINVAL;

Again this partial open coding looks wrong to me.

Jan

Re: [PATCHv2 4/5] evtchn: remove the locking when unmasking an event channel

[David Vrabel]

On 16/06/15 10:19, Jan Beulich wrote:
>>>> On 15.06.15 at 17:48, <david.vrabel@citrix.com> wrote:
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -977,9 +977,7 @@ int evtchn_unmask(unsigned int port)
>>      struct domain *d = current->domain;
>>      struct evtchn *evtchn;
>>  
>> -    ASSERT(spin_is_locked(&d->event_lock));
>> -
>> -    if ( unlikely(!port_is_valid(d, port)) )
>> +    if ( port >= read_atomic(&d->valid_evtchns) )
>>          return -EINVAL;
> 
> Again this partial open coding looks wrong to me.

Again, d->valid_evtchns is the only field we can safely check without
d->event_lock.

I think this is fine because unmasking an unbound event channel is harmless.

David

[PATCHv2 5/5] 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 cache line), thus putting the per-channel locks
into their own cache line.

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

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
I'm not sure we actually want to do this.  I think it would be better
to pack the struct evtchn and use vmap to turn the pages into a linear
array for quicker lookup.
---
 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

Re: [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes

[Jan Beulich]

>>> On 15.06.15 at 17:48, <david.vrabel@citrix.com> wrote:
> 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 cache line), thus putting the per-channel locks
> into their own cache line.
> 
> This does not decrease the number of struct evtchn's per-page.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
> I'm not sure we actually want to do this.  I think it would be better
> to pack the struct evtchn and use vmap to turn the pages into a linear
> array for quicker lookup.

But then you'd end up with two locks on one cache line again. I.e.
I can see the possible benefit of making the tree a linear table, but
I don't see how that eliminates the desire for the change here.

Jan

Re: [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes

[Julien Grall]

Hi David,

On 15/06/2015 16:48, David Vrabel wrote:
> 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)));

Why don't you use __cacheline_aligned?

Regards,

-- 
Julien Grall

Re: [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes

[Jan Beulich]

>>> On 16.06.15 at 13:14, <julien.grall@citrix.com> wrote:
> On 15/06/2015 16:48, David Vrabel wrote:
>> 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)));
> 
> Why don't you use __cacheline_aligned?

That would double the size on x86, for little or no benefit.

Jan

Re: [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes

[Julien Grall]

On 16/06/15 12:59, Jan Beulich wrote:
>>>> On 16.06.15 at 13:14, <julien.grall@citrix.com> wrote:
>> On 15/06/2015 16:48, David Vrabel wrote:
>>> 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)));
>>
>> Why don't you use __cacheline_aligned?
> 
> That would double the size on x86, for little or no benefit.

Well, the cache line size is not necessarily 64 bytes on every
architecture. In the case of ARM, the cache line depends on the
processor version.

__cacheline_aligned is the only way to ensure that the cache line is not
shared on ARM.

AFAIU, the goal of this patch is to avoid sharing the cache line. If
not, the commit message is misleading because it claims that a cache
line is always 64 bytes...

Regards,

-- 
Julien Grall

Re: [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes

[David Vrabel]

On 16/06/15 13:57, Julien Grall wrote:
> On 16/06/15 12:59, Jan Beulich wrote:
>>>>> On 16.06.15 at 13:14, <julien.grall@citrix.com> wrote:
>>> On 15/06/2015 16:48, David Vrabel wrote:
>>>> 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)));
>>>
>>> Why don't you use __cacheline_aligned?
>>
>> That would double the size on x86, for little or no benefit.
> 
> Well, the cache line size is not necessarily 64 bytes on every
> architecture. In the case of ARM, the cache line depends on the
> processor version.
> 
> __cacheline_aligned is the only way to ensure that the cache line is not
> shared on ARM.
> 
> AFAIU, the goal of this patch is to avoid sharing the cache line. If
> not, the commit message is misleading because it claims that a cache
> line is always 64 bytes...

We want to avoid sharing the cache line where we can do so for no
additional memory cost.

David

Re: [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes

[Julien Grall]

On 16/06/15 14:13, David Vrabel wrote:
> On 16/06/15 13:57, Julien Grall wrote:
>> On 16/06/15 12:59, Jan Beulich wrote:
>>>>>> On 16.06.15 at 13:14, <julien.grall@citrix.com> wrote:
>>>> On 15/06/2015 16:48, David Vrabel wrote:
>>>>> 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)));
>>>>
>>>> Why don't you use __cacheline_aligned?
>>>
>>> That would double the size on x86, for little or no benefit.
>>
>> Well, the cache line size is not necessarily 64 bytes on every
>> architecture. In the case of ARM, the cache line depends on the
>> processor version.
>>
>> __cacheline_aligned is the only way to ensure that the cache line is not
>> shared on ARM.
>>
>> AFAIU, the goal of this patch is to avoid sharing the cache line. If
>> not, the commit message is misleading because it claims that a cache
>> line is always 64 bytes...
> 
> We want to avoid sharing the cache line where we can do so for no
> additional memory cost.

I would expand the commit message to make clear that we may not share
the cache line. The "64 bytes (cache line)" is confusing and without any
background it can be interpreted wrongly.

Regards,

-- 
Julien Grall