Re: [PATCH RESEND 02/12] xl: allow for node-wise specification of vcpu pinning

[George Dunlap <george.dunlap@eu.citrix.com>]

On 11/05/2013 02:34 PM, Dario Faggioli wrote:
> Making it possible to use something like the following:
>   * "nodes:0-3": all pCPUs of nodes 0,1,2,3;
>   * "nodes:0-3,^node:2": all pCPUS of nodes 0,1,3;
>   * "1,nodes:1-2,^6": pCPU 1 plus all pCPUs of nodes 1,2
>     but not pCPU 6;
>   * ...
>
> In both domain config file and `xl vcpu-pin'.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

Overall looks like a pretty clean patch; just a few comments.

> ---
> Picking this up from a previously submitted series ("xl:
> allow for node-wise specification of vcpu pinning") as the
> changes in that and in this series would otherwise be
> conflicting. If this is considered fine, Feel free to apply
> it from here and skip the corresponding e-mail in the
> original submission.
> ---
> Changes from v2:
>   * turned a 'return' into 'goto out', consistently with the
>     most of exit patterns;
>   * harmonized error handling: now parse_range() return a
>     libxl error code, as requested during review;
>   * dealing with "all" moved inside update_cpumap_range().
>     It's tricky to move it in parse_range() (as requested
>     during review), since we need the cpumap being modified
>     handy when dealing with it. However, having it in
>     update_cpumap_range() simplifies the code just as much
>     as that;
>   * explicitly checking for junk after a valid value or range
>     in parse_range(), as requested during review;
>   * xl exits on parsing failing, so no need to reset the
>     cpumap to something sensible in vcpupin_parse(), as
>     suggested during review;
>
> Changes from v1:
>   * code rearranged in order to look more simple to follow
>     and understand, as requested during review;
>   * improved docs in xl.cfg.pod.5, as requested during
>     review;
>   * strtoul() now returns into unsigned long, and the
>     case where it returns ULONG_MAX is now taken into
>     account, as requested during review;
>   * stuff like "all,^7" now works, as requested during
>     review. Specifying just "^7" does not work either
>     before or after this change
>   * killed some magic (i.e., `ptr += 5 + (ptr[4] == 's'`)
>     by introducing STR_SKIP_PREFIX() macro, as requested
>     during review.
> ---
>   docs/man/xl.cfg.pod.5    |   20 ++++++
>   tools/libxl/xl_cmdimpl.c |  145 ++++++++++++++++++++++++++++++++--------------
>   2 files changed, 121 insertions(+), 44 deletions(-)
>
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index d2d8921..1c98cb4 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -115,7 +115,25 @@ To allow all the vcpus of the guest to run on all the cpus on the host.
>
>   =item "0-3,5,^1"
>
> -To allow all the vcpus of the guest to run on cpus 0,2,3,5.
> +To allow all the vcpus of the guest to run on cpus 0,2,3,5. Combining
> +this with "all" is possible, meaning "all,^7" results in all the vcpus
> +of the guest running on all the cpus on the host except cpu 7.
> +
> +=item "nodes:0-3,node:^2"

Here you use both "nodes" and "node", while the code seems to only check 
for "nodes".  I was originally going to say we should just check one; 
but on the other hand, it's just an extra string compare -- I feel like 
we might as well accept either "node" or "nodes".  (No need to enforce 
plurality: "nodes:2" and "node:1-3" should both be fine with me.)

> +
> +To allow all the vcpus of the guest to run on the cpus from NUMA nodes
> +0,1,3 of the host. So, if cpus 0-3 belongs to node 0, cpus 4-7 belongs
> +to node 1 and cpus 8-11 to node 3, the above would mean all the vcpus
> +of the guest will run on cpus 0-3,8-11.
> +
> +Combining this notation with the one above is possible. For instance,
> +"1,node:2,^6", means all the vcpus of the guest will run on cpu 1 and
> +on all the cpus of NUMA node 2, but not on cpu 6. Following the same
> +example as above, that would be cpus 1,4,5,7.
> +
> +Combining this with "all" is also possible, meaning "all,^nodes:1"
> +results in all the vcpus of the guest running on all the cpus on the
> +host, except for the cpus belonging to the host NUMA node 1.
>
>   =item ["2", "3"] (or [2, 3])
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 40feb7d..b8755b9 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -59,6 +59,11 @@
>           }                                                               \
>       })
>
> +#define STR_HAS_PREFIX( a, b )  \
> +    ( strncmp(a, b, strlen(b)) == 0 )
> +#define STR_SKIP_PREFIX( a, b ) \
> +    ( STR_HAS_PREFIX(a, b) ? (a) += strlen(b) : NULL )
> +
>
>   int logfile = 2;
>
> @@ -513,61 +518,115 @@ static void split_string_into_string_list(const char *str,
>       free(s);
>   }
>
> -static int vcpupin_parse(char *cpu, libxl_bitmap *cpumap)
> +static int parse_range(const char *str, unsigned long *a, unsigned long *b)
> +{
> +    char *nstr, *endptr;
> +
> +    *a = *b = strtoul(str, &endptr, 10);
> +    if (endptr == str || *a == ULONG_MAX)
> +        return ERROR_INVAL;
> +
> +    if (*endptr == '-') {
> +        nstr = endptr + 1;
> +
> +        *b = strtoul(nstr, &endptr, 10);
> +        if (endptr == nstr || *b == ULONG_MAX || *b < *a)
> +            return ERROR_INVAL;
> +    }
> +
> +    /* Valid value or range so far, but we also don't want junk after that */
> +    if (*endptr != '\0')
> +        return ERROR_INVAL;
> +
> +    return 0;
> +}
> +
> +/*
> + * Add or removes a specific set of cpus (specified in str, either as
> + * single cpus or as entire NUMA nodes) to/from cpumap.
> + */
> +static int update_cpumap_range(const char *str, libxl_bitmap *cpumap)
>   {
> -    libxl_bitmap exclude_cpumap;
> -    uint32_t cpuida, cpuidb;
> -    char *endptr, *toka, *tokb, *saveptr = NULL;
> -    int i, rc = 0, rmcpu;
> +    unsigned long ida, idb;
> +    libxl_bitmap node_cpumap;
> +    bool is_not = false, is_nodes = false;
> +    int rc = 0;
> +
> +    libxl_bitmap_init(&node_cpumap);
>
> -    if (!strcmp(cpu, "all")) {
> +    rc = libxl_node_bitmap_alloc(ctx, &node_cpumap, 0);
> +    if (rc) {
> +        fprintf(stderr, "libxl_node_bitmap_alloc failed.\n");
> +        goto out;
> +    }
> +
> +    /* Are we adding or removing cpus/nodes? */
> +    if (STR_SKIP_PREFIX(str, "^")) {
> +        is_not = true;
> +    }
> +
> +    /* Are we dealing with cpus or full nodes? */
> +    if (STR_SKIP_PREFIX(str, "nodes:")) {
> +        is_nodes = true;
> +    }
> +
> +    if (STR_HAS_PREFIX(str, "all")) {

Is there any reason not to keep this "strcmp"?  As it is, this will 
accept any string that *starts* with "all", which isn't exactly what you 
want, I don't think.

Other than that, I think it looks good.

  -George