Re: [Xen-devel] [PATCH v2 2/7] libxc: Plumb Xen with vNUMA topology for domain.

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On mer, 2013-11-13 at 22:26 -0500, Elena Ufimtseva wrote:
> diff --git a/tools/libxc/xc_dom.h b/tools/libxc/xc_dom.h
> @@ -114,6 +114,15 @@ struct xc_dom_image {
>      struct xc_dom_phys *phys_pages;
>      int realmodearea_log;
>  
> +    /*
> +     * vNUMA topology and memory allocation structure.
> +     * Defines the way to allocate memory on per NUMA
> +     * physical nodes that is defined by vnode_numamap.
> +     */
> +    uint32_t nr_vnodes;
> +    uint64_t *vnuma_memszs;
> +    unsigned int *vnode_numamap;
> +
>
This vnode_numamap is, basically vnode-to-pnode mapping, isn't it? If
yes, considering you have vcpu_to_vnode already, what about actually
calling it vnode_to_pnode (or something similar)?

Here in Xen (either hypervisor or toolstack), hinting to 'map' and
'mapping', especially when there is memory involved, tend to have a
completely different meaning...

> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
>  
> +/* Plumbs Xen with vNUMA topology */
> +int xc_domain_setvnodes(xc_interface *xch,
> +                        uint32_t domid,
> +                        uint16_t nr_vnodes,
> +                        uint16_t nr_vcpus,
> +                        vmemrange_t *vmemrange,
> +                        unsigned int *vdistance,
> +                        unsigned int *vcpu_to_vnode,
> +                        unsigned int *vnode_numamap)
> +{
> +    int rc;
> +    DECLARE_DOMCTL;
> +    DECLARE_HYPERCALL_BOUNCE(vmemrange, sizeof(*vmemrange) * nr_vnodes,
> +                                    XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
>
These all should be XC_HYPERCALL_BUFFER_BOUNCE_IN, I think.

You don't really do any copy_to_guest() in 1/7 for any of these
parameters, and, at the same time, you really don't expect, here, to get
something back from the hypervisor in them, do you?

> +    DECLARE_HYPERCALL_BOUNCE(vdistance, sizeof(*vdistance) *
> +                                    nr_vnodes * nr_vnodes,
> +                                    XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
> +    DECLARE_HYPERCALL_BOUNCE(vcpu_to_vnode, sizeof(*vcpu_to_vnode) * 
> nr_vcpus,
> +                                    XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
> +    DECLARE_HYPERCALL_BOUNCE(vnode_numamap, sizeof(*vnode_numamap) *
> +                                    nr_vnodes,
> +                                    XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
> +    if ( nr_vnodes == 0 )
> +        return -EINVAL;
> +
>
Oh, how funny, we were just talking about this with Konrad the other
day. :-)

So, my impression is that, in libxc, we return 0 on success and, in case
of error, we return -1 and set errno. So, whenever it is another xc call
that fails, you can rely on it having set errno properly and just return
-1. If it's you, like in this case, you should be the one setting errno
(and returning -1).

Again, this is what I've always done and what I find the most common in
libxc. Perhaps we need a word from one of the maintainer to enlighten us
about what's the best practise here.

If I'm right, pretty much all the error handling should be changed in
this patch, and I'll avoid pointing at all the occurrences here.

> +    if ( vdistance == NULL || vcpu_to_vnode == NULL ||
> +         vmemrange == NULL || vnode_numamap == NULL ) {
>
Mmm... didn't we say in the previous patch (1/7, the one implementing
this DOMCTL in xen) that it is fine for vnode_numamap to be NULL? If we
error out like this here, how will ever Xen see it being NULL?

> +        PERROR("Incorrect parameters for XEN_DOMCTL_setvnumainfo.\n");
> +        return -EINVAL;
> +    }
> +    
> +    rc = -EFAULT;

> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index 8cf3f3b..451660e 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1108,6 +1108,15 @@ int xc_domain_set_memmap_limit(xc_interface *xch,
>                                 uint32_t domid,
>                                 unsigned long map_limitkb);
>  
> +int xc_domain_setvnodes(xc_interface *xch,
> +                        uint32_t domid,
> +                        uint16_t nr_vnodes,
> +                        uint16_t nr_vcpus,
> +                        vmemrange_t *vmemrange,
> +                        unsigned int *vdistance,
> +                        unsigned int *vcpu_to_vnode,
> +                        unsigned int *vnode_numamap);
> +
>
This is a minor thing, but I wonder whether xc_domain_setvnuma()
wouldn't make a better name for this. Thoughts?

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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