Re: [Xen-devel] [PATCH RFC v2 1/7] xen/vNUMA: vNUMA support for PV guests.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.09.13 at 10:49, Elena Ufimtseva <ufimtseva@xxxxxxxxx> wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -227,6 +227,7 @@ struct domain *domain_create(
>      spin_lock_init(&d->node_affinity_lock);
>      d->node_affinity = NODE_MASK_ALL;
>      d->auto_node_affinity = 1;
> +    d->vnuma.nr_vnodes = 0;

Pointless (struct domain gets zero-allocated)?

> @@ -530,8 +531,9 @@ int domain_kill(struct domain *d)
>          evtchn_destroy(d);
>          gnttab_release_mappings(d);
>          tmem_destroy(d->tmem);
> -        domain_set_outstanding_pages(d, 0);
>          d->tmem = NULL;
> +        domain_set_outstanding_pages(d, 0);

Why is this being moved?

> +void domain_vnuma_destroy(struct domain_vnuma_info *v)
> +{
> +    if (v->vnuma_memblks) {
> +        xfree(v->vnuma_memblks);
> +        v->vnuma_memblks = NULL;
> +    }
> +    if (v->vcpu_to_vnode) {
> +        xfree(v->vcpu_to_vnode);
> +        v->vcpu_to_vnode = NULL;
> +    }
> +    if (v->vdistance) {
> +        xfree(v->vdistance);
> +        v->vdistance = NULL;
> +    }
> +    if (v->vnode_to_pnode) {
> +        xfree(v->vnode_to_pnode);
> +        v->vnode_to_pnode = NULL;
> +    }
> +    v->nr_vnodes = 0;

All the if()s are pointless - xfree() deals with NULL input quite fine.
And for the record, they also all don't match our coding style.

> @@ -862,7 +863,72 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>          ret = set_global_virq_handler(d, virq);
>      }
>      break;
> -

Please keep that newline, and insert another one after the final
break of the code you add.

> +    case XEN_DOMCTL_setvnumainfo:
> +    {
> +        unsigned int i, j, nr_vnodes, dist_size;
> +        unsigned int dist, vmap, vntop;
> +        vnuma_memblk_t vmemblk;
> +        
> +        ret = -EINVAL;
> +        dist = i = j = 0;
> +        nr_vnodes = op->u.vnuma.nr_vnodes; 
> +        if (nr_vnodes <= 0 || nr_vnodes > NR_CPUS)
> +            break;
> +        d->vnuma.nr_vnodes = nr_vnodes;
> +        dist_size = nr_vnodes * nr_vnodes;
> +        if (
> +        (d->vnuma.vdistance = xmalloc_bytes(
> +                 sizeof(*d->vnuma.vdistance) * dist_size)) == NULL ||
> +        (d->vnuma.vnuma_memblks = xmalloc_bytes(
> +                 sizeof(*d->vnuma.vnuma_memblks) * nr_vnodes)) == NULL ||
> +        (d->vnuma.vcpu_to_vnode = xmalloc_bytes(
> +                 sizeof(*d->vnuma.vcpu_to_vnode) * d->max_vcpus)) == NULL ||
> +        (d->vnuma.vnode_to_pnode = xmalloc_bytes(
> +                 sizeof(*d->vnuma.vnode_to_pnode) * nr_vnodes)) == NULL )
> +            goto err_dom;

Coding style. Also, you leak all memory that you managed to get
hold of in the error case.

> +        for ( i = 0; i < nr_vnodes; i++ )
> +            for ( j = 0; j < nr_vnodes; j++ )
> +            {
> +                if ( unlikely(__copy_from_guest_offset(&dist,

__copy_from_guest_offset() without prior guest_handle_okay()?
And the error code should be -EFAULT in that case.

> +                                        op->u.vnuma.vdistance,
> +                                        __vnode_distance_offset(d, i, j), 
> 1)) )
> +                    goto err_dom;
> +                __vnode_distance_set(d, i, j, dist);
> +            }
> +        for ( i = 0; i < nr_vnodes; i++ )

If possible please fold this with the earlier loop.

> +        {
> +            if ( unlikely(__copy_from_guest_offset(&vmemblk,
> +                                    op->u.vnuma.vnuma_memblks, i, 1)) )
> +                goto err_dom;
> +            d->vnuma.vnuma_memblks[i].start = vmemblk.start;
> +            d->vnuma.vnuma_memblks[i].end = vmemblk.end;
> +        }
> +        for ( i = 0; i < d->max_vcpus; i++ )
> +        {
> +            if ( unlikely(__copy_from_guest_offset(&vmap,
> +                           op->u.vnuma.vcpu_to_vnode, i, 1)) )
> +                goto err_dom;
> +            d->vnuma.vcpu_to_vnode[i] = vmap;

If the types are compatible, things like this don't need doing in a
loop - you could copy the whole array in one go.

> +        }
> +        if ( !guest_handle_is_null(op->u.vnuma.vnode_to_pnode) )
> +        {
> +            for ( i = 0; i < nr_vnodes; i++ )
> +            {
> +                if ( unlikely(__copy_from_guest_offset(&vntop,
> +                                        op->u.vnuma.vnode_to_pnode, i, 1)) )
> +                    goto err_dom;
> +                d->vnuma.vnode_to_pnode[i] = vntop;
> +            }
> +        }
> +        else
> +            for(i = 0; i < nr_vnodes; i++)

Coding style.

> +                d->vnuma.vnode_to_pnode[i] = NUMA_NO_NODE;

What's the value of using this domctl this way?

> +        ret = 0;
> +        break;
> +err_dom:
> +        ret = -EINVAL;

"ret" should not be unconditionally set to a fixed value here, the
more that you set it almost first thing in the case statement.

> @@ -732,7 +733,62 @@ long do_memory_op(unsigned long cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          rcu_unlock_domain(d);
>  
>          break;
> +    case XENMEM_get_vnuma_info:
> +    {
> +        int i, j;
> +        struct vnuma_topology_info mtopology;
> +        struct vnuma_topology_info touser_topo;
> +        struct domain *d;
> +        unsigned int max_pages, max_vcpus, nr_vnodes;
> +        vnuma_memblk_t *vblks;
>  
> +        rc = -EINVAL;
> +        if ( guest_handle_is_null(arg) )
> +            return rc;
> +        if( copy_from_guest(&mtopology, arg, 1) )
> +            return -EINVAL;

Consistency please, preferably such that you - as long as possible -
always return -E rather than rc.

Also the return value here again is -EFAULT.

> +        if ( (d = rcu_lock_domain_by_any_id(mtopology.domid)) == NULL )
> +            return -EINVAL;

And -ESRCH or some such here (please use existing code as
reference).

> +        touser_topo.nr_vnodes = d->vnuma.nr_vnodes;
> +        max_pages = d->max_pages;
> +        max_vcpus = d->max_vcpus;
> +        rcu_unlock_domain(d);
> +    
> +        nr_vnodes = touser_topo.nr_vnodes;
> +        rc = copy_to_guest(arg, &touser_topo, 1);
> +        if ( rc )
> +            return -EFAULT;
> +        if ( nr_vnodes == 0 || nr_vnodes > max_vcpus )
> +            return -EFAULT;

-EINVAL

> +        vblks = xmalloc_array(struct vnuma_memblk, nr_vnodes);
> +        if ( vblks == NULL )
> +            return -EFAULT;

-ENOMEM

> +        for ( i = 0; i < nr_vnodes; i++ )
> +        {
> +                if ( copy_to_guest_offset(mtopology.vnuma_memblks, i,
> +                           &d->vnuma.vnuma_memblks[i], 1) < 0 )

Indentation.

> +                    goto out;
> +        }
> +        for ( i = 0; i < touser_topo.nr_vnodes; i++ )

Again, please fold this with the previous loop, and use (preferably)
the simple variable as loop boundary (but in any case be consistent).

> +            for ( j = 0; j < touser_topo.nr_vnodes; j++ )
> +            {
> +                if ( copy_to_guest_offset(mtopology.vdistance,
> +                                            __vnode_distance_offset(d, i, j),
> +                                            &__vnode_distance(d, i, j), 1) )
> +                    goto out;
> +            }
> +        for ( i = 0; i < d->max_vcpus ; i++ )
> +        {
> +            if ( copy_to_guest_offset(mtopology.vcpu_to_vnode, i,
> +                                        &d->vnuma.vcpu_to_vnode[i], 1) )
> +                    goto out;
> +        }
> +        return rc;
> +out:
> +        if ( vblks ) xfree(vblks);

Coding style.

> --- /dev/null
> +++ b/xen/include/xen/vnuma.h
> @@ -0,0 +1,27 @@
> +#ifndef _VNUMA_H
> +#define _VNUMA_H
> +#include <public/memory.h>
> +
> +/* DEFINE_XEN_GUEST_HANDLE(vnuma_memblk_t); */
> +
> +struct vnuma_topology_info {
> +    domid_t domid;
> +    uint16_t nr_vnodes;
> +    uint32_t _pad;
> +    XEN_GUEST_HANDLE_64(uint) vdistance;
> +    XEN_GUEST_HANDLE_64(uint) vcpu_to_vnode;
> +    XEN_GUEST_HANDLE_64(vnuma_memblk_t) vnuma_memblks;
> +};
> +typedef struct vnuma_topology_info vnuma_topology_info_t;
> +DEFINE_XEN_GUEST_HANDLE(vnuma_topology_info_t);

At least up to here this appears to belong into the public header.

Jan

> +
> +#define __vnode_distance_offset(_dom, _i, _j) \
> +        ( ((_j) * ((_dom)->vnuma.nr_vnodes)) + (_i) )
> +
> +#define __vnode_distance(_dom, _i, _j) \
> +        ( (_dom)->vnuma.vdistance[__vnode_distance_offset((_dom), (_i), 
> (_j))] )
> +
> +#define __vnode_distance_set(_dom, _i, _j, _v) \
> +        do { __vnode_distance((_dom), (_i), (_j)) = (_v); } while(0)
> +
> +#endif
> -- 
> 1.7.10.4



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