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

2015-06-17 Thread Jan Beulich 
 On 16.06.15 at 18:39, david.vra...@citrix.com wrote:
 On 16/06/15 17:19, Jan Beulich wrote:
 On 16.06.15 at 17:58, david.vra...@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


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-06-16 Thread 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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-06-16 Thread Jan Beulich 
 On 16.06.15 at 17:19, david.vra...@citrix.com wrote:
 On 16/06/15 10:18, Jan Beulich wrote:
 On 15.06.15 at 17:48, david.vra...@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


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-06-16 Thread David Vrabel 
On 16/06/15 17:19, Jan Beulich wrote:
 On 16.06.15 at 17:58, david.vra...@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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-06-16 Thread Jan Beulich 
 On 15.06.15 at 17:48, david.vra...@citrix.com wrote:
 @@ -609,21 +662,18 @@ int evtchn_send(struct domain *ld, unsigned int lport)
  struct domain *rd;
  intrport, 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


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-06-16 Thread David Vrabel 
On 16/06/15 10:18, Jan Beulich wrote:
 On 15.06.15 at 17:48, david.vra...@citrix.com wrote:
 @@ -609,21 +662,18 @@ int evtchn_send(struct domain *ld, unsigned int lport)
  struct domain *rd;
  intrport, 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



___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-06-16 Thread Jan Beulich 
 On 16.06.15 at 11:34, david.vra...@citrix.com wrote:
 On 16/06/15 10:18, Jan Beulich wrote:
 On 15.06.15 at 17:48, david.vra...@citrix.com wrote:
 @@ -609,21 +662,18 @@ int evtchn_send(struct domain *ld, unsigned int lport)
  struct domain *rd;
  intrport, 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


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-06-16 Thread David Vrabel 
On 16/06/15 10:51, Jan Beulich wrote:
 On 16.06.15 at 11:34, david.vra...@citrix.com wrote:
 On 16/06/15 10:18, Jan Beulich wrote:
 On 15.06.15 at 17:48, david.vra...@citrix.com wrote:
 @@ -609,21 +662,18 @@ int evtchn_send(struct domain *ld, unsigned int 
 lport)
  struct domain *rd;
  intrport, 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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-06-15 Thread 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.vra...@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;
 intrport, ret = 0;
 
-spin_lock(ld-event_lock);
-
-if ( unlikely(!port_is_valid(ld, lport)) )
-{
-spin_unlock(ld-event_lock);
+if (