[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.
Hi guys Thank you for reviewing, I will clean these out. George, after talking to Dario, I think the max number of physical nodes will not exceed 256. Dario's automatic NUMA placement work with this number and I think it can be easily u8. Unless anyone has other thoughts. Elena On Mon, Sep 16, 2013 at 11:46 AM, George Dunlap <George.Dunlap@xxxxxxxxxxxxx> wrote: > 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 -- Elena _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |