|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |