[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC v2 1/7] xen/vNUMA: vNUMA support for PV guests.
On Fri, Sep 13, 2013 at 9:49 AM, Elena Ufimtseva <ufimtseva@xxxxxxxxx> wrote: > +void domain_vnuma_destroy(struct domain_vnuma_info *v) As long as you're respinning, I think I'd prefer this be named something different -- 'v' is almost universally of type "struct vcpu" in the Xen code, so this code is a little confusing. I might use "vinfo", or even just take a domain pointer and dereference it yourself. > +{ > + 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; > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > index 9760d50..e5d05c7 100644 > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -29,6 +29,7 @@ > #include <asm/page.h> > #include <public/domctl.h> > #include <xsm/xsm.h> > +#include <xen/vnuma.h> > > static DEFINE_SPINLOCK(domctl_lock); > DEFINE_SPINLOCK(vcpu_alloc_lock); > @@ -862,7 +863,72 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) > u_domctl) > ret = set_global_virq_handler(d, virq); > } > break; > - > + 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; I think the zeroing of these is unnecessary -- you zero them / initialize them before you use them below. You need to break this code into "paragraphs" of related changes, with a space in between. For example, maybe here... > + nr_vnodes = op->u.vnuma.nr_vnodes; > + if (nr_vnodes <= 0 || nr_vnodes > NR_CPUS) > + break; here... > + d->vnuma.nr_vnodes = nr_vnodes; > + dist_size = nr_vnodes * nr_vnodes; here... > + 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; here... Also, for an idiomatic way to do this kind of multiple allocation w/ error checking, take a look at xen/arch/x86/hvm/hvm.c:hvm_domain_initialise() (to pick a random function I've worked with recently). Look at the > + for ( i = 0; i < nr_vnodes; i++ ) > + for ( j = 0; j < nr_vnodes; j++ ) > + { > + if ( unlikely(__copy_from_guest_offset(&dist, > + op->u.vnuma.vdistance, > + __vnode_distance_offset(d, i, j), > 1)) ) > + goto err_dom; > + __vnode_distance_set(d, i, j, dist); > + } here... Also, in each of these error paths, you leak the structures allocated above. > + for ( i = 0; i < nr_vnodes; i++ ) > + { > + 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; > + } here... > + 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; > + } here... > + 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++) > + d->vnuma.vnode_to_pnode[i] = NUMA_NO_NODE; here... > + ret = 0; and maybe here. > + break; > +err_dom: > + ret = -EINVAL; > + } > + break; > default: > ret = arch_do_domctl(op, d, u_domctl); > break; > diff --git a/xen/common/memory.c b/xen/common/memory.c > index 50b740f..6dc2452 100644 > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -28,6 +28,7 @@ > #include <public/memory.h> > #include <xsm/xsm.h> > #include <xen/trace.h> > +#include <xen/vnuma.h> > > struct memop_args { > /* INPUT */ > @@ -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; here... > + if ( guest_handle_is_null(arg) ) > + return rc; here... > + if( copy_from_guest(&mtopology, arg, 1) ) > + return -EINVAL; here... (and so on) This should be -EFAULT > + if ( (d = rcu_lock_domain_by_any_id(mtopology.domid)) == NULL ) > + return -EINVAL; > + 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; > + vblks = xmalloc_array(struct vnuma_memblk, nr_vnodes); > + if ( vblks == NULL ) > + return -EFAULT; Uum, what is this for? It doesn't appear to be used -- if there's a copy failure it's freed again, if things succeed it's leaked. > + for ( i = 0; i < nr_vnodes; i++ ) > + { > + if ( copy_to_guest_offset(mtopology.vnuma_memblks, i, > + &d->vnuma.vnuma_memblks[i], 1) < 0 ) > + goto out; 4 spaces for indentation, please. > + } > + for ( i = 0; i < touser_topo.nr_vnodes; i++ ) > + 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); > + return rc; > + break; > + } > default: > rc = arch_memory_op(op, arg); > break; > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > index 4c5b2bb..3574d0a 100644 > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -35,6 +35,7 @@ > #include "xen.h" > #include "grant_table.h" > #include "hvm/save.h" > +#include "memory.h" > > #define XEN_DOMCTL_INTERFACE_VERSION 0x00000009 > > @@ -852,6 +853,17 @@ struct xen_domctl_set_broken_page_p2m { > typedef struct xen_domctl_set_broken_page_p2m > xen_domctl_set_broken_page_p2m_t; > DEFINE_XEN_GUEST_HANDLE(xen_domctl_set_broken_page_p2m_t); > > +struct xen_domctl_vnuma { > + uint16_t nr_vnodes; > + XEN_GUEST_HANDLE_64(uint) vdistance; > + XEN_GUEST_HANDLE_64(uint) vcpu_to_vnode; > + XEN_GUEST_HANDLE_64(uint) vnode_to_pnode; > + XEN_GUEST_HANDLE_64(vnuma_memblk_t) vnuma_memblks; > +}; > + > +typedef struct xen_domctl_vnuma xen_domctl_vnuma_t; > +DEFINE_XEN_GUEST_HANDLE(xen_domctl_vnuma_t); > + > struct xen_domctl { > uint32_t cmd; > #define XEN_DOMCTL_createdomain 1 > @@ -920,6 +932,7 @@ struct xen_domctl { > #define XEN_DOMCTL_set_broken_page_p2m 67 > #define XEN_DOMCTL_setnodeaffinity 68 > #define XEN_DOMCTL_getnodeaffinity 69 > +#define XEN_DOMCTL_setvnumainfo 70 > #define XEN_DOMCTL_gdbsx_guestmemio 1000 > #define XEN_DOMCTL_gdbsx_pausevcpu 1001 > #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 > @@ -979,6 +992,7 @@ struct xen_domctl { > struct xen_domctl_set_broken_page_p2m set_broken_page_p2m; > struct xen_domctl_gdbsx_pauseunp_vcpu gdbsx_pauseunp_vcpu; > struct xen_domctl_gdbsx_domstatus gdbsx_domstatus; > + struct xen_domctl_vnuma vnuma; > uint8_t pad[128]; > } u; > }; > @@ -986,7 +1000,6 @@ typedef struct xen_domctl xen_domctl_t; > DEFINE_XEN_GUEST_HANDLE(xen_domctl_t); > > #endif /* __XEN_PUBLIC_DOMCTL_H__ */ > - > /* > * Local variables: > * mode: C > diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h > index 7a26dee..28f6aaf 100644 > --- a/xen/include/public/memory.h > +++ b/xen/include/public/memory.h > @@ -453,12 +453,19 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t); > * Caller must be privileged or the hypercall fails. > */ > #define XENMEM_claim_pages 24 > - > /* > * XENMEM_claim_pages flags - the are no flags at this time. > * The zero value is appropiate. > */ > > +struct vnuma_memblk { > + uint64_t start, end; > +}; > +typedef struct vnuma_memblk vnuma_memblk_t; > +DEFINE_XEN_GUEST_HANDLE(vnuma_memblk_t); > + > +#define XENMEM_get_vnuma_info 25 > + > #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */ > > #endif /* __XEN_PUBLIC_MEMORY_H__ */ > diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h > index a057069..c9d53e3 100644 > --- a/xen/include/xen/domain.h > +++ b/xen/include/xen/domain.h > @@ -4,6 +4,7 @@ > > #include <public/xen.h> > #include <asm/domain.h> > +#include <public/memory.h> > > typedef union { > struct vcpu_guest_context *nat; > @@ -89,4 +90,14 @@ extern unsigned int xen_processor_pmbits; > > extern bool_t opt_dom0_vcpus_pin; > > +struct domain_vnuma_info { > + uint16_t nr_vnodes; > + uint *vdistance; > + uint *vcpu_to_vnode; > + uint *vnode_to_pnode; > + vnuma_memblk_t *vnuma_memblks; > +}; Are these the right sizes for things to allocate? Do we really need 32 bits to represent distance, vnode and pnode? Can we use u16 or u8? -George _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |