[PATCHv3 0/6] evtchn: Improve scalebility

[PATCHv3 0/6] 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)

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

[PATCHv3 1/6] 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>
---
 xen/common/event_channel.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 947880f..90e3121 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -200,6 +200,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);
 }
@@ -1187,7 +1188,6 @@ 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);
 
@@ -1287,10 +1287,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);
-    }
 
     /* Free all event-channel buckets. */
     spin_lock(&d->event_lock);
-- 
1.7.10.4

Re: [PATCHv3 1/6] evtchn: clear xen_consumer when clearing state

[Jan Beulich]

>>> On 17.06.15 at 14:02, <david.vrabel@citrix.com> wrote:
> @@ -1187,7 +1188,6 @@ 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);
>  
> @@ -1287,10 +1287,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);
> -    }

How does this work with the consumer_is_xen() check in
__evtchn_close()?

Jan

[PATCHv3 2/6] 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>
---
 xen/common/event_channel.c |   48 +++++++++++---------------------------------
 1 file changed, 12 insertions(+), 36 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 90e3121..ab3b48e 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -1175,22 +1175,6 @@ 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);
 }
 
@@ -1202,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);
@@ -1279,7 +1257,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);
@@ -1289,8 +1267,17 @@ void evtchn_destroy(struct domain *d)
     for ( i = 0; port_is_valid(d, i); i++ )
         (void)__evtchn_close(d, i);
 
+    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] )
@@ -1298,20 +1285,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

Re: [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()

[Jan Beulich]

>>> On 17.06.15 at 14:02, <david.vrabel@citrix.com> wrote:
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -1175,22 +1175,6 @@ 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));

At least in debug builds I think these would better be retained.

Jan

Re: [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()

[David Vrabel]

On 18/06/15 11:36, Jan Beulich wrote:
>>>> On 17.06.15 at 14:02, <david.vrabel@citrix.com> wrote:
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -1175,22 +1175,6 @@ 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));

I can keep this one.

>> -    chn = evtchn_from_port(d, port);
>> -    BUG_ON(!consumer_is_xen(chn));
> 
> At least in debug builds I think these would better be retained.

But this one has to go because it will always trip when
free_xen_event_channel() is called after evtchn_destroy() (which will
have cleared xen_consumer).

David

Re: [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()

[Jan Beulich]

>>> On 18.06.15 at 12:40, <david.vrabel@citrix.com> wrote:
> On 18/06/15 11:36, Jan Beulich wrote:
>>>>> On 17.06.15 at 14:02, <david.vrabel@citrix.com> wrote:
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -1175,22 +1175,6 @@ 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));
> 
> I can keep this one.
> 
>>> -    chn = evtchn_from_port(d, port);
>>> -    BUG_ON(!consumer_is_xen(chn));
>> 
>> At least in debug builds I think these would better be retained.
> 
> But this one has to go because it will always trip when
> free_xen_event_channel() is called after evtchn_destroy() (which will
> have cleared xen_consumer).

Depending on how the problem with patch 1 is going to be
addressed, I suppose.

Jan

Re: [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()

[Jan Beulich]

>>> On 18.06.15 at 12:40, <david.vrabel@citrix.com> wrote:
> On 18/06/15 11:36, Jan Beulich wrote:
>>>>> On 17.06.15 at 14:02, <david.vrabel@citrix.com> wrote:
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -1175,22 +1175,6 @@ 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));
> 
> I can keep this one.
> 
>>> -    chn = evtchn_from_port(d, port);
>>> -    BUG_ON(!consumer_is_xen(chn));
>> 
>> At least in debug builds I think these would better be retained.
> 
> But this one has to go because it will always trip when
> free_xen_event_channel() is called after evtchn_destroy() (which will
> have cleared xen_consumer).

Then why not

    BUG_ON(!consumer_is_xen(chn) && !d->is_dying);

or keep the d->is_dying check in place? I can see why accelerating
notify_via_xen_event_channel() is useful, but
free_xen_event_channel()?

Jan

Re: [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()

[David Vrabel]

On 19/06/15 10:29, Jan Beulich wrote:
>>>> On 18.06.15 at 12:40, <david.vrabel@citrix.com> wrote:
>> On 18/06/15 11:36, Jan Beulich wrote:
>>>>>> On 17.06.15 at 14:02, <david.vrabel@citrix.com> wrote:
>>>> --- a/xen/common/event_channel.c
>>>> +++ b/xen/common/event_channel.c
>>>> @@ -1175,22 +1175,6 @@ 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));
>>
>> I can keep this one.
>>
>>>> -    chn = evtchn_from_port(d, port);
>>>> -    BUG_ON(!consumer_is_xen(chn));
>>>
>>> At least in debug builds I think these would better be retained.
>>
>> But this one has to go because it will always trip when
>> free_xen_event_channel() is called after evtchn_destroy() (which will
>> have cleared xen_consumer).
> 
> Then why not
> 
>     BUG_ON(!consumer_is_xen(chn) && !d->is_dying);
> 
> or keep the d->is_dying check in place? I can see why accelerating
> notify_via_xen_event_channel() is useful, but
> free_xen_event_channel()?

This BUG_ON() is a pretty weak check and I don't really see the point of
it.  I'm not respinning v4 just for this.

David

Re: [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()

[Jan Beulich]

>>> On 19.06.15 at 11:52, <david.vrabel@citrix.com> wrote:
> On 19/06/15 10:29, Jan Beulich wrote:
>>>>> On 18.06.15 at 12:40, <david.vrabel@citrix.com> wrote:
>>> On 18/06/15 11:36, Jan Beulich wrote:
>>>>>>> On 17.06.15 at 14:02, <david.vrabel@citrix.com> wrote:
>>>>> --- a/xen/common/event_channel.c
>>>>> +++ b/xen/common/event_channel.c
>>>>> @@ -1175,22 +1175,6 @@ 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));
>>>
>>> I can keep this one.
>>>
>>>>> -    chn = evtchn_from_port(d, port);
>>>>> -    BUG_ON(!consumer_is_xen(chn));
>>>>
>>>> At least in debug builds I think these would better be retained.
>>>
>>> But this one has to go because it will always trip when
>>> free_xen_event_channel() is called after evtchn_destroy() (which will
>>> have cleared xen_consumer).
>> 
>> Then why not
>> 
>>     BUG_ON(!consumer_is_xen(chn) && !d->is_dying);
>> 
>> or keep the d->is_dying check in place? I can see why accelerating
>> notify_via_xen_event_channel() is useful, but
>> free_xen_event_channel()?
> 
> This BUG_ON() is a pretty weak check and I don't really see the point of
> it.  I'm not respinning v4 just for this.

I'm not sure what makes this more weak than the other BUG_ON()
you agreed to retain - both try to validate that the callers don't do
bad things. Admitted, both would better be ASSERT()s...

As to spinning v4 - I see no need, as I can easily adjust this while
committing, as long as you don't disagree to have your name under
the result.

Jan

Re: [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()

[David Vrabel]

On 19/06/15 11:55, Jan Beulich wrote:
>>>> On 19.06.15 at 11:52, <david.vrabel@citrix.com> wrote:
>> On 19/06/15 10:29, Jan Beulich wrote:
>>>>>> On 18.06.15 at 12:40, <david.vrabel@citrix.com> wrote:
>>>> On 18/06/15 11:36, Jan Beulich wrote:
>>>>>>>> On 17.06.15 at 14:02, <david.vrabel@citrix.com> wrote:
>>>>>> --- a/xen/common/event_channel.c
>>>>>> +++ b/xen/common/event_channel.c
>>>>>> @@ -1175,22 +1175,6 @@ 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));
>>>>
>>>> I can keep this one.
>>>>
>>>>>> -    chn = evtchn_from_port(d, port);
>>>>>> -    BUG_ON(!consumer_is_xen(chn));
>>>>>
>>>>> At least in debug builds I think these would better be retained.
>>>>
>>>> But this one has to go because it will always trip when
>>>> free_xen_event_channel() is called after evtchn_destroy() (which will
>>>> have cleared xen_consumer).
>>>
>>> Then why not
>>>
>>>     BUG_ON(!consumer_is_xen(chn) && !d->is_dying);
>>>
>>> or keep the d->is_dying check in place? I can see why accelerating
>>> notify_via_xen_event_channel() is useful, but
>>> free_xen_event_channel()?
>>
>> This BUG_ON() is a pretty weak check and I don't really see the point of
>> it.  I'm not respinning v4 just for this.
> 
> I'm not sure what makes this more weak than the other BUG_ON()
> you agreed to retain - both try to validate that the callers don't do
> bad things. Admitted, both would better be ASSERT()s...
> 
> As to spinning v4 - I see no need, as I can easily adjust this while
> committing, as long as you don't disagree to have your name under
> the result.

