Re: [Xen-devel] [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes
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.gr...@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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes
On 16.06.15 at 13:14, julien.gr...@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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes
On 16/06/15 12:59, Jan Beulich wrote: On 16.06.15 at 13:14, julien.gr...@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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes
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.gr...@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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes
On 15.06.15 at 17:48, david.vra...@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.vra...@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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes
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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCHv2 5/5] evtchn: pad struct evtchn to 64 bytes
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.vra...@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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
