Re: [PATCH v2 1/7] xen: vNUMA support for guests.

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 8908 bytes --]

On mer, 2013-11-13 at 22:26 -0500, Elena Ufimtseva wrote:
> Defines interface, structures and hypercalls for guests that wish
> to retreive vNUMA topology from Xen.
>
Well, not only "for guests that wish to retrieve" the vNUMA topology,
also for toolstacks that wish to configure it.

> Two subop hypercalls introduced by patch:
> XEN_DOMCTL_setvnumainfo to define vNUMA domain topology per domain
> and XENMEM_get_vnuma_info to retreive that topology by guest.
> 
> Signed-off-by: Elena Ufimtseva <ufimtseva@gmail.com>
> ---

> diff --git a/xen/common/domain.c b/xen/common/domain.c
> @@ -539,6 +539,7 @@ int domain_kill(struct domain *d)
>          tmem_destroy(d->tmem);
>          domain_set_outstanding_pages(d, 0);
>          d->tmem = NULL;
> +        domain_vnuma_destroy(&d->vnuma);
>          /* fallthrough */
>      case DOMDYING_dying:
>          rc = domain_relinquish_resources(d);
> @@ -1297,6 +1298,15 @@ int continue_hypercall_on_cpu(
>      return 0;
>  }
>  
> +void domain_vnuma_destroy(struct domain_vnuma_info *v)
> +{
I think vnuma_destroy() is ok, as it is tmem_destroy(),
evtchn_destory(), etc.

> +    v->nr_vnodes = 0;
> +    xfree(v->vmemrange);
> +    xfree(v->vcpu_to_vnode);
> +    xfree(v->vdistance);
> +    xfree(v->vnode_numamap);
> +}
> +

> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> @@ -871,6 +872,87 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>      }
>      break;
>  
> +    case XEN_DOMCTL_setvnumainfo:
> +    {
> +        unsigned int i = 0, dist_size;
> +        uint nr_vnodes;
> +        ret = -EFAULT;
> +
> +        /* Already set? */
> +        if ( d->vnuma.nr_vnodes > 0 )
> +            return 0;
> +
> +        nr_vnodes = op->u.vnuma.nr_vnodes;
> +        
> +        if ( nr_vnodes == 0 )
> +            return ret;
> +        if ( nr_vnodes * nr_vnodes > UINT_MAX )
> +            return ret;
> +
Mmm... I think this three 'return's ought all to be 'break's, or you'll
never get to execute the common exit path from do_domctl().

> +        /*
> +         * If null, vnode_numamap will set default to
> +         * point to allocation mechanism to dont use
> +         * per physical node allocation or this is for
> +         * cases when there is no physical NUMA.
> +         */
> +        if ( guest_handle_is_null(op->u.vnuma.vdistance) ||
> +             guest_handle_is_null(op->u.vnuma.vmemrange) ||
> +             guest_handle_is_null(op->u.vnuma.vcpu_to_vnode) )
> +            goto err_dom;
> +        
Sorry, I'm not sure I fully understand the comment: you're saying that
it is ok for vnuma_nodemap to be NULL, right?

> +        dist_size = nr_vnodes * nr_vnodes;
> +        
> +        d->vnuma.vdistance = xmalloc_array(unsigned int, dist_size);
> +        d->vnuma.vmemrange = xmalloc_array(vmemrange_t, nr_vnodes);
> +        d->vnuma.vcpu_to_vnode = xmalloc_array(unsigned int, d->max_vcpus);
> +        d->vnuma.vnode_numamap = xmalloc_array(unsigned int, nr_vnodes);
> +
> +        if ( d->vnuma.vdistance == NULL ||
> +             d->vnuma.vmemrange == NULL ||
> +             d->vnuma.vcpu_to_vnode == NULL ||
> +             d->vnuma.vnode_numamap == NULL )
> +        {
> +            ret = -ENOMEM;
> +            goto err_dom;
> +        }
>
Well, in general, things like just 'err' or 'out' are fine as labels.
However, in this case, since we're inside quite a big switch{}, a bit
more of context could be helpful. What about killing the '_dom' part
(which does not really say much) and putting a meaningful prefix? Also,
it's not like the code you're jumping at is executed only on error, so
even the 'err_' part looks incorrect.

Personally, I'd go for something like 'setvnumainfo_out' (see
XEN_DOMCTL_getdomaininfo for reference).

> +        if ( unlikely(copy_from_guest(d->vnuma.vdistance,
> +                                    op->u.vnuma.vdistance,
> +                                    dist_size)) )
> +            goto err_dom;
> +        if ( unlikely(copy_from_guest(d->vnuma.vmemrange,
> +                                    op->u.vnuma.vmemrange,
> +                                    nr_vnodes)) )
> +            goto err_dom;
> +        if ( unlikely(copy_from_guest(d->vnuma.vcpu_to_vnode,
> +                                    op->u.vnuma.vcpu_to_vnode,
> +                                    d->max_vcpus)) )
> +            goto err_dom;
> +        if ( !guest_handle_is_null(op->u.vnuma.vnode_numamap) )
> +        {
> +            if ( unlikely(copy_from_guest(d->vnuma.vnode_numamap,
> +                                        op->u.vnuma.vnode_numamap,
> +                                        nr_vnodes)) )
> +                goto err_dom;
> +        }
> +        else
> +            for ( i = 0; i < nr_vnodes; i++ )
> +                d->vnuma.vnode_numamap[i] = NUMA_NO_NODE;
> +        
> +        /* Everything is good, lets set the number of vnodes */
> +        d->vnuma.nr_vnodes = nr_vnodes;
Put a blank line here.

> +        ret = 0;
> +err_dom:
> +        if ( ret != 0 )
> +        {
> +            d->vnuma.nr_vnodes = 0;
> +            xfree(d->vnuma.vdistance);
> +            xfree(d->vnuma.vmemrange);
> +            xfree(d->vnuma.vcpu_to_vnode);
> +            xfree(d->vnuma.vnode_numamap);
> +        }
> +    }
> +    break;
> +

> diff --git a/xen/common/memory.c b/xen/common/memory.c
> @@ -733,6 +734,41 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>          break;
>  
> +    case XENMEM_get_vnuma_info:
> +    {
> +        vnuma_topology_info_t mtopology;
> +        struct domain *d;
> +
> +        rc = -EFAULT;
> +        if ( copy_from_guest(&mtopology, arg, 1) )
> +            return -EFAULT;
> +        if ( (d = rcu_lock_domain_by_any_id(mtopology.domid)) == NULL )
> +            return -ESRCH;
> +        
> +        if ( (d->vnuma.nr_vnodes == 0) || (d->vnuma.nr_vnodes > d->max_vcpus) )
> +            return EOPNOTSUPP;
>
I think you need to rcu_unlock_xxx() here.
Also, -EONOTSUPP (note the '-') ?

> +       
> +        if ( __copy_to_guest(mtopology.vmemrange,
> +                                d->vnuma.vmemrange,
> +                                d->vnuma.nr_vnodes) != 0 )
> +            goto vnumaout;
> +        if ( __copy_to_guest(mtopology.vdistance,
> +                                d->vnuma.vdistance,
> +                                d->vnuma.nr_vnodes * d->vnuma.nr_vnodes) != 0 )
> +            goto vnumaout;
> +        if ( __copy_to_guest(mtopology.vcpu_to_vnode,
> +                                d->vnuma.vcpu_to_vnode,
> +                                d->max_vcpus) != 0 )
> +            goto vnumaout;
> +        
> +        if ( __copy_to_guest(mtopology.nr_vnodes, &d->vnuma.nr_vnodes, 1) != 0 )
> +            goto vnumaout;
> +        rc = 0;
> +vnumaout:
> vnumainfo_out ?

> +        rcu_unlock_domain(d);
> +        break;
> +    }
> +
>      default:
>          rc = arch_memory_op(op, arg);
>          break;

> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> +/* 
> + * XEN_DOMCTL_setvnumainfo: sets the vNUMA topology
> + * parameters a guest may request.
> + */
> +struct xen_domctl_vnuma {
> +    uint32_t nr_vnodes;
> +    uint32_t __pad;
> +    XEN_GUEST_HANDLE_64(uint) vdistance;
> +    XEN_GUEST_HANDLE_64(uint) vcpu_to_vnode;
> +    /* domain memory mapping map to physical NUMA nodes */
> +    XEN_GUEST_HANDLE_64(uint) vnode_numamap;
> +    /* 
> +     * memory rages that vNUMA node can represent
                 ^ranges

> +     * If more than one, its a linked list.
> +     */
> +    XEN_GUEST_HANDLE_64(vmemrange_t) vmemrange;
> +};

> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index a057069..bc61bab 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -89,4 +89,14 @@ extern unsigned int xen_processor_pmbits;
>  
>  extern bool_t opt_dom0_vcpus_pin;
>  
> +struct domain_vnuma_info {
> +    uint nr_vnodes;
> +    uint *vdistance;
> +    uint *vcpu_to_vnode;
> +    uint *vnode_numamap;
> +    struct vmemrange *vmemrange;
> +};
> +
I think you can kill the 'domain_' prefix. It's pretty clear this is a
per-domain thing, from the fact that it lives inside struct domain.

> +void domain_vnuma_destroy(struct domain_vnuma_info *v);
> +
Why do you need to declare this function here? Isn't this used only in
domain.c ?

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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