I disagree.

For this assert to be safe it needs to take suitable locks such as:

#ifdef DEBUG
    struct evtchn *chn;

    chn = evtchn_from_port(d, port);
    spin_lock(&chn->lock);
    BUG_ON(chn->state != ECS_FREE && !consumer_is_xen(chn));
    spin_unlock(&chn->lock);
#endif

or if you want the is_dying check, you need the event lock instead.

I wrote the first one, saw it was lots of noise for almost no gain and
threw it away.

David

Re: [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()

[Jan Beulich]

>>> On 19.06.15 at 14:23, <david.vrabel@citrix.com> wrote:
> On 19/06/15 11:55, Jan Beulich wrote:
>>>>> On 19.06.15 at 11:52, <david.vrabel@citrix.com> wrote:
>>> On 19/06/15 10:29, Jan Beulich wrote:
>>>>>>> On 18.06.15 at 12:40, <david.vrabel@citrix.com> wrote:
>>>>> On 18/06/15 11:36, Jan Beulich wrote:
>>>>>>>>> On 17.06.15 at 14:02, <david.vrabel@citrix.com> wrote:
>>>>>>> --- a/xen/common/event_channel.c
>>>>>>> +++ b/xen/common/event_channel.c
>>>>>>> @@ -1175,22 +1175,6 @@ 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));
>>>>>
>>>>> I can keep this one.
>>>>>
>>>>>>> -    chn = evtchn_from_port(d, port);
>>>>>>> -    BUG_ON(!consumer_is_xen(chn));
>>>>>>
>>>>>> At least in debug builds I think these would better be retained.
>>>>>
>>>>> But this one has to go because it will always trip when
>>>>> free_xen_event_channel() is called after evtchn_destroy() (which will
>>>>> have cleared xen_consumer).
>>>>
>>>> Then why not
>>>>
>>>>     BUG_ON(!consumer_is_xen(chn) && !d->is_dying);
>>>>
>>>> or keep the d->is_dying check in place? I can see why accelerating
>>>> notify_via_xen_event_channel() is useful, but
>>>> free_xen_event_channel()?
>>>
>>> This BUG_ON() is a pretty weak check and I don't really see the point of
>>> it.  I'm not respinning v4 just for this.
>> 
>> I'm not sure what makes this more weak than the other BUG_ON()
>> you agreed to retain - both try to validate that the callers don't do
>> bad things. Admitted, both would better be ASSERT()s...
>> 
>> As to spinning v4 - I see no need, as I can easily adjust this while
>> committing, as long as you don't disagree to have your name under
>> the result.
> 
> I disagree.
> 
> For this assert to be safe it needs to take suitable locks such as:
> 
> #ifdef DEBUG
>     struct evtchn *chn;
> 
>     chn = evtchn_from_port(d, port);
>     spin_lock(&chn->lock);
>     BUG_ON(chn->state != ECS_FREE && !consumer_is_xen(chn));
>     spin_unlock(&chn->lock);
> #endif
> 
> or if you want the is_dying check, you need the event lock instead.
> 
> I wrote the first one, saw it was lots of noise for almost no gain and
> threw it away.

Which is why as an alternative I suggested not to touch
free_xen_event_channel() at all in this patch.

Jan

Re: [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()

[David Vrabel]

On 19/06/15 14:04, Jan Beulich wrote:
>>>> On 19.06.15 at 14:23, <david.vrabel@citrix.com> wrote:
>> On 19/06/15 11:55, Jan Beulich wrote:
>>>>>> On 19.06.15 at 11:52, <david.vrabel@citrix.com> wrote:
>>>> On 19/06/15 10:29, Jan Beulich wrote:
>>>>>>>> On 18.06.15 at 12:40, <david.vrabel@citrix.com> wrote:
>>>>>> On 18/06/15 11:36, Jan Beulich wrote:
>>>>>>>>>> On 17.06.15 at 14:02, <david.vrabel@citrix.com> wrote:
>>>>>>>> --- a/xen/common/event_channel.c
>>>>>>>> +++ b/xen/common/event_channel.c
>>>>>>>> @@ -1175,22 +1175,6 @@ 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));
>>>>>>
>>>>>> I can keep this one.
>>>>>>
>>>>>>>> -    chn = evtchn_from_port(d, port);
>>>>>>>> -    BUG_ON(!consumer_is_xen(chn));
>>>>>>>
>>>>>>> At least in debug builds I think these would better be retained.
>>>>>>
>>>>>> But this one has to go because it will always trip when
>>>>>> free_xen_event_channel() is called after evtchn_destroy() (which will
>>>>>> have cleared xen_consumer).
>>>>>
>>>>> Then why not
>>>>>
>>>>>     BUG_ON(!consumer_is_xen(chn) && !d->is_dying);
>>>>>
>>>>> or keep the d->is_dying check in place? I can see why accelerating
>>>>> notify_via_xen_event_channel() is useful, but
>>>>> free_xen_event_channel()?
>>>>
>>>> This BUG_ON() is a pretty weak check and I don't really see the point of
>>>> it.  I'm not respinning v4 just for this.
>>>
>>> I'm not sure what makes this more weak than the other BUG_ON()
>>> you agreed to retain - both try to validate that the callers don't do
>>> bad things. Admitted, both would better be ASSERT()s...
>>>
>>> As to spinning v4 - I see no need, as I can easily adjust this while
>>> committing, as long as you don't disagree to have your name under
>>> the result.
>>
>> I disagree.
>>
>> For this assert to be safe it needs to take suitable locks such as:
>>
>> #ifdef DEBUG
>>     struct evtchn *chn;
>>
>>     chn = evtchn_from_port(d, port);
>>     spin_lock(&chn->lock);
>>     BUG_ON(chn->state != ECS_FREE && !consumer_is_xen(chn));
>>     spin_unlock(&chn->lock);
>> #endif
>>
>> or if you want the is_dying check, you need the event lock instead.
>>
>> I wrote the first one, saw it was lots of noise for almost no gain and
>> threw it away.
> 
> Which is why as an alternative I suggested not to touch
> free_xen_event_channel() at all in this patch.

I found the this is_dying check confusing and since it is now
unnecessary and not very useful it is better to remove it to make the
code easier for others to understand.

David

[PATCHv3 3/6] 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 is only increased (while holding d->event_lock), so
port_is_valid() may be safely called without taking the lock (this
will be useful later).

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
v3:
- Remove redundant d->evtchn test.

v2:
- Used unsigned int for d->valid_evtchns.
---
 xen/common/event_channel.c |    3 +++
 xen/include/xen/event.h    |    6 +-----
 xen/include/xen/sched.h    |    5 +++--
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index ab3b48e..4924796 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;
 }
 
@@ -1232,6 +1234,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..af923d1 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -89,11 +89,7 @@ static inline bool_t port_is_valid(struct domain *d, unsigned int p)
 {
     if ( p >= d->max_evtchns )
         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

[PATCHv3 4/6] 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>
---
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 |   82 ++++++++++++++++++++++++++++++++++++--------
 xen/include/xen/sched.h    |    1 +
 2 files changed, 69 insertions(+), 14 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 4924796..e7c4445 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
@@ -468,7 +513,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;
@@ -575,15 +619,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 )
@@ -610,21 +663,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 +703,7 @@ int evtchn_send(struct domain *ld, unsigned int lport)
     }
 
 out:
-    spin_unlock(&ld->event_lock);
+    spin_unlock(&lchn->lock);
 
     return ret;
 }
@@ -1164,11 +1214,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);
 
@@ -1186,11 +1240,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));
@@ -1199,7 +1253,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

Re: [PATCHv3 4/6] evtchn: use a per-event channel lock for sending events

[Jan Beulich]

>>> On 17.06.15 at 14:03, <david.vrabel@citrix.com> wrote:
> 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>

But iiuc this functionally depends on the earlier patches, and
hence can't go in without the issues there addressed.

Jan

Re: [PATCHv3 4/6] evtchn: use a per-event channel lock for sending events

[David Vrabel]

On 18/06/15 12:20, Jan Beulich wrote:
>>>> On 17.06.15 at 14:03, <david.vrabel@citrix.com> wrote:
>> 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>
> 
> But iiuc this functionally depends on the earlier patches, and
> hence can't go in without the issues there addressed.

Yes, this depends on #1, #2 and #3.

David

[PATCHv3 5/6] 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>
---
v3:
- Use port_is_valid() again.
---
 xen/common/event_channel.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index e7c4445..eabb4c8 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -978,8 +978,6 @@ 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)) )
         return -EINVAL;
 
@@ -1146,9 +1144,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: [PATCHv3 5/6] evtchn: remove the locking when unmasking an event channel

