* [PATCHv3 0/6] evtchn: Improve scalebility
@ 2015-06-17 12:02 David Vrabel
2015-06-17 12:02 ` [PATCHv3 1/6] evtchn: clear xen_consumer when clearing state David Vrabel
` (5 more replies)
0 siblings, 6 replies; 24+ messages in thread
From: David Vrabel @ 2015-06-17 12:02 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Tim Deegan, David Vrabel, Jan Beulich, Ian Campbell
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCHv3 1/6] evtchn: clear xen_consumer when clearing state
2015-06-17 12:02 [PATCHv3 0/6] evtchn: Improve scalebility David Vrabel
@ 2015-06-17 12:02 ` David Vrabel
2015-06-18 10:30 ` Jan Beulich
2015-06-17 12:02 ` [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final() David Vrabel
` (4 subsequent siblings)
5 siblings, 1 reply; 24+ messages in thread
From: David Vrabel @ 2015-06-17 12:02 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Tim Deegan, David Vrabel, Jan Beulich, Ian Campbell
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()
2015-06-17 12:02 [PATCHv3 0/6] evtchn: Improve scalebility David Vrabel
2015-06-17 12:02 ` [PATCHv3 1/6] evtchn: clear xen_consumer when clearing state David Vrabel
@ 2015-06-17 12:02 ` David Vrabel
2015-06-18 10:36 ` Jan Beulich
2015-06-17 12:03 ` [PATCHv3 3/6] evtchn: simplify port_is_valid() David Vrabel
` (3 subsequent siblings)
5 siblings, 1 reply; 24+ messages in thread
From: David Vrabel @ 2015-06-17 12:02 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Tim Deegan, David Vrabel, Jan Beulich, Ian Campbell
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCHv3 3/6] evtchn: simplify port_is_valid()
2015-06-17 12:02 [PATCHv3 0/6] evtchn: Improve scalebility David Vrabel
2015-06-17 12:02 ` [PATCHv3 1/6] evtchn: clear xen_consumer when clearing state David Vrabel
2015-06-17 12:02 ` [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final() David Vrabel
@ 2015-06-17 12:03 ` David Vrabel
2015-06-17 12:03 ` [PATCHv3 4/6] evtchn: use a per-event channel lock for sending events David Vrabel
` (2 subsequent siblings)
5 siblings, 0 replies; 24+ messages in thread
From: David Vrabel @ 2015-06-17 12:03 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Tim Deegan, David Vrabel, Jan Beulich, Ian Campbell
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCHv3 4/6] evtchn: use a per-event channel lock for sending events
2015-06-17 12:02 [PATCHv3 0/6] evtchn: Improve scalebility David Vrabel
` (2 preceding siblings ...)
2015-06-17 12:03 ` [PATCHv3 3/6] evtchn: simplify port_is_valid() David Vrabel
@ 2015-06-17 12:03 ` David Vrabel
2015-06-18 11:20 ` Jan Beulich
2015-06-17 12:03 ` [PATCHv3 5/6] evtchn: remove the locking when unmasking an event channel David Vrabel
2015-06-17 12:03 ` [PATCHv3 6/6] evtchn: pad struct evtchn to 64 bytes David Vrabel
5 siblings, 1 reply; 24+ messages in thread
From: David Vrabel @ 2015-06-17 12:03 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Tim Deegan, David Vrabel, Jan Beulich, Ian Campbell
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCHv3 5/6] evtchn: remove the locking when unmasking an event channel
2015-06-17 12:02 [PATCHv3 0/6] evtchn: Improve scalebility David Vrabel
` (3 preceding siblings ...)
2015-06-17 12:03 ` [PATCHv3 4/6] evtchn: use a per-event channel lock for sending events David Vrabel
@ 2015-06-17 12:03 ` David Vrabel
2015-06-18 11:30 ` Jan Beulich
2015-06-17 12:03 ` [PATCHv3 6/6] evtchn: pad struct evtchn to 64 bytes David Vrabel
5 siblings, 1 reply; 24+ messages in thread
From: David Vrabel @ 2015-06-17 12:03 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Tim Deegan, David Vrabel, Jan Beulich, Ian Campbell
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(¤t->domain->event_lock);
rc = evtchn_unmask(unmask.port);
- spin_unlock(¤t->domain->event_lock);
break;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCHv3 6/6] evtchn: pad struct evtchn to 64 bytes
2015-06-17 12:02 [PATCHv3 0/6] evtchn: Improve scalebility David Vrabel
` (4 preceding siblings ...)
2015-06-17 12:03 ` [PATCHv3 5/6] evtchn: remove the locking when unmasking an event channel David Vrabel
@ 2015-06-17 12:03 ` David Vrabel
2015-06-18 11:31 ` Jan Beulich
5 siblings, 1 reply; 24+ messages in thread
From: David Vrabel @ 2015-06-17 12:03 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Tim Deegan, David Vrabel, Jan Beulich, Ian Campbell
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCHv3 1/6] evtchn: clear xen_consumer when clearing state
2015-06-17 12:02 ` [PATCHv3 1/6] evtchn: clear xen_consumer when clearing state David Vrabel
@ 2015-06-18 10:30 ` Jan Beulich
0 siblings, 0 replies; 24+ messages in thread
From: Jan Beulich @ 2015-06-18 10:30 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
>>> 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()
2015-06-17 12:02 ` [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final() David Vrabel
@ 2015-06-18 10:36 ` Jan Beulich
2015-06-18 10:40 ` David Vrabel
0 siblings, 1 reply; 24+ messages in thread
From: Jan Beulich @ 2015-06-18 10:36 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
>>> 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()
2015-06-18 10:36 ` Jan Beulich
@ 2015-06-18 10:40 ` David Vrabel
2015-06-18 11:01 ` Jan Beulich
2015-06-19 9:29 ` Jan Beulich
0 siblings, 2 replies; 24+ messages in thread
From: David Vrabel @ 2015-06-18 10:40 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()
2015-06-18 10:40 ` David Vrabel
@ 2015-06-18 11:01 ` Jan Beulich
2015-06-19 9:29 ` Jan Beulich
1 sibling, 0 replies; 24+ messages in thread
From: Jan Beulich @ 2015-06-18 11:01 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
>>> 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv3 4/6] evtchn: use a per-event channel lock for sending events
2015-06-17 12:03 ` [PATCHv3 4/6] evtchn: use a per-event channel lock for sending events David Vrabel
@ 2015-06-18 11:20 ` Jan Beulich
2015-06-18 11:39 ` David Vrabel
0 siblings, 1 reply; 24+ messages in thread
From: Jan Beulich @ 2015-06-18 11:20 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
>>> 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv3 5/6] evtchn: remove the locking when unmasking an event channel
2015-06-17 12:03 ` [PATCHv3 5/6] evtchn: remove the locking when unmasking an event channel David Vrabel
@ 2015-06-18 11:30 ` Jan Beulich
2015-06-18 11:36 ` David Vrabel
0 siblings, 1 reply; 24+ messages in thread
From: Jan Beulich @ 2015-06-18 11:30 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
>>> 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(¤t->domain->event_lock);
> rc = evtchn_unmask(unmask.port);
> - spin_unlock(¤t->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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv3 6/6] evtchn: pad struct evtchn to 64 bytes
2015-06-17 12:03 ` [PATCHv3 6/6] evtchn: pad struct evtchn to 64 bytes David Vrabel
@ 2015-06-18 11:31 ` Jan Beulich
0 siblings, 0 replies; 24+ messages in thread
From: Jan Beulich @ 2015-06-18 11:31 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
>>> 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>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv3 5/6] evtchn: remove the locking when unmasking an event channel
2015-06-18 11:30 ` Jan Beulich
@ 2015-06-18 11:36 ` David Vrabel
2015-06-18 12:08 ` Jan Beulich
0 siblings, 1 reply; 24+ messages in thread
From: David Vrabel @ 2015-06-18 11:36 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
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(¤t->domain->event_lock);
>> rc = evtchn_unmask(unmask.port);
>> - spin_unlock(¤t->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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv3 4/6] evtchn: use a per-event channel lock for sending events
2015-06-18 11:20 ` Jan Beulich
@ 2015-06-18 11:39 ` David Vrabel
0 siblings, 0 replies; 24+ messages in thread
From: David Vrabel @ 2015-06-18 11:39 UTC (permalink / raw)
To: Jan Beulich, David Vrabel
Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv3 5/6] evtchn: remove the locking when unmasking an event channel
2015-06-18 11:36 ` David Vrabel
@ 2015-06-18 12:08 ` Jan Beulich
2015-06-18 12:17 ` David Vrabel
0 siblings, 1 reply; 24+ messages in thread
From: Jan Beulich @ 2015-06-18 12:08 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
>>> 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(¤t->domain->event_lock);
>>> rc = evtchn_unmask(unmask.port);
>>> - spin_unlock(¤t->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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv3 5/6] evtchn: remove the locking when unmasking an event channel
2015-06-18 12:08 ` Jan Beulich
@ 2015-06-18 12:17 ` David Vrabel
0 siblings, 0 replies; 24+ messages in thread
From: David Vrabel @ 2015-06-18 12:17 UTC (permalink / raw)
To: Jan Beulich, David Vrabel
Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
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(¤t->domain->event_lock);
>>>> rc = evtchn_unmask(unmask.port);
>>>> - spin_unlock(¤t->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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()
2015-06-18 10:40 ` David Vrabel
2015-06-18 11:01 ` Jan Beulich
@ 2015-06-19 9:29 ` Jan Beulich
2015-06-19 9:52 ` David Vrabel
1 sibling, 1 reply; 24+ messages in thread
From: Jan Beulich @ 2015-06-19 9:29 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
>>> 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()
2015-06-19 9:29 ` Jan Beulich
@ 2015-06-19 9:52 ` David Vrabel
2015-06-19 10:55 ` Jan Beulich
0 siblings, 1 reply; 24+ messages in thread
From: David Vrabel @ 2015-06-19 9:52 UTC (permalink / raw)
To: Jan Beulich, David Vrabel
Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()
2015-06-19 9:52 ` David Vrabel
@ 2015-06-19 10:55 ` Jan Beulich
2015-06-19 12:23 ` David Vrabel
0 siblings, 1 reply; 24+ messages in thread
From: Jan Beulich @ 2015-06-19 10:55 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
>>> 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()
2015-06-19 10:55 ` Jan Beulich
@ 2015-06-19 12:23 ` David Vrabel
2015-06-19 13:04 ` Jan Beulich
0 siblings, 1 reply; 24+ messages in thread
From: David Vrabel @ 2015-06-19 12:23 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()
2015-06-19 12:23 ` David Vrabel
@ 2015-06-19 13:04 ` Jan Beulich
2015-06-19 16:58 ` David Vrabel
0 siblings, 1 reply; 24+ messages in thread
From: Jan Beulich @ 2015-06-19 13:04 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
>>> 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final()
2015-06-19 13:04 ` Jan Beulich
@ 2015-06-19 16:58 ` David Vrabel
0 siblings, 0 replies; 24+ messages in thread
From: David Vrabel @ 2015-06-19 16:58 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
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
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2015-06-19 16:58 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-17 12:02 [PATCHv3 0/6] evtchn: Improve scalebility David Vrabel
2015-06-17 12:02 ` [PATCHv3 1/6] evtchn: clear xen_consumer when clearing state David Vrabel
2015-06-18 10:30 ` Jan Beulich
2015-06-17 12:02 ` [PATCHv3 2/6] evtchn: defer freeing struct evtchn's until evtchn_destroy_final() David Vrabel
2015-06-18 10:36 ` Jan Beulich
2015-06-18 10:40 ` David Vrabel
2015-06-18 11:01 ` Jan Beulich
2015-06-19 9:29 ` Jan Beulich
2015-06-19 9:52 ` David Vrabel
2015-06-19 10:55 ` Jan Beulich
2015-06-19 12:23 ` David Vrabel
2015-06-19 13:04 ` Jan Beulich
2015-06-19 16:58 ` David Vrabel
2015-06-17 12:03 ` [PATCHv3 3/6] evtchn: simplify port_is_valid() David Vrabel
2015-06-17 12:03 ` [PATCHv3 4/6] evtchn: use a per-event channel lock for sending events David Vrabel
2015-06-18 11:20 ` Jan Beulich
2015-06-18 11:39 ` David Vrabel
2015-06-17 12:03 ` [PATCHv3 5/6] evtchn: remove the locking when unmasking an event channel David Vrabel
2015-06-18 11:30 ` Jan Beulich
2015-06-18 11:36 ` David Vrabel
2015-06-18 12:08 ` Jan Beulich
2015-06-18 12:17 ` David Vrabel
2015-06-17 12:03 ` [PATCHv3 6/6] evtchn: pad struct evtchn to 64 bytes David Vrabel
2015-06-18 11:31 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.