Re: [Xen-devel] [PATCH v5 7/8] libxl/libxc: Move libxl_get_numainfo()'s hypercall buffer management to libxc

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 03/20/2015 09:49 AM, Konrad Rzeszutek Wilk wrote:
> On Thu, Mar 19, 2015 at 05:54:03PM -0400, Boris Ostrovsky wrote:
> > xc_numainfo() is not expected to be used on a hot path and therefore
> > hypercall buffer management can be pushed into libxc. This will simplify
> > life for callers.
> >
> > Also update error logging macros.
> >
> > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> > ---
> > Changes in v5:
> > * Adjust to new interface (as described in changelog for patch 4/8)
> >
> >   tools/libxc/include/xenctrl.h     |    4 ++-
> >   tools/libxc/xc_misc.c             |   41 ++++++++++++++++++++++++-----
> >   tools/libxl/libxl.c               |   52 ++++++++++++-------------------------
> >   tools/python/xen/lowlevel/xc/xc.c |   38 ++++++++++-----------------
> >   4 files changed, 68 insertions(+), 67 deletions(-)
> >
> > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> > index 14d22ce..54540e7 100644
> > --- a/tools/libxc/include/xenctrl.h
> > +++ b/tools/libxc/include/xenctrl.h
> > @@ -1228,6 +1228,7 @@ int xc_send_debug_keys(xc_interface *xch, char *keys);
> >   typedef xen_sysctl_physinfo_t xc_physinfo_t;
> >   typedef xen_sysctl_cputopo_t xc_cputopo_t;
> >   typedef xen_sysctl_numainfo_t xc_numainfo_t;
> > +typedef xen_sysctl_meminfo_t xc_meminfo_t;
> >   
> >   typedef uint32_t xc_cpu_to_node_t;
> >   typedef uint32_t xc_cpu_to_socket_t;
> > @@ -1239,7 +1240,8 @@ typedef uint32_t xc_node_to_node_dist_t;
> >   int xc_physinfo(xc_interface *xch, xc_physinfo_t *info);
> >   int xc_cputopoinfo(xc_interface *xch, unsigned *max_cpus,
> >                      xc_cputopo_t *cputopo);
> > -int xc_numainfo(xc_interface *xch, xc_numainfo_t *info);
> > +int xc_numainfo(xc_interface *xch, unsigned *max_nodes,
> > +                xc_meminfo_t *meminfo, uint32_t *distance);
> >   
> >   int xc_sched_id(xc_interface *xch,
> >                   int *sched_id);
> > diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
> > index 411128e..607ae61 100644
> > --- a/tools/libxc/xc_misc.c
> > +++ b/tools/libxc/xc_misc.c
> > @@ -209,22 +209,49 @@ out:
> >       return ret;
> >   }
> >   
> > -int xc_numainfo(xc_interface *xch,
> > -                xc_numainfo_t *put_info)
> > +int xc_numainfo(xc_interface *xch, unsigned *max_nodes,
> > +                xc_meminfo_t *meminfo, uint32_t *distance)
> >   {
> >       int ret;
> >       DECLARE_SYSCTL;
> > +    DECLARE_HYPERCALL_BOUNCE(meminfo, *max_nodes * sizeof(*meminfo),
> > +                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
> > +    DECLARE_HYPERCALL_BOUNCE(distance,
> > +                             *max_nodes * *max_nodes * sizeof(*distance),
> > +                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
> >   
> > -    sysctl.cmd = XEN_SYSCTL_numainfo;
> > +    if (meminfo && distance) {
> The syntax in xc_misc.c looks mostly to be of the
> 'if ( meminfo && distance )
>   {
> ' type.
>
> > +        if ((ret = xc_hypercall_bounce_pre(xch, meminfo)))
> > +            goto out;
> > +        if ((ret = xc_hypercall_bounce_pre(xch, distance)))
> > +            goto out;
> >   
> > -    memcpy(&sysctl.u.numainfo, put_info, sizeof(*put_info));
> > +        sysctl.u.numainfo.num_nodes = *max_nodes;
> > +        set_xen_guest_handle(sysctl.u.numainfo.meminfo, meminfo);
> > +        set_xen_guest_handle(sysctl.u.numainfo.distance, distance);
> > +    } else if (meminfo || distance) {
> > +        errno = EINVAL;
> > +        return -1;
> > +    } else {
> > +        set_xen_guest_handle(sysctl.u.numainfo.meminfo,
> > +                             HYPERCALL_BUFFER_NULL);
> > +        set_xen_guest_handle(sysctl.u.numainfo.distance,
> > +                             HYPERCALL_BUFFER_NULL);
> > +    }
> >   
> > +    sysctl.cmd = XEN_SYSCTL_numainfo;
> >       if ((ret = do_sysctl(xch, &sysctl)) != 0)
> > -        return ret;
> > +        goto out;
> >   
> > -    memcpy(put_info, &sysctl.u.numainfo, sizeof(*put_info));
> > +    *max_nodes = sysctl.u.numainfo.num_nodes;
> >   
> > -    return 0;
> > +out:
> > +    if (meminfo && distance) {
> > +        xc_hypercall_bounce_post(xch, meminfo);
> > +        xc_hypercall_bounce_post(xch, distance);
> > +    }
> > +
> > +    return ret;
> >   }
> >   
> >   
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 2b7d19c..ee97a54 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -5094,62 +5094,44 @@ libxl_cputopology *libxl_get_cpu_topology(libxl_ctx 
> > *ctx, int *nb_cpu_out)
> >   libxl_numainfo *libxl_get_numainfo(libxl_ctx *ctx, int *nr)
> >   {
> >       GC_INIT(ctx);
> > -    xc_numainfo_t ninfo;
> > -    DECLARE_HYPERCALL_BUFFER(xen_sysctl_meminfo_t, meminfo);
> > -    DECLARE_HYPERCALL_BUFFER(uint32_t, distance);
> > +    xc_meminfo_t *meminfo;
> > +    uint32_t *distance;
> >       libxl_numainfo *ret = NULL;
> >       int i, j;
> > +    unsigned num_nodes;
> >   
> > -    set_xen_guest_handle(ninfo.meminfo, HYPERCALL_BUFFER_NULL);
> > -    set_xen_guest_handle(ninfo.distance, HYPERCALL_BUFFER_NULL);
> > -    if ( xc_numainfo(ctx->xch, &ninfo) != 0)
> > -    {
> > -        LIBXL__LOG(ctx, XTL_ERROR, "Unable to determine number of NODES");
> > -        ret = NULL;
> > +    if (xc_numainfo(ctx->xch, &num_nodes, NULL, NULL)) {
> I think for this one you need 'if ( xc_numa.. )
>   {'
I actually think it's the other way around: when I made the first change 
in 4/8 I added spaces, which this file's style generally doesn't use.
Which, conveniently, is different from the style used by xc_misc.c

And I am quite confused about curly bracket positions. They are all over 
the place (pun intended).
-boris



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