[Jan Beulich]

>>> On 17.06.15 at 14:03, <david.vrabel@citrix.com> wrote:
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -978,8 +978,6 @@ 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)) )
>          return -EINVAL;
>  
> @@ -1146,9 +1144,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);

And, looking particularly at evtchn_fifo_unmask() (and its descendant
evtchn_fifo_set_pending()), you get away without acquiring the port
lock in or around evtchn_port_unmask()? If indeed so, this one would
again be independent on 1, 2, and 4, i.e. could go in together with 3.

Jan

Re: [PATCHv3 5/6] evtchn: remove the locking when unmasking an event channel

[David Vrabel]

On 18/06/15 12:30, Jan Beulich wrote:
>>>> On 17.06.15 at 14:03, <david.vrabel@citrix.com> wrote:
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -978,8 +978,6 @@ 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)) )
>>          return -EINVAL;
>>  
>> @@ -1146,9 +1144,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);
> 
> And, looking particularly at evtchn_fifo_unmask() (and its descendant
> evtchn_fifo_set_pending()), you get away without acquiring the port
> lock in or around evtchn_port_unmask()? If indeed so, this one would
> again be independent on 1, 2, and 4, i.e. could go in together with 3.

Yes.  This is only dependent on #3 (simplify port_is_valid()).

David

Re: [PATCHv3 5/6] evtchn: remove the locking when unmasking an event channel

[Jan Beulich]

>>> On 18.06.15 at 13:36, <david.vrabel@citrix.com> wrote:
> On 18/06/15 12:30, Jan Beulich wrote:
>>>>> On 17.06.15 at 14:03, <david.vrabel@citrix.com> wrote:
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -978,8 +978,6 @@ 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)) )
>>>          return -EINVAL;
>>>  
>>> @@ -1146,9 +1144,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);
>> 
>> And, looking particularly at evtchn_fifo_unmask() (and its descendant
>> evtchn_fifo_set_pending()), you get away without acquiring the port
>> lock in or around evtchn_port_unmask()? If indeed so, this one would
>> again be independent on 1, 2, and 4, i.e. could go in together with 3.
> 
> Yes.  This is only dependent on #3 (simplify port_is_valid()).

I'm still not convinced that not taking the port lock is correct: It
is my understanding that e.g. the (last_vcpu_id,last_priority) pair
needs to be updated atomically. Yet nothing prevents the
(notify_vcpu_id,priority) pair now from getting updated in the
middle of it being snapshot in evtchn_fifo_set_pending(). Are you
saying this is no problem?

Jan

Re: [PATCHv3 5/6] evtchn: remove the locking when unmasking an event channel

[David Vrabel]

On 18/06/15 13:08, Jan Beulich wrote:
>>>> On 18.06.15 at 13:36, <david.vrabel@citrix.com> wrote:
>> On 18/06/15 12:30, Jan Beulich wrote:
>>>>>> On 17.06.15 at 14:03, <david.vrabel@citrix.com> wrote:
>>>> --- a/xen/common/event_channel.c
>>>> +++ b/xen/common/event_channel.c
>>>> @@ -978,8 +978,6 @@ 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)) )
>>>>          return -EINVAL;
>>>>  
>>>> @@ -1146,9 +1144,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);
>>>
>>> And, looking particularly at evtchn_fifo_unmask() (and its descendant
>>> evtchn_fifo_set_pending()), you get away without acquiring the port
>>> lock in or around evtchn_port_unmask()? If indeed so, this one would
>>> again be independent on 1, 2, and 4, i.e. could go in together with 3.
>>
>> Yes.  This is only dependent on #3 (simplify port_is_valid()).
> 
> I'm still not convinced that not taking the port lock is correct: It
> is my understanding that e.g. the (last_vcpu_id,last_priority) pair
> needs to be updated atomically. Yet nothing prevents the
> (notify_vcpu_id,priority) pair now from getting updated in the
> middle of it being snapshot in evtchn_fifo_set_pending(). Are you
> saying this is no problem?

This is serialized by the q lock.

evtchn_fifo_set_pending() has never been serialized by the event lock
because concurrent evtchn_send() calls with two interdomain channels
from different domains would have taken different event locks.

David

[PATCHv3 6/6] 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>
---
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

Re: [PATCHv3 6/6] evtchn: pad struct evtchn to 64 bytes

[Jan Beulich]

>>> On 17.06.15 at 14:03, <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 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>