* [PATCHv2 0/5] evtchn: Improve scalebility
@ 2015-06-15 15:48 David Vrabel
2015-06-15 15:48 ` [PATCHv2 1/5] evtchn: factor out freeing an event channel David Vrabel
` (4 more replies)
0 siblings, 5 replies; 27+ messages in thread
From: David Vrabel @ 2015-06-15 15:48 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)
v2:
- Use unsigned int for d->valid_evtchns.
- Compare channel pointers in double_evtchn_lock().
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCHv2 1/5] evtchn: factor out freeing an event channel
2015-06-15 15:48 [PATCHv2 0/5] evtchn: Improve scalebility David Vrabel
@ 2015-06-15 15:48 ` David Vrabel
2015-06-15 15:48 ` [PATCHv2 2/5] evtchn: simplify port_is_valid() David Vrabel
` (3 subsequent siblings)
4 siblings, 0 replies; 27+ messages in thread
From: David Vrabel @ 2015-06-15 15:48 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Tim Deegan, David Vrabel, Jan Beulich, Ian Campbell
We're going to want to free an event channel from two places. Factor out
the code into a free_evtchn() function.
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
xen/common/event_channel.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index bf9b2f8..947880f 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -192,6 +192,17 @@ static int get_free_port(struct domain *d)
return port;
}
+static void free_evtchn(struct domain *d, struct evtchn *chn)
+{
+ /* Clear pending event to avoid unexpected behavior on re-bind. */
+ evtchn_port_clear_pending(d, chn);
+
+ /* Reset binding to vcpu0 when the channel is freed. */
+ chn->state = ECS_FREE;
+ chn->notify_vcpu_id = 0;
+
+ xsm_evtchn_close_post(chn);
+}
static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
{
@@ -569,14 +580,7 @@ static long __evtchn_close(struct domain *d1, int port1)
BUG();
}
- /* Clear pending event to avoid unexpected behavior on re-bind. */
- evtchn_port_clear_pending(d1, chn1);
-
- /* Reset binding to vcpu0 when the channel is freed. */
- chn1->state = ECS_FREE;
- chn1->notify_vcpu_id = 0;
-
- xsm_evtchn_close_post(chn1);
+ free_evtchn(d1, chn1);
out:
if ( d2 != NULL )
--
1.7.10.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCHv2 2/5] evtchn: simplify port_is_valid()
2015-06-15 15:48 [PATCHv2 0/5] evtchn: Improve scalebility David Vrabel
2015-06-15 15:48 ` [PATCHv2 1/5] evtchn: factor out freeing an event channel David Vrabel
@ 2015-06-15 15:48 ` David Vrabel
2015-06-15 15:57 ` Ian Campbell
2015-06-15 15:48 ` [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events David Vrabel
` (2 subsequent siblings)
4 siblings, 1 reply; 27+ messages in thread
From: David Vrabel @ 2015-06-15 15:48 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 can also be tested without holding d->event_lock which
will be useful later on.
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
v2:
- Used unsigned int for d->valid_evtchns.
---
xen/common/event_channel.c | 3 +++
xen/include/xen/event.h | 4 +---
xen/include/xen/sched.h | 5 +++--
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 947880f..fd48646 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -189,6 +189,8 @@ static int get_free_port(struct domain *d)
return -ENOMEM;
bucket_from_port(d, port) = chn;
+ write_atomic(&d->valid_evtchns, d->valid_evtchns + EVTCHNS_PER_BUCKET);
+
return port;
}
@@ -1254,6 +1256,7 @@ int evtchn_init(struct domain *d)
d->evtchn = alloc_evtchn_bucket(d, 0);
if ( !d->evtchn )
return -ENOMEM;
+ d->valid_evtchns = EVTCHNS_PER_BUCKET;
spin_lock_init_prof(d, event_lock);
if ( get_free_port(d) != 0 )
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index 690f865..a08ecdb 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -91,9 +91,7 @@ static inline bool_t port_is_valid(struct domain *d, unsigned int p)
return 0;
if ( !d->evtchn )
return 0;
- if ( p < EVTCHNS_PER_BUCKET )
- return 1;
- return group_from_port(d, p) != NULL && bucket_from_port(d, p) != NULL;
+ return p < read_atomic(&d->valid_evtchns);
}
static inline struct evtchn *evtchn_from_port(struct domain *d, unsigned int p)
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 80c6f62..604d047 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -336,8 +336,9 @@ struct domain
/* Event channel information. */
struct evtchn *evtchn; /* first bucket only */
struct evtchn **evtchn_group[NR_EVTCHN_GROUPS]; /* all other buckets */
- unsigned int max_evtchns;
- unsigned int max_evtchn_port;
+ unsigned int max_evtchns; /* number supported by ABI */
+ unsigned int max_evtchn_port; /* max permitted port number */
+ unsigned int valid_evtchns; /* number of allocated event channels */
spinlock_t event_lock;
const struct evtchn_port_ops *evtchn_port_ops;
struct evtchn_fifo_domain *evtchn_fifo;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events
2015-06-15 15:48 [PATCHv2 0/5] evtchn: Improve scalebility David Vrabel
2015-06-15 15:48 ` [PATCHv2 1/5] evtchn: factor out freeing an event channel David Vrabel
2015-06-15 15:48 ` [PATCHv2 2/5] evtchn: simplify port_is_valid() David Vrabel
@ 2015-06-15 15:48 ` David Vrabel
2015-06-16 9:18 ` Jan Beulich
2015-06-15 15:48 ` [PATCHv2 4/5] evtchn: remove the locking when unmasking an event channel David Vrabel
2015-06-15 15:48 ` [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes David Vrabel
4 siblings, 1 reply; 27+ messages in thread
From: David Vrabel @ 2015-06-15 15:48 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.
To avoid having to take the remote event channel locks when sending to
an interdomain event channel, the local and remote channel locks are
both held when binding or closing an interdomain event channel.
This significantly increases the number of events that can be sent
from multiple VCPUs.
But, struct evtchn increases in size, reducing the number that fit
into a single page to 64 (instead of 128).
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
v2:
- Compare channel pointers in double_evtchn_lock().
---
xen/common/event_channel.c | 80 ++++++++++++++++++++++++++++++++++++++------
xen/include/xen/sched.h | 1 +
2 files changed, 70 insertions(+), 11 deletions(-)
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index fd48646..416c76f 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -139,6 +139,7 @@ static struct evtchn *alloc_evtchn_bucket(struct domain *d, unsigned int port)
return NULL;
}
chn[i].port = port + i;
+ spin_lock_init(&chn[i].lock);
}
return chn;
}
@@ -228,11 +229,15 @@ static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
if ( rc )
goto out;
+ spin_lock(&chn->lock);
+
chn->state = ECS_UNBOUND;
if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF )
chn->u.unbound.remote_domid = current->domain->domain_id;
evtchn_port_init(d, chn);
+ spin_unlock(&chn->lock);
+
alloc->port = port;
out:
@@ -243,6 +248,28 @@ static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
}
+static void double_evtchn_lock(struct evtchn *lchn, struct evtchn *rchn)
+{
+ if ( lchn < rchn )
+ {
+ spin_lock(&lchn->lock);
+ spin_lock(&rchn->lock);
+ }
+ else
+ {
+ if ( lchn != rchn )
+ spin_lock(&rchn->lock);
+ spin_lock(&lchn->lock);
+ }
+}
+
+static void double_evtchn_unlock(struct evtchn *lchn, struct evtchn *rchn)
+{
+ spin_unlock(&lchn->lock);
+ if ( lchn != rchn )
+ spin_unlock(&rchn->lock);
+}
+
static long evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
{
struct evtchn *lchn, *rchn;
@@ -285,6 +312,8 @@ static long evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
if ( rc )
goto out;
+ double_evtchn_lock(lchn, rchn);
+
lchn->u.interdomain.remote_dom = rd;
lchn->u.interdomain.remote_port = rport;
lchn->state = ECS_INTERDOMAIN;
@@ -300,6 +329,8 @@ static long evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
*/
evtchn_port_set_pending(ld, lchn->notify_vcpu_id, lchn);
+ double_evtchn_unlock(lchn, rchn);
+
bind->local_port = lport;
out:
@@ -340,11 +371,16 @@ static long evtchn_bind_virq(evtchn_bind_virq_t *bind)
ERROR_EXIT(port);
chn = evtchn_from_port(d, port);
+
+ spin_lock(&chn->lock);
+
chn->state = ECS_VIRQ;
chn->notify_vcpu_id = vcpu;
chn->u.virq = virq;
evtchn_port_init(d, chn);
+ spin_unlock(&chn->lock);
+
v->virq_to_evtchn[virq] = bind->port = port;
out:
@@ -371,10 +407,15 @@ static long evtchn_bind_ipi(evtchn_bind_ipi_t *bind)
ERROR_EXIT(port);
chn = evtchn_from_port(d, port);
+
+ spin_lock(&chn->lock);
+
chn->state = ECS_IPI;
chn->notify_vcpu_id = vcpu;
evtchn_port_init(d, chn);
+ spin_unlock(&chn->lock);
+
bind->port = port;
out:
@@ -449,11 +490,15 @@ static long evtchn_bind_pirq(evtchn_bind_pirq_t *bind)
goto out;
}
+ spin_lock(&chn->lock);
+
chn->state = ECS_PIRQ;
chn->u.pirq.irq = pirq;
link_pirq_port(port, chn, v);
evtchn_port_init(d, chn);
+ spin_unlock(&chn->lock);
+
bind->port = port;
#ifdef CONFIG_X86
@@ -467,7 +512,6 @@ static long evtchn_bind_pirq(evtchn_bind_pirq_t *bind)
return rc;
}
-
static long __evtchn_close(struct domain *d1, int port1)
{
struct domain *d2 = NULL;
@@ -574,15 +618,24 @@ static long __evtchn_close(struct domain *d1, int port1)
BUG_ON(chn2->state != ECS_INTERDOMAIN);
BUG_ON(chn2->u.interdomain.remote_dom != d1);
+ double_evtchn_lock(chn1, chn2);
+
+ free_evtchn(d1, chn1);
+
chn2->state = ECS_UNBOUND;
chn2->u.unbound.remote_domid = d1->domain_id;
- break;
+
+ double_evtchn_unlock(chn1, chn2);
+
+ goto out;
default:
BUG();
}
+ spin_lock(&chn1->lock);
free_evtchn(d1, chn1);
+ spin_unlock(&chn1->lock);
out:
if ( d2 != NULL )
@@ -609,21 +662,18 @@ int evtchn_send(struct domain *ld, unsigned int lport)
struct domain *rd;
int rport, ret = 0;
- spin_lock(&ld->event_lock);
-
- if ( unlikely(!port_is_valid(ld, lport)) )
- {
- spin_unlock(&ld->event_lock);
+ if ( unlikely(lport >= read_atomic(&ld->valid_evtchns)) )
return -EINVAL;
- }
lchn = evtchn_from_port(ld, lport);
+ spin_lock(&lchn->lock);
+
/* Guest cannot send via a Xen-attached event channel. */
if ( unlikely(consumer_is_xen(lchn)) )
{
- spin_unlock(&ld->event_lock);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
ret = xsm_evtchn_send(XSM_HOOK, ld, lchn);
@@ -652,7 +702,7 @@ int evtchn_send(struct domain *ld, unsigned int lport)
}
out:
- spin_unlock(&ld->event_lock);
+ spin_unlock(&lchn->lock);
return ret;
}
@@ -1163,11 +1213,15 @@ int alloc_unbound_xen_event_channel(
if ( rc )
goto out;
+ spin_lock(&chn->lock);
+
chn->state = ECS_UNBOUND;
chn->xen_consumer = get_xen_consumer(notification_fn);
chn->notify_vcpu_id = lvcpu;
chn->u.unbound.remote_domid = remote_domid;
+ spin_unlock(&chn->lock);
+
out:
spin_unlock(&ld->event_lock);
@@ -1214,6 +1268,8 @@ void notify_via_xen_event_channel(struct domain *ld, int lport)
lchn = evtchn_from_port(ld, lport);
ASSERT(consumer_is_xen(lchn));
+ spin_lock(&lchn->lock);
+
if ( likely(lchn->state == ECS_INTERDOMAIN) )
{
rd = lchn->u.interdomain.remote_dom;
@@ -1221,6 +1277,8 @@ void notify_via_xen_event_channel(struct domain *ld, int lport)
evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
}
+ spin_unlock(&lchn->lock);
+
spin_unlock(&ld->event_lock);
}
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 604d047..44ea92d 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -79,6 +79,7 @@ extern domid_t hardware_domid;
struct evtchn
{
+ spinlock_t lock;
#define ECS_FREE 0 /* Channel is available for use. */
#define ECS_RESERVED 1 /* Channel is reserved. */
#define ECS_UNBOUND 2 /* Channel is waiting to bind to a remote domain. */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCHv2 4/5] evtchn: remove the locking when unmasking an event channel
2015-06-15 15:48 [PATCHv2 0/5] evtchn: Improve scalebility David Vrabel
` (2 preceding siblings ...)
2015-06-15 15:48 ` [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events David Vrabel
@ 2015-06-15 15:48 ` David Vrabel
2015-06-16 9:19 ` Jan Beulich
2015-06-15 15:48 ` [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes David Vrabel
4 siblings, 1 reply; 27+ messages in thread
From: David Vrabel @ 2015-06-15 15:48 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>
---
xen/common/event_channel.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 416c76f..aa821ea 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -977,9 +977,7 @@ int evtchn_unmask(unsigned int port)
struct domain *d = current->domain;
struct evtchn *evtchn;
- ASSERT(spin_is_locked(&d->event_lock));
-
- if ( unlikely(!port_is_valid(d, port)) )
+ if ( port >= read_atomic(&d->valid_evtchns) )
return -EINVAL;
evtchn = evtchn_from_port(d, port);
@@ -1145,9 +1143,7 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
struct evtchn_unmask unmask;
if ( copy_from_guest(&unmask, arg, 1) != 0 )
return -EFAULT;
- spin_lock(¤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] 27+ messages in thread
* [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes
2015-06-15 15:48 [PATCHv2 0/5] evtchn: Improve scalebility David Vrabel
` (3 preceding siblings ...)
2015-06-15 15:48 ` [PATCHv2 4/5] evtchn: remove the locking when unmasking an event channel David Vrabel
@ 2015-06-15 15:48 ` David Vrabel
2015-06-16 9:22 ` Jan Beulich
2015-06-16 11:14 ` Julien Grall
4 siblings, 2 replies; 27+ messages in thread
From: David Vrabel @ 2015-06-15 15:48 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 cache line), thus putting the per-channel locks
into their own cache line.
This does not decrease the number of struct evtchn's per-page.
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
I'm not sure we actually want to do this. I think it would be better
to pack the struct evtchn and use vmap to turn the pages into a linear
array for quicker lookup.
---
xen/include/xen/sched.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 44ea92d..a0ff9d2 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -129,7 +129,7 @@ struct evtchn
#endif
} ssid;
#endif
-};
+} __attribute__((aligned(64)));
int evtchn_init(struct domain *d); /* from domain_create */
void evtchn_destroy(struct domain *d); /* from domain_kill */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCHv2 2/5] evtchn: simplify port_is_valid()
2015-06-15 15:48 ` [PATCHv2 2/5] evtchn: simplify port_is_valid() David Vrabel
@ 2015-06-15 15:57 ` Ian Campbell
2015-06-15 15:59 ` David Vrabel
0 siblings, 1 reply; 27+ messages in thread
From: Ian Campbell @ 2015-06-15 15:57 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Tim Deegan, Jan Beulich
On Mon, 2015-06-15 at 16:48 +0100, David Vrabel wrote:
> By keeping a count of the number of currently valid event channels,
> port_is_valid() can be simplified.
>
> d->valid_evtchns can also be tested without holding d->event_lock which
> will be useful later on.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
> v2:
> - Used unsigned int for d->valid_evtchns.
> ---
> xen/common/event_channel.c | 3 +++
> xen/include/xen/event.h | 4 +---
> xen/include/xen/sched.h | 5 +++--
> 3 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> index 947880f..fd48646 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -189,6 +189,8 @@ static int get_free_port(struct domain *d)
> return -ENOMEM;
> bucket_from_port(d, port) = chn;
>
> + write_atomic(&d->valid_evtchns, d->valid_evtchns + EVTCHNS_PER_BUCKET);
Shouldn't this be atomic_add? Otherwise the result of what you have here
is actually a non-atomic read/add/write.
Ian.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 2/5] evtchn: simplify port_is_valid()
2015-06-15 15:57 ` Ian Campbell
@ 2015-06-15 15:59 ` David Vrabel
2015-06-15 16:09 ` Ian Campbell
0 siblings, 1 reply; 27+ messages in thread
From: David Vrabel @ 2015-06-15 15:59 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel, Keir Fraser, Tim Deegan, Jan Beulich
On 15/06/15 16:57, Ian Campbell wrote:
> On Mon, 2015-06-15 at 16:48 +0100, David Vrabel wrote:
>> By keeping a count of the number of currently valid event channels,
>> port_is_valid() can be simplified.
>>
>> d->valid_evtchns can also be tested without holding d->event_lock which
>> will be useful later on.
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> ---
>> v2:
>> - Used unsigned int for d->valid_evtchns.
>> ---
>> xen/common/event_channel.c | 3 +++
>> xen/include/xen/event.h | 4 +---
>> xen/include/xen/sched.h | 5 +++--
>> 3 files changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
>> index 947880f..fd48646 100644
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -189,6 +189,8 @@ static int get_free_port(struct domain *d)
>> return -ENOMEM;
>> bucket_from_port(d, port) = chn;
>>
>> + write_atomic(&d->valid_evtchns, d->valid_evtchns + EVTCHNS_PER_BUCKET);
>
> Shouldn't this be atomic_add? Otherwise the result of what you have here
> is actually a non-atomic read/add/write.
This field is only updated while holding d->event_lock. Only reads are
unlocked.
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 2/5] evtchn: simplify port_is_valid()
2015-06-15 15:59 ` David Vrabel
@ 2015-06-15 16:09 ` Ian Campbell
0 siblings, 0 replies; 27+ messages in thread
From: Ian Campbell @ 2015-06-15 16:09 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Tim Deegan, Jan Beulich
On Mon, 2015-06-15 at 16:59 +0100, David Vrabel wrote:
> On 15/06/15 16:57, Ian Campbell wrote:
> > On Mon, 2015-06-15 at 16:48 +0100, David Vrabel wrote:
> >> By keeping a count of the number of currently valid event channels,
> >> port_is_valid() can be simplified.
> >>
> >> d->valid_evtchns can also be tested without holding d->event_lock which
> >> will be useful later on.
> >>
> >> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> >> ---
> >> v2:
> >> - Used unsigned int for d->valid_evtchns.
> >> ---
> >> xen/common/event_channel.c | 3 +++
> >> xen/include/xen/event.h | 4 +---
> >> xen/include/xen/sched.h | 5 +++--
> >> 3 files changed, 7 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> >> index 947880f..fd48646 100644
> >> --- a/xen/common/event_channel.c
> >> +++ b/xen/common/event_channel.c
> >> @@ -189,6 +189,8 @@ static int get_free_port(struct domain *d)
> >> return -ENOMEM;
> >> bucket_from_port(d, port) = chn;
> >>
> >> + write_atomic(&d->valid_evtchns, d->valid_evtchns + EVTCHNS_PER_BUCKET);
> >
> > Shouldn't this be atomic_add? Otherwise the result of what you have here
> > is actually a non-atomic read/add/write.
>
> This field is only updated while holding d->event_lock. Only reads are
> unlocked.
Oh good, thanks!
Ian.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events
2015-06-15 15:48 ` [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events David Vrabel
@ 2015-06-16 9:18 ` Jan Beulich
2015-06-16 9:34 ` David Vrabel
2015-06-16 15:19 ` David Vrabel
0 siblings, 2 replies; 27+ messages in thread
From: Jan Beulich @ 2015-06-16 9:18 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
>>> On 15.06.15 at 17:48, <david.vrabel@citrix.com> wrote:
> @@ -609,21 +662,18 @@ int evtchn_send(struct domain *ld, unsigned int lport)
> struct domain *rd;
> int rport, ret = 0;
>
> - spin_lock(&ld->event_lock);
> -
> - if ( unlikely(!port_is_valid(ld, lport)) )
> - {
> - spin_unlock(&ld->event_lock);
> + if ( unlikely(lport >= read_atomic(&ld->valid_evtchns)) )
> return -EINVAL;
> - }
I don't think you really want to open code part of port_is_valid()
(and avoid other parts of it) here? Or if really so, I think a comment
should be added to explain it.
> @@ -1163,11 +1213,15 @@ int alloc_unbound_xen_event_channel(
> if ( rc )
> goto out;
>
> + spin_lock(&chn->lock);
> +
> chn->state = ECS_UNBOUND;
> chn->xen_consumer = get_xen_consumer(notification_fn);
> chn->notify_vcpu_id = lvcpu;
> chn->u.unbound.remote_domid = remote_domid;
>
> + spin_unlock(&chn->lock);
> +
> out:
> spin_unlock(&ld->event_lock);
I don't see why you shouldn't be able to move up this unlock.
And then - no change to free_xen_event_channel()?
> @@ -1214,6 +1268,8 @@ void notify_via_xen_event_channel(struct domain *ld, int lport)
> lchn = evtchn_from_port(ld, lport);
> ASSERT(consumer_is_xen(lchn));
>
> + spin_lock(&lchn->lock);
> +
> if ( likely(lchn->state == ECS_INTERDOMAIN) )
> {
> rd = lchn->u.interdomain.remote_dom;
> @@ -1221,6 +1277,8 @@ void notify_via_xen_event_channel(struct domain *ld, int lport)
> evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
> }
>
> + spin_unlock(&lchn->lock);
> +
> spin_unlock(&ld->event_lock);
> }
Again I think the event lock can be dropped earlier now.
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 4/5] evtchn: remove the locking when unmasking an event channel
2015-06-15 15:48 ` [PATCHv2 4/5] evtchn: remove the locking when unmasking an event channel David Vrabel
@ 2015-06-16 9:19 ` Jan Beulich
2015-06-16 9:40 ` David Vrabel
0 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2015-06-16 9:19 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
>>> On 15.06.15 at 17:48, <david.vrabel@citrix.com> wrote:
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -977,9 +977,7 @@ int evtchn_unmask(unsigned int port)
> struct domain *d = current->domain;
> struct evtchn *evtchn;
>
> - ASSERT(spin_is_locked(&d->event_lock));
> -
> - if ( unlikely(!port_is_valid(d, port)) )
> + if ( port >= read_atomic(&d->valid_evtchns) )
> return -EINVAL;
Again this partial open coding looks wrong to me.
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes
2015-06-15 15:48 ` [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes David Vrabel
@ 2015-06-16 9:22 ` Jan Beulich
2015-06-16 11:14 ` Julien Grall
1 sibling, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2015-06-16 9:22 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
>>> On 15.06.15 at 17:48, <david.vrabel@citrix.com> wrote:
> The number of struct evtchn in a page must be a power of two. Under
> some workloads performance is improved slightly by padding struct
> evtchn to 64 bytes (a cache line), thus putting the per-channel locks
> into their own cache line.
>
> This does not decrease the number of struct evtchn's per-page.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
> I'm not sure we actually want to do this. I think it would be better
> to pack the struct evtchn and use vmap to turn the pages into a linear
> array for quicker lookup.
But then you'd end up with two locks on one cache line again. I.e.
I can see the possible benefit of making the tree a linear table, but
I don't see how that eliminates the desire for the change here.
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events
2015-06-16 9:18 ` Jan Beulich
@ 2015-06-16 9:34 ` David Vrabel
2015-06-16 9:51 ` Jan Beulich
2015-06-16 15:19 ` David Vrabel
1 sibling, 1 reply; 27+ messages in thread
From: David Vrabel @ 2015-06-16 9:34 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
On 16/06/15 10:18, Jan Beulich wrote:
>>>> On 15.06.15 at 17:48, <david.vrabel@citrix.com> wrote:
>> @@ -609,21 +662,18 @@ int evtchn_send(struct domain *ld, unsigned int lport)
>> struct domain *rd;
>> int rport, ret = 0;
>>
>> - spin_lock(&ld->event_lock);
>> -
>> - if ( unlikely(!port_is_valid(ld, lport)) )
>> - {
>> - spin_unlock(&ld->event_lock);
>> + if ( unlikely(lport >= read_atomic(&ld->valid_evtchns)) )
>> return -EINVAL;
>> - }
>
> I don't think you really want to open code part of port_is_valid()
> (and avoid other parts of it) here? Or if really so, I think a comment
> should be added to explain it.
The ld->valid_evtchns is the only field we can safely check without
ld->event_lock.
We do check the channel state and the code that set this state uses the
full port_is_valid() call. I'll add a comment.
> And then - no change to free_xen_event_channel()?
It looked alright to me, but now I see the clear of xen_consumer needs
to be done under the channel lock. Probably by moving it to
__evtchn_close().
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 4/5] evtchn: remove the locking when unmasking an event channel
2015-06-16 9:19 ` Jan Beulich
@ 2015-06-16 9:40 ` David Vrabel
0 siblings, 0 replies; 27+ messages in thread
From: David Vrabel @ 2015-06-16 9:40 UTC (permalink / raw)
To: Jan Beulich, David Vrabel
Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
On 16/06/15 10:19, Jan Beulich wrote:
>>>> On 15.06.15 at 17:48, <david.vrabel@citrix.com> wrote:
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -977,9 +977,7 @@ int evtchn_unmask(unsigned int port)
>> struct domain *d = current->domain;
>> struct evtchn *evtchn;
>>
>> - ASSERT(spin_is_locked(&d->event_lock));
>> -
>> - if ( unlikely(!port_is_valid(d, port)) )
>> + if ( port >= read_atomic(&d->valid_evtchns) )
>> return -EINVAL;
>
> Again this partial open coding looks wrong to me.
Again, d->valid_evtchns is the only field we can safely check without
d->event_lock.
I think this is fine because unmasking an unbound event channel is harmless.
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events
2015-06-16 9:34 ` David Vrabel
@ 2015-06-16 9:51 ` Jan Beulich
2015-06-16 9:57 ` David Vrabel
0 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2015-06-16 9:51 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
>>> On 16.06.15 at 11:34, <david.vrabel@citrix.com> wrote:
> On 16/06/15 10:18, Jan Beulich wrote:
>>>>> On 15.06.15 at 17:48, <david.vrabel@citrix.com> wrote:
>>> @@ -609,21 +662,18 @@ int evtchn_send(struct domain *ld, unsigned int lport)
>>> struct domain *rd;
>>> int rport, ret = 0;
>>>
>>> - spin_lock(&ld->event_lock);
>>> -
>>> - if ( unlikely(!port_is_valid(ld, lport)) )
>>> - {
>>> - spin_unlock(&ld->event_lock);
>>> + if ( unlikely(lport >= read_atomic(&ld->valid_evtchns)) )
>>> return -EINVAL;
>>> - }
>>
>> I don't think you really want to open code part of port_is_valid()
>> (and avoid other parts of it) here? Or if really so, I think a comment
>> should be added to explain it.
>
> The ld->valid_evtchns is the only field we can safely check without
> ld->event_lock.
>
> We do check the channel state and the code that set this state uses the
> full port_is_valid() call. I'll add a comment.
Hmm, port_is_valid() also checks d->max_evtchns and d->evtchn.
The latter is involved in evtchn_from_port(), so I can't see how
you checking the channel's state _afterwards_ can leverage that
whoever set this state did a full check.
Another question is whether with the ->valid_evtchns check the
->evtchn check is necessary at all anymore. (The check against
->max_evtchns isn't wrong with the lock not held, i.e. could only
end up being too strict, and hence the open coding would then
still be questionable.)
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events
2015-06-16 9:51 ` Jan Beulich
@ 2015-06-16 9:57 ` David Vrabel
0 siblings, 0 replies; 27+ messages in thread
From: David Vrabel @ 2015-06-16 9:57 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
On 16/06/15 10:51, Jan Beulich wrote:
>>>> On 16.06.15 at 11:34, <david.vrabel@citrix.com> wrote:
>> On 16/06/15 10:18, Jan Beulich wrote:
>>>>>> On 15.06.15 at 17:48, <david.vrabel@citrix.com> wrote:
>>>> @@ -609,21 +662,18 @@ int evtchn_send(struct domain *ld, unsigned int lport)
>>>> struct domain *rd;
>>>> int rport, ret = 0;
>>>>
>>>> - spin_lock(&ld->event_lock);
>>>> -
>>>> - if ( unlikely(!port_is_valid(ld, lport)) )
>>>> - {
>>>> - spin_unlock(&ld->event_lock);
>>>> + if ( unlikely(lport >= read_atomic(&ld->valid_evtchns)) )
>>>> return -EINVAL;
>>>> - }
>>>
>>> I don't think you really want to open code part of port_is_valid()
>>> (and avoid other parts of it) here? Or if really so, I think a comment
>>> should be added to explain it.
>>
>> The ld->valid_evtchns is the only field we can safely check without
>> ld->event_lock.
>>
>> We do check the channel state and the code that set this state uses the
>> full port_is_valid() call. I'll add a comment.
>
> Hmm, port_is_valid() also checks d->max_evtchns and d->evtchn.
> The latter is involved in evtchn_from_port(), so I can't see how
> you checking the channel's state _afterwards_ can leverage that
> whoever set this state did a full check.
>
> Another question is whether with the ->valid_evtchns check the
> ->evtchn check is necessary at all anymore. (The check against
> ->max_evtchns isn't wrong with the lock not held, i.e. could only
> end up being too strict, and hence the open coding would then
> still be questionable.)
Ok. I'll remove the d->evtchn check from port_is_valid() and use it.
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes
2015-06-15 15:48 ` [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes David Vrabel
2015-06-16 9:22 ` Jan Beulich
@ 2015-06-16 11:14 ` Julien Grall
2015-06-16 11:59 ` Jan Beulich
1 sibling, 1 reply; 27+ messages in thread
From: Julien Grall @ 2015-06-16 11:14 UTC (permalink / raw)
To: David Vrabel, xen-devel
Cc: Tim Deegan, Keir Fraser, Ian Campbell, Jan Beulich
Hi David,
On 15/06/2015 16:48, David Vrabel wrote:
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 44ea92d..a0ff9d2 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -129,7 +129,7 @@ struct evtchn
> #endif
> } ssid;
> #endif
> -};
> +} __attribute__((aligned(64)));
Why don't you use __cacheline_aligned?
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes
2015-06-16 11:14 ` Julien Grall
@ 2015-06-16 11:59 ` Jan Beulich
2015-06-16 12:57 ` Julien Grall
0 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2015-06-16 11:59 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Tim Deegan, Keir Fraser, David Vrabel, Ian Campbell
>>> On 16.06.15 at 13:14, <julien.grall@citrix.com> wrote:
> On 15/06/2015 16:48, David Vrabel wrote:
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index 44ea92d..a0ff9d2 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -129,7 +129,7 @@ struct evtchn
>> #endif
>> } ssid;
>> #endif
>> -};
>> +} __attribute__((aligned(64)));
>
> Why don't you use __cacheline_aligned?
That would double the size on x86, for little or no benefit.
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes
2015-06-16 11:59 ` Jan Beulich
@ 2015-06-16 12:57 ` Julien Grall
2015-06-16 13:13 ` David Vrabel
0 siblings, 1 reply; 27+ messages in thread
From: Julien Grall @ 2015-06-16 12:57 UTC (permalink / raw)
To: Jan Beulich, Julien Grall
Cc: xen-devel, Keir Fraser, Tim Deegan, David Vrabel, Ian Campbell
On 16/06/15 12:59, Jan Beulich wrote:
>>>> On 16.06.15 at 13:14, <julien.grall@citrix.com> wrote:
>> On 15/06/2015 16:48, David Vrabel wrote:
>>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>>> index 44ea92d..a0ff9d2 100644
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -129,7 +129,7 @@ struct evtchn
>>> #endif
>>> } ssid;
>>> #endif
>>> -};
>>> +} __attribute__((aligned(64)));
>>
>> Why don't you use __cacheline_aligned?
>
> That would double the size on x86, for little or no benefit.
Well, the cache line size is not necessarily 64 bytes on every
architecture. In the case of ARM, the cache line depends on the
processor version.
__cacheline_aligned is the only way to ensure that the cache line is not
shared on ARM.
AFAIU, the goal of this patch is to avoid sharing the cache line. If
not, the commit message is misleading because it claims that a cache
line is always 64 bytes...
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes
2015-06-16 12:57 ` Julien Grall
@ 2015-06-16 13:13 ` David Vrabel
2015-06-16 13:27 ` Julien Grall
0 siblings, 1 reply; 27+ messages in thread
From: David Vrabel @ 2015-06-16 13:13 UTC (permalink / raw)
To: Julien Grall, Jan Beulich, Julien Grall
Cc: xen-devel, Ian Campbell, Keir Fraser, David Vrabel, Tim Deegan
On 16/06/15 13:57, Julien Grall wrote:
> On 16/06/15 12:59, Jan Beulich wrote:
>>>>> On 16.06.15 at 13:14, <julien.grall@citrix.com> wrote:
>>> On 15/06/2015 16:48, David Vrabel wrote:
>>>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>>>> index 44ea92d..a0ff9d2 100644
>>>> --- a/xen/include/xen/sched.h
>>>> +++ b/xen/include/xen/sched.h
>>>> @@ -129,7 +129,7 @@ struct evtchn
>>>> #endif
>>>> } ssid;
>>>> #endif
>>>> -};
>>>> +} __attribute__((aligned(64)));
>>>
>>> Why don't you use __cacheline_aligned?
>>
>> That would double the size on x86, for little or no benefit.
>
> Well, the cache line size is not necessarily 64 bytes on every
> architecture. In the case of ARM, the cache line depends on the
> processor version.
>
> __cacheline_aligned is the only way to ensure that the cache line is not
> shared on ARM.
>
> AFAIU, the goal of this patch is to avoid sharing the cache line. If
> not, the commit message is misleading because it claims that a cache
> line is always 64 bytes...
We want to avoid sharing the cache line where we can do so for no
additional memory cost.
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes
2015-06-16 13:13 ` David Vrabel
@ 2015-06-16 13:27 ` Julien Grall
0 siblings, 0 replies; 27+ messages in thread
From: Julien Grall @ 2015-06-16 13:27 UTC (permalink / raw)
To: David Vrabel, Jan Beulich, Julien Grall
Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
On 16/06/15 14:13, David Vrabel wrote:
> On 16/06/15 13:57, Julien Grall wrote:
>> On 16/06/15 12:59, Jan Beulich wrote:
>>>>>> On 16.06.15 at 13:14, <julien.grall@citrix.com> wrote:
>>>> On 15/06/2015 16:48, David Vrabel wrote:
>>>>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>>>>> index 44ea92d..a0ff9d2 100644
>>>>> --- a/xen/include/xen/sched.h
>>>>> +++ b/xen/include/xen/sched.h
>>>>> @@ -129,7 +129,7 @@ struct evtchn
>>>>> #endif
>>>>> } ssid;
>>>>> #endif
>>>>> -};
>>>>> +} __attribute__((aligned(64)));
>>>>
>>>> Why don't you use __cacheline_aligned?
>>>
>>> That would double the size on x86, for little or no benefit.
>>
>> Well, the cache line size is not necessarily 64 bytes on every
>> architecture. In the case of ARM, the cache line depends on the
>> processor version.
>>
>> __cacheline_aligned is the only way to ensure that the cache line is not
>> shared on ARM.
>>
>> AFAIU, the goal of this patch is to avoid sharing the cache line. If
>> not, the commit message is misleading because it claims that a cache
>> line is always 64 bytes...
>
> We want to avoid sharing the cache line where we can do so for no
> additional memory cost.
I would expand the commit message to make clear that we may not share
the cache line. The "64 bytes (cache line)" is confusing and without any
background it can be interpreted wrongly.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events
2015-06-16 9:18 ` Jan Beulich
2015-06-16 9:34 ` David Vrabel
@ 2015-06-16 15:19 ` David Vrabel
2015-06-16 15:58 ` David Vrabel
2015-06-16 16:06 ` Jan Beulich
1 sibling, 2 replies; 27+ messages in thread
From: David Vrabel @ 2015-06-16 15:19 UTC (permalink / raw)
To: Jan Beulich, David Vrabel
Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
On 16/06/15 10:18, Jan Beulich wrote:
>>>> On 15.06.15 at 17:48, <david.vrabel@citrix.com> wrote:
>> @@ -1163,11 +1213,15 @@ int alloc_unbound_xen_event_channel(
>> if ( rc )
>> goto out;
>>
>> + spin_lock(&chn->lock);
>> +
>> chn->state = ECS_UNBOUND;
>> chn->xen_consumer = get_xen_consumer(notification_fn);
>> chn->notify_vcpu_id = lvcpu;
>> chn->u.unbound.remote_domid = remote_domid;
>>
>> + spin_unlock(&chn->lock);
>> +
>> out:
>> spin_unlock(&ld->event_lock);
>
> I don't see why you shouldn't be able to move up this unlock.
Because we need to (also) hold ld->event_lock while changing the state
from ECS_FREE or a concurrent get_free_port() will find this port again.
>> @@ -1221,6 +1277,8 @@ void notify_via_xen_event_channel(struct domain *ld, int lport)
>> evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
>> }
>>
>> + spin_unlock(&lchn->lock);
>> +
>> spin_unlock(&ld->event_lock);
>> }
>
> Again I think the event lock can be dropped earlier now.
Ditto.
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events
2015-06-16 15:19 ` David Vrabel
@ 2015-06-16 15:58 ` David Vrabel
2015-06-16 16:19 ` Jan Beulich
2015-06-16 16:06 ` Jan Beulich
1 sibling, 1 reply; 27+ messages in thread
From: David Vrabel @ 2015-06-16 15:58 UTC (permalink / raw)
To: David Vrabel, Jan Beulich
Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events
2015-06-16 15:19 ` David Vrabel
2015-06-16 15:58 ` David Vrabel
@ 2015-06-16 16:06 ` Jan Beulich
1 sibling, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2015-06-16 16:06 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
>>> On 16.06.15 at 17:19, <david.vrabel@citrix.com> wrote:
> On 16/06/15 10:18, Jan Beulich wrote:
>>>>> On 15.06.15 at 17:48, <david.vrabel@citrix.com> wrote:
>>> @@ -1163,11 +1213,15 @@ int alloc_unbound_xen_event_channel(
>>> if ( rc )
>>> goto out;
>>>
>>> + spin_lock(&chn->lock);
>>> +
>>> chn->state = ECS_UNBOUND;
>>> chn->xen_consumer = get_xen_consumer(notification_fn);
>>> chn->notify_vcpu_id = lvcpu;
>>> chn->u.unbound.remote_domid = remote_domid;
>>>
>>> + spin_unlock(&chn->lock);
>>> +
>>> out:
>>> spin_unlock(&ld->event_lock);
>>
>> I don't see why you shouldn't be able to move up this unlock.
>
> Because we need to (also) hold ld->event_lock while changing the state
> from ECS_FREE or a concurrent get_free_port() will find this port again.
I buy this one (and moving the unlock up after the state adjustment
is unlikely to be worth it), but ...
>>> @@ -1221,6 +1277,8 @@ void notify_via_xen_event_channel(struct domain *ld, int lport)
>>> evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
>>> }
>>>
>>> + spin_unlock(&lchn->lock);
>>> +
>>> spin_unlock(&ld->event_lock);
>>> }
>>
>> Again I think the event lock can be dropped earlier now.
>
> Ditto.
... there's no state change involved here.
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events
2015-06-16 15:58 ` David Vrabel
@ 2015-06-16 16:19 ` Jan Beulich
2015-06-16 16:39 ` David Vrabel
0 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2015-06-16 16:19 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
>>> On 16.06.15 at 17:58, <david.vrabel@citrix.com> wrote:
> On 16/06/15 16:19, David Vrabel wrote:
>>>> @@ -1221,6 +1277,8 @@ void notify_via_xen_event_channel(struct domain *ld,
> int lport)
>>>> evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
>>>> }
>>>>
>>>> + spin_unlock(&lchn->lock);
>>>> +
>>>> spin_unlock(&ld->event_lock);
>>>> }
>>>
>>> Again I think the event lock can be dropped earlier now.
>>
>> Ditto.
>
> Uh, no. This is notify. I've kept the locking like this because of the
> ld->is_dying check. I think we need the ld->event_lock in case
> d->is_dying is set and evtchn_destroy(ld) is called.
Right, but if evtchn_destroy() was a concern, then this wouldn't
apply just here, but also in the sending path you are relaxing.
Afaict due to the channel lock being taken in __evtchn_close()
you can drop the even lock here as the latest after you acquired
the channel one (I haven't been able to convince myself yet that
dropping it even before that would be okay).
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events
2015-06-16 16:19 ` Jan Beulich
@ 2015-06-16 16:39 ` David Vrabel
2015-06-17 7:05 ` Jan Beulich
0 siblings, 1 reply; 27+ messages in thread
From: David Vrabel @ 2015-06-16 16:39 UTC (permalink / raw)
To: Jan Beulich, David Vrabel
Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
On 16/06/15 17:19, Jan Beulich wrote:
>>>> On 16.06.15 at 17:58, <david.vrabel@citrix.com> wrote:
>> On 16/06/15 16:19, David Vrabel wrote:
>>>>> @@ -1221,6 +1277,8 @@ void notify_via_xen_event_channel(struct domain *ld,
>> int lport)
>>>>> evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
>>>>> }
>>>>>
>>>>> + spin_unlock(&lchn->lock);
>>>>> +
>>>>> spin_unlock(&ld->event_lock);
>>>>> }
>>>>
>>>> Again I think the event lock can be dropped earlier now.
>>>
>>> Ditto.
>>
>> Uh, no. This is notify. I've kept the locking like this because of the
>> ld->is_dying check. I think we need the ld->event_lock in case
>> d->is_dying is set and evtchn_destroy(ld) is called.
>
> Right, but if evtchn_destroy() was a concern, then this wouldn't
> apply just here, but also in the sending path you are relaxing.
> Afaict due to the channel lock being taken in __evtchn_close()
> you can drop the even lock here as the latest after you acquired
> the channel one (I haven't been able to convince myself yet that
> dropping it even before that would be okay).
But in the evtchn_send() case, we're in a hypercall so we know
ld->is_dying is false and thus cannot be racing with evtchn_destroy(ld).
It would be good to remove event_lock from notify_xen_event_channel() as
well since this is heavily used for ioreqs and vm events. Let me have a
more careful look.
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events
2015-06-16 16:39 ` David Vrabel
@ 2015-06-17 7:05 ` Jan Beulich
0 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2015-06-17 7:05 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
>>> On 16.06.15 at 18:39, <david.vrabel@citrix.com> wrote:
> On 16/06/15 17:19, Jan Beulich wrote:
>>>>> On 16.06.15 at 17:58, <david.vrabel@citrix.com> wrote:
>>> On 16/06/15 16:19, David Vrabel wrote:
>>>>>> @@ -1221,6 +1277,8 @@ void notify_via_xen_event_channel(struct domain *ld,
>>> int lport)
>>>>>> evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
>>>>>> }
>>>>>>
>>>>>> + spin_unlock(&lchn->lock);
>>>>>> +
>>>>>> spin_unlock(&ld->event_lock);
>>>>>> }
>>>>>
>>>>> Again I think the event lock can be dropped earlier now.
>>>>
>>>> Ditto.
>>>
>>> Uh, no. This is notify. I've kept the locking like this because of the
>>> ld->is_dying check. I think we need the ld->event_lock in case
>>> d->is_dying is set and evtchn_destroy(ld) is called.
>>
>> Right, but if evtchn_destroy() was a concern, then this wouldn't
>> apply just here, but also in the sending path you are relaxing.
>> Afaict due to the channel lock being taken in __evtchn_close()
>> you can drop the even lock here as the latest after you acquired
>> the channel one (I haven't been able to convince myself yet that
>> dropping it even before that would be okay).
>
> But in the evtchn_send() case, we're in a hypercall so we know
> ld->is_dying is false and thus cannot be racing with evtchn_destroy(ld).
That's only the common code path; there's an evtchn_send() call
from __domain_finalise_shutdown() which afaict doesn't make such
guarantees (in particular there clearly are d != current->domain
cases here).
And then - how is being in a hypercall a guard against is_dying not
getting set?
> It would be good to remove event_lock from notify_xen_event_channel() as
> well since this is heavily used for ioreqs and vm events. Let me have a
> more careful look.
Indeed, thanks.
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2015-06-17 7:05 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-15 15:48 [PATCHv2 0/5] evtchn: Improve scalebility David Vrabel
2015-06-15 15:48 ` [PATCHv2 1/5] evtchn: factor out freeing an event channel David Vrabel
2015-06-15 15:48 ` [PATCHv2 2/5] evtchn: simplify port_is_valid() David Vrabel
2015-06-15 15:57 ` Ian Campbell
2015-06-15 15:59 ` David Vrabel
2015-06-15 16:09 ` Ian Campbell
2015-06-15 15:48 ` [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events David Vrabel
2015-06-16 9:18 ` Jan Beulich
2015-06-16 9:34 ` David Vrabel
2015-06-16 9:51 ` Jan Beulich
2015-06-16 9:57 ` David Vrabel
2015-06-16 15:19 ` David Vrabel
2015-06-16 15:58 ` David Vrabel
2015-06-16 16:19 ` Jan Beulich
2015-06-16 16:39 ` David Vrabel
2015-06-17 7:05 ` Jan Beulich
2015-06-16 16:06 ` Jan Beulich
2015-06-15 15:48 ` [PATCHv2 4/5] evtchn: remove the locking when unmasking an event channel David Vrabel
2015-06-16 9:19 ` Jan Beulich
2015-06-16 9:40 ` David Vrabel
2015-06-15 15:48 ` [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes David Vrabel
2015-06-16 9:22 ` Jan Beulich
2015-06-16 11:14 ` Julien Grall
2015-06-16 11:59 ` Jan Beulich
2015-06-16 12:57 ` Julien Grall
2015-06-16 13:13 ` David Vrabel
2015-06-16 13:27 ` Julien Grall
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.