[PATCH] tools/libxc: Avoid hypercalls for cacheflush on x86

[PATCH] tools/libxc: Avoid hypercalls for cacheflush on x86

[Andrew Cooper]

XEN_DOMCTL_cacheflush hypercalls are -ENOSYS on x86.  As domain
creation/migration is already very hypercall-heavy, avoid making extra
redundant hypercalls

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

---

Discovered when running valgrind against XenServer trunk, which is now 4.4
based.
---
 tools/libxc/xc_domain.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 369c3f3..b9cc1db 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -51,12 +51,18 @@ int xc_domain_create(xc_interface *xch,
 int xc_domain_cacheflush(xc_interface *xch, uint32_t domid,
                          xen_pfn_t start_pfn, xen_pfn_t nr_pfns)
 {
+#if defined (__i386__) || defined (__x86_64__)
+    /* This is not implemented or needed for x86.  Avoid repeatedly going to
+     * Xen to find -ENOSYS. */
+    return -ENOSYS;
+#else
     DECLARE_DOMCTL;
     domctl.cmd = XEN_DOMCTL_cacheflush;
     domctl.domain = (domid_t)domid;
     domctl.u.cacheflush.start_pfn = start_pfn;
     domctl.u.cacheflush.nr_pfns = nr_pfns;
     return do_domctl(xch, &domctl);
+#endif
 }
 
 int xc_domain_pause(xc_interface *xch,
-- 
1.7.10.4

Re: [PATCH] tools/libxc: Avoid hypercalls for cacheflush on x86

[David Vrabel]

On 03/03/14 14:37, Andrew Cooper wrote:
> XEN_DOMCTL_cacheflush hypercalls are -ENOSYS on x86.  As domain
> creation/migration is already very hypercall-heavy, avoid making extra
> redundant hypercalls
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> 
> ---
> 
> Discovered when running valgrind against XenServer trunk, which is now 4.4
> based.
> ---
>  tools/libxc/xc_domain.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index 369c3f3..b9cc1db 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -51,12 +51,18 @@ int xc_domain_create(xc_interface *xch,
>  int xc_domain_cacheflush(xc_interface *xch, uint32_t domid,
>                           xen_pfn_t start_pfn, xen_pfn_t nr_pfns)
>  {
> +#if defined (__i386__) || defined (__x86_64__)
> +    /* This is not implemented or needed for x86.  Avoid repeatedly going to
> +     * Xen to find -ENOSYS. */
> +    return -ENOSYS;
> +#else
>      DECLARE_DOMCTL;
>      domctl.cmd = XEN_DOMCTL_cacheflush;
>      domctl.domain = (domid_t)domid;
>      domctl.u.cacheflush.start_pfn = start_pfn;
>      domctl.u.cacheflush.nr_pfns = nr_pfns;
>      return do_domctl(xch, &domctl);
> +#endif

Rather than icky #ifdef's I suggest caching the result of any -ENOSYS
return value.

David

Re: [PATCH] tools/libxc: Avoid hypercalls for cacheflush on x86

[Ian Jackson]

Andrew Cooper writes ("[PATCH] tools/libxc: Avoid hypercalls for cacheflush on x86"):
> XEN_DOMCTL_cacheflush hypercalls are -ENOSYS on x86.  As domain
> creation/migration is already very hypercall-heavy, avoid making extra
> redundant hypercalls

Surely this should be done by moving the relevant code to an
arm-specific file, not by #ifdeffery.

Thanks,
Ian.

Re: [PATCH] tools/libxc: Avoid hypercalls for cacheflush on x86

[Ian Campbell]

On Mon, 2014-03-03 at 16:22 +0000, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH] tools/libxc: Avoid hypercalls for cacheflush on x86"):
> > XEN_DOMCTL_cacheflush hypercalls are -ENOSYS on x86.  As domain
> > creation/migration is already very hypercall-heavy, avoid making extra
> > redundant hypercalls
> 
> Surely this should be done by moving the relevant code to an
> arm-specific file, not by #ifdeffery.

I think it is very debatable that sort-circuiting a single ENOSYS
hypercall in the tools is even worth it at all. "very hypercall heavy"
doesn't cut it as a rationale IMHO. Unless an actual demonstrable
improvement is seen, perhaps over a larger series removing a large
number of so called redundant calls, it's just an additional source of
confusion (due to gating at multiple levels) as far as I'm concerned.

Ian.

Re: [PATCH] tools/libxc: Avoid hypercalls for cacheflush on x86

[Ian Campbell]

On Mon, 2014-03-03 at 16:22 +0000, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH] tools/libxc: Avoid hypercalls for cacheflush on x86"):
> > XEN_DOMCTL_cacheflush hypercalls are -ENOSYS on x86.  As domain
> > creation/migration is already very hypercall-heavy, avoid making extra
> > redundant hypercalls
> 
> Surely this should be done by moving the relevant code to an
> arm-specific file, not by #ifdeffery.

I think it is very debatable that sort-circuiting a single ENOSYS
hypercall in the tools is even worth it at all. "very hypercall heavy"
doesn't cut it as a rationale IMHO. Unless an actual demonstrable
improvement is seen, perhaps over a larger series removing a large
number of so called redundant calls, it's just an additional source of
confusion (due to gating at multiple levels) as far as I'm concerned.

Ian.

Re: [PATCH] tools/libxc: Avoid hypercalls for cacheflush on x86

[Jan Beulich]

>>> On 03.03.14 at 15:37, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -51,12 +51,18 @@ int xc_domain_create(xc_interface *xch,
>  int xc_domain_cacheflush(xc_interface *xch, uint32_t domid,
>                           xen_pfn_t start_pfn, xen_pfn_t nr_pfns)
>  {
> +#if defined (__i386__) || defined (__x86_64__)
> +    /* This is not implemented or needed for x86.  Avoid repeatedly going to
> +     * Xen to find -ENOSYS. */
> +    return -ENOSYS;

Leaving aside the question of whether this is worthwhile in the first
place, this is wrong: You want to return -1 here, setting errno to
ENOSYS.

Jan

Re: [PATCH] tools/libxc: Avoid hypercalls for cacheflush on x86

[Andrew Cooper]

On 04/03/14 01:14, Ian Campbell wrote:
> On Mon, 2014-03-03 at 16:22 +0000, Ian Jackson wrote:
>> Andrew Cooper writes ("[PATCH] tools/libxc: Avoid hypercalls for cacheflush on x86"):
>>> XEN_DOMCTL_cacheflush hypercalls are -ENOSYS on x86.  As domain
>>> creation/migration is already very hypercall-heavy, avoid making extra
>>> redundant hypercalls
>> Surely this should be done by moving the relevant code to an
>> arm-specific file, not by #ifdeffery.
> I think it is very debatable that sort-circuiting a single ENOSYS
> hypercall in the tools is even worth it at all. "very hypercall heavy"
> doesn't cut it as a rationale IMHO. Unless an actual demonstrable
> improvement is seen, perhaps over a larger series removing a large
> number of so called redundant calls, it's just an additional source of
> confusion (due to gating at multiple levels) as far as I'm concerned.
>
> Ian.
>

For booting a single guest, noone will notice.  Booting 10 or more at a
time however is very different.

XenServer, along with all other virtualisation platforms live in a world
where "Net time to boot $N guests" (where N is measured in hundreds) is
an important quantity, and needless overhead from things like this cause
dom0 needless load that it would rather do without.

I should say that I have not done any performance metrics on this - as
noted in the patch I stumbled upon it with valgrind.

~Andrew

Re: [PATCH] tools/libxc: Avoid hypercalls for cacheflush on x86

[Ian Campbell]

On Tue, 2014-03-04 at 11:22 +0000, Andrew Cooper wrote:
> On 04/03/14 01:14, Ian Campbell wrote:
> > On Mon, 2014-03-03 at 16:22 +0000, Ian Jackson wrote:
> >> Andrew Cooper writes ("[PATCH] tools/libxc: Avoid hypercalls for cacheflush on x86"):
> >>> XEN_DOMCTL_cacheflush hypercalls are -ENOSYS on x86.  As domain
> >>> creation/migration is already very hypercall-heavy, avoid making extra
> >>> redundant hypercalls
> >> Surely this should be done by moving the relevant code to an
> >> arm-specific file, not by #ifdeffery.
> > I think it is very debatable that sort-circuiting a single ENOSYS
> > hypercall in the tools is even worth it at all. "very hypercall heavy"
> > doesn't cut it as a rationale IMHO. Unless an actual demonstrable
> > improvement is seen, perhaps over a larger series removing a large
> > number of so called redundant calls, it's just an additional source of
> > confusion (due to gating at multiple levels) as far as I'm concerned.
> >
> > Ian.
> >
> 
> For booting a single guest, noone will notice.  Booting 10 or more at a
> time however is very different.
> 
> XenServer, along with all other virtualisation platforms live in a world
> where "Net time to boot $N guests" (where N is measured in hundreds) is
> an important quantity, and needless overhead from things like this cause
> dom0 needless load that it would rather do without.

That's fine.

> I should say that I have not done any performance metrics on this - as
> noted in the patch I stumbled upon it with valgrind.

And in the absence of such performance metrics I see no reason to apply
this patch, which was what my initial reply said.

Ian.