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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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@oracle.com>
>> ---
>> 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