From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v5 08/24] libxl: introduce libxl__vnuma_config_check Date: Fri, 13 Feb 2015 15:40:32 +0000 Message-ID: <54DE1AF0.80304@citrix.com> References: <1423770294-9779-1-git-send-email-wei.liu2@citrix.com> <1423770294-9779-9-git-send-email-wei.liu2@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1423770294-9779-9-git-send-email-wei.liu2@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu , xen-devel@lists.xen.org Cc: dario.faggioli@citrix.com, JBeulich@suse.com, ian.jackson@eu.citrix.com, ian.campbell@citrix.com, ufimtseva@gmail.com List-Id: xen-devel@lists.xenproject.org On 12/02/15 19:44, Wei Liu wrote: > This function is used to check whether vNUMA configuration (be it > auto-generated or supplied by user) is valid. > > Define a new error code ERROR_VNUMA_CONFIG_INVALID. > > The checks performed can be found in the comment of the function. > > This vNUMA function (and future ones) is placed in a new file called > libxl_vnuma.c > > Signed-off-by: Wei Liu > Cc: Ian Campbell > Cc: Ian Jackson > Cc: Dario Faggioli > Cc: Elena Ufimtseva > --- > Changes in v5: > 1. Define and use new error code. > 2. Use LOG macro. > 3. Fix hard tabs. > > Changes in v4: > 1. Adapt to new interface. > > Changes in v3: > 1. Rewrite commit log. > 2. Shorten two error messages. > --- > tools/libxl/Makefile | 2 +- > tools/libxl/libxl_internal.h | 7 +++ > tools/libxl/libxl_types.idl | 1 + > tools/libxl/libxl_vnuma.c | 131 +++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 140 insertions(+), 1 deletion(-) > create mode 100644 tools/libxl/libxl_vnuma.c > > diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile > index 7329521..1b16598 100644 > --- a/tools/libxl/Makefile > +++ b/tools/libxl/Makefile > @@ -93,7 +93,7 @@ LIBXL_LIBS += -lyajl > LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \ > libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \ > libxl_internal.o libxl_utils.o libxl_uuid.o \ > - libxl_json.o libxl_aoutils.o libxl_numa.o \ > + libxl_json.o libxl_aoutils.o libxl_numa.o libxl_vnuma.o \ > libxl_save_callout.o _libxl_save_msgs_callout.o \ > libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y) > LIBXL_OBJS += libxl_genid.o > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h > index 6d3ac58..258be0d 100644 > --- a/tools/libxl/libxl_internal.h > +++ b/tools/libxl/libxl_internal.h > @@ -3394,6 +3394,13 @@ void libxl__numa_candidate_put_nodemap(libxl__gc *gc, > libxl_bitmap_copy(CTX, &cndt->nodemap, nodemap); > } > > +/* Check if vNUMA config is valid. Returns 0 if valid, > + * ERROR_VNUMA_CONFIG_INVALID otherwise. > + */ > +int libxl__vnuma_config_check(libxl__gc *gc, > + const libxl_domain_build_info *b_info, > + const libxl__domain_build_state *state); > + > _hidden int libxl__ms_vm_genid_set(libxl__gc *gc, uint32_t domid, > const libxl_ms_vm_genid *id); > > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index 14c7e7c..23951fc 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -63,6 +63,7 @@ libxl_error = Enumeration("error", [ > (-17, "DEVICE_EXISTS"), > (-18, "REMUS_DEVOPS_DOES_NOT_MATCH"), > (-19, "REMUS_DEVICE_NOT_SUPPORTED"), > + (-20, "VNUMA_CONFIG_INVALID"), > ], value_namespace = "") > > libxl_domain_type = Enumeration("domain_type", [ > diff --git a/tools/libxl/libxl_vnuma.c b/tools/libxl/libxl_vnuma.c > new file mode 100644 > index 0000000..fa5aa8d > --- /dev/null > +++ b/tools/libxl/libxl_vnuma.c > @@ -0,0 +1,131 @@ > +/* > + * Copyright (C) 2014 Citrix Ltd. > + * Author Wei Liu > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU Lesser General Public License as published > + * by the Free Software Foundation; version 2.1 only. with the special > + * exception on linking described in file LICENSE. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU Lesser General Public License for more details. > + */ > +#include "libxl_osdeps.h" /* must come before any other headers */ > +#include "libxl_internal.h" > +#include > + > +/* Sort vmemranges in ascending order with "start" */ > +static int compare_vmemrange(const void *a, const void *b) > +{ > + const xen_vmemrange_t *x = a, *y = b; > + if (x->start < y->start) > + return -1; > + if (x->start > y->start) > + return 1; > + return 0; > +} > + > +/* Check if vNUMA configuration is valid: > + * 1. all pnodes inside vnode_to_pnode array are valid > + * 2. one vcpu belongs to and only belongs to one vnode > + * 3. each vmemrange is valid and doesn't overlap with each other > + */ > +int libxl__vnuma_config_check(libxl__gc *gc, > + const libxl_domain_build_info *b_info, > + const libxl__domain_build_state *state) > +{ > + int i, j, rc = ERROR_VNUMA_CONFIG_INVALID, nr_nodes; i, j and nr_nodes are all semantically unsigned. > + libxl_numainfo *ninfo = NULL; > + uint64_t total_memkb = 0; > + libxl_bitmap cpumap; > + libxl_vnode_info *p; > + > + libxl_bitmap_init(&cpumap); > + > + /* Check pnode specified is valid */ > + ninfo = libxl_get_numainfo(CTX, &nr_nodes); > + if (!ninfo) { > + LOG(ERROR, "libxl_get_numainfo failed"); > + goto out; > + } > + > + for (i = 0; i < b_info->num_vnuma_nodes; i++) { > + uint32_t pnode; > + > + p = &b_info->vnuma_nodes[i]; > + pnode = p->pnode; > + > + /* The pnode specified is not valid? */ > + if (pnode >= nr_nodes) { > + LOG(ERROR, "Invalid pnode %d specified", pnode); pnode is uint32_t, so should be %u > + goto out; > + } > + > + total_memkb += p->memkb; > + } > + > + if (total_memkb != b_info->max_memkb) { > + LOG(ERROR, "Amount of memory mismatch (0x%"PRIx64" != 0x%"PRIx64")", > + total_memkb, b_info->max_memkb); > + goto out; > + } > + > + /* Check vcpu mapping */ > + libxl_cpu_bitmap_alloc(CTX, &cpumap, b_info->max_vcpus); > + libxl_bitmap_set_none(&cpumap); Worth using/making libxl_cpu_bitmap_zalloc(), or perhaps making this a defined semantic of the alloc() function? This would seem to be a very common pair of operations to perform. > + for (i = 0; i < b_info->num_vnuma_nodes; i++) { > + p = &b_info->vnuma_nodes[i]; > + libxl_for_each_set_bit(j, p->vcpus) { > + if (!libxl_bitmap_test(&cpumap, j)) > + libxl_bitmap_set(&cpumap, j); > + else { > + LOG(ERROR, "Vcpu %d assigned more than once", j); > + goto out; > + } > + } This libxl_for_each_set_bit() loop can be optimised to a bitmap_intersects() for the error condition, and bitmap_or() for the success case. > + } > + > + for (i = 0; i < b_info->max_vcpus; i++) { > + if (!libxl_bitmap_test(&cpumap, i)) { > + LOG(ERROR, "Vcpu %d is not assigned to any vnode", i); > + goto out; > + } This loop can be optimised to !bitmap_all_set(). > + } > + > + /* Check vmemranges */ > + qsort(state->vmemranges, state->num_vmemranges, sizeof(xen_vmemrange_t), > + compare_vmemrange); > + > + for (i = 0; i < state->num_vmemranges; i++) { > + if (state->vmemranges[i].end < state->vmemranges[i].start) { > + LOG(ERROR, "Vmemrange end < start"); > + goto out; > + } > + } > + > + for (i = 0; i < state->num_vmemranges - 1; i++) { > + if (state->vmemranges[i].end > state->vmemranges[i+1].start) { > + LOG(ERROR, > + "Vmemranges overlapped, 0x%"PRIx64"-0x%"PRIx64", 0x%"PRIx64"-0x%"PRIx64, > + state->vmemranges[i].start, state->vmemranges[i].end, > + state->vmemranges[i+1].start, state->vmemranges[i+1].end); > + goto out; > + } > + } > + > + rc = 0; > +out: > + if (ninfo) libxl_numainfo_dispose(ninfo); Completely Unrelated to the content of this patch, I suggest that $FOO_dispose() functions are required to act as free() does with NULL pointers. It would simply caller error handling. ~Andrew > + libxl_bitmap_dispose(&cpumap); > + return rc; > +} > + > +/* > + * Local variables: > + * mode: C > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */