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