[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH RFC 1/7] xen/vnuma: subop hypercall and vnuma topology structures.



>>> On 27.08.13 at 09:54, Elena Ufimtseva <ufimtseva@xxxxxxxxx> wrote:
> Defines XENMEM subop hypercall for PV vNUMA enabled guests and provides
> vNUMA topology information from per-domain vnuma topology build info.
> TODO:
> subop XENMEM hypercall is subject to change to sysctl subop.

That would mean it's intended to be used by the tool stack only. I
thought that the balloon driver (and perhaps other code) are also
intended to be consumers.

> @@ -732,7 +733,94 @@ 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;
> +        struct vnuma_topology_info mtopology;
> +        struct vnuma_topology_info touser_topo;
> +        struct domain *d;
> +        unsigned int max_pages;
> +        vnuma_memblk_t *vblks;
> +        XEN_GUEST_HANDLE(int) vdistance;
> +        XEN_GUEST_HANDLE_PARAM(int) vdist_param;
> +        XEN_GUEST_HANDLE(vnuma_memblk_t) buf;
> +        XEN_GUEST_HANDLE_PARAM(vnuma_memblk_t) buf_param;
> +        XEN_GUEST_HANDLE(int) vcpu_to_vnode;
> +        XEN_GUEST_HANDLE_PARAM(int) vmap_param;
> +
> +        rc = -1;

You absolutely need to use proper -E... values when returnin
hypercall status.

> +        if ( guest_handle_is_null(arg) )
> +            return rc;
> +        if( copy_from_guest(&mtopology, arg, 1) )
> +        {
> +            gdprintk(XENLOG_INFO, "Cannot get copy_from_guest..\n");
> +            return -EFAULT;
> +        }
> +        gdprintk(XENLOG_INFO, "Domain id is %d\n",mtopology.domid);

I appreciate that you need such for debugging, but this should be
removed before posting patches.

> +        if ( (d = rcu_lock_domain_by_any_id(mtopology.domid)) == NULL )
> +        {
> +            gdprintk(XENLOG_INFO, "Numa: Could not get domain id.\n");
> +            return -ESRCH;
> +        }
> +        rcu_unlock_domain(d);
> +        touser_topo.nr_vnodes = d->vnuma.nr_vnodes;

Mis-ordered: First you want to use d, then rcu-unlock it.

> +        rc = copy_to_guest(arg, &touser_topo, 1);
> +        if ( rc )
> +        {
> +            gdprintk(XENLOG_INFO, "Bad news, could not copy to guest NUMA 
> info\n");
> +            return -EFAULT;
> +        }
> +        max_pages = d->max_pages;
> +        if ( touser_topo.nr_vnodes == 0 || touser_topo.nr_vnodes > 
> d->max_vcpus )
> +        {
> +            gdprintk(XENLOG_INFO, "vNUMA: Error in block creation - vnodes 
> %d, vcpus %d \n", touser_topo.nr_vnodes, d->max_vcpus);
> +            return -EFAULT;
> +        }
> +        vblks = (vnuma_memblk_t *)xmalloc_array(struct vnuma_memblk, 
> touser_topo.nr_vnodes);
> +        if ( vblks == NULL )
> +        {
> +            gdprintk(XENLOG_INFO, "vNUMA: Could not get memory for 
> memblocks\n");
> +            return -1;
> +        }
> +        buf_param = guest_handle_cast(mtopology.vnuma_memblks, 
> vnuma_memblk_t);

By giving the structure field a proper type you should be able to
avoid the use of guest_handle_cast() here and below.

> +        buf = guest_handle_from_param(buf_param, vnuma_memblk_t);
> +        for ( i = 0; i < touser_topo.nr_vnodes; i++ )
> +        {
> +                gdprintk(XENLOG_INFO, "vmemblk[%d] start %#lx end %#lx\n", 
> i, d->vnuma.vnuma_memblks[i].start, d->vnuma.vnuma_memblks[i].end);

Actually, I'm going to give up here (for this file) - the not cleaned up
code is obfuscating the real meat of the code too much for
reasonable reviewing.

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -453,6 +453,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
>   * Caller must be privileged or the hypercall fails.
>   */
>  #define XENMEM_claim_pages                  24
> +#define XENMEM_get_vnuma_info               25
>  
>  /*
>   * XENMEM_claim_pages flags - the are no flags at this time.

Misplaced - don't put this in the middle of another logical section.

> --- /dev/null
> +++ b/xen/include/public/vnuma.h
> @@ -0,0 +1,12 @@
> +#ifndef __XEN_PUBLIC_VNUMA_H
> +#define __XEN_PUBLIC_VNUMA_H
> +
> +#include "xen.h"
> +
> +struct vnuma_memblk {
> +              uint64_t start, end;
> +};
> +typedef struct vnuma_memblk vnuma_memblk_t;
> +DEFINE_XEN_GUEST_HANDLE(vnuma_memblk_t);
> +
> +#endif

Unmotivated new file. Plus the type isn't used elsewhere in the
public interface.

> @@ -89,4 +90,12 @@ extern unsigned int xen_processor_pmbits;
>  
>  extern bool_t opt_dom0_vcpus_pin;
>  
> +struct domain_vnuma_info {
> +    uint16_t nr_vnodes;
> +    int *vdistance;
> +    vnuma_memblk_t *vnuma_memblks;
> +    int *vcpu_to_vnode;
> +    int *vnode_to_pnode;

Can any of the "int" fields reasonably be negative? If not, they
ought to be "unsigned int".

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

At least up to here it seems like this is part of the intended public
interface, and hence ought to go into public/memory.h.

> +#define __vnode_distance_offset(_dom, _i, _j) \
> +        ( ((_j)*((_dom)->vnuma.nr_vnodes)) + (_i) )

Missing blanks around *.

> +
> +#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)

Proper parenthesization is clearly necessary, but there are clearly
some that are reducing legibility of the code without having any
useful purpose.

Jan

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.