|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 08/24] libxl: introduce libxl__vnuma_config_check
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 <wei.liu2@xxxxxxxxxx>
> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> Cc: Elena Ufimtseva <ufimtseva@xxxxxxxxx>
> ---
> 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 <wei.liu2@xxxxxxxxxx>
> + *
> + * 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 <stdlib.h>
> +
> +/* 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:
> + */
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |