Re: [Xen-devel] [PATCH v4 14/15] xl: enable for specifying node-affinity in the config file

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-11-22 at 19:58 +0100, Dario Faggioli wrote:
> in a similar way to how it is possible to specify vcpu-affinity.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> ---
> Changes from v3:
>  * fix typos and language issues in docs and comments, as
>    suggested during review;
>  * common code to soft and hard affinity parsing factored
>    together, as requested uring review.
> 
> Changes from v2:
>  * use the new libxl API. Although the implementation changed
>    only a little bit, I removed IanJ's Acked-by, although I am
>    here saying that he did provided it, as requested.
> ---
>  docs/man/xl.cfg.pod.5    |   25 +++++-
>  tools/libxl/xl_cmdimpl.c |  192 
> ++++++++++++++++++++++++++++++----------------
>  2 files changed, 146 insertions(+), 71 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 5e17b5d..9566393 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> +=item B<cpus_soft="CPU-LIST">
> +
> +Exactly as B<cpus=>, but specifies soft affinity, rather than pinning
> +(also called hard affinity). When using the credit scheduler, this
> +means the vcpus of the domain prefers to run on these pcpus.

Just "prefer".

> +
> +A C<CPU-LIST> is specified exactly as above, for B<cpus=>.
> +
> +If this option is not specified, the vcpus of the guest will not have
> +any preference regarding on what cpu to run, and the scheduler will
> +treat all the cpus where a vcpu can execute (if B<cpus=> is specified),
> +or all the host cpus (if not), the same.

You can end this sentence after "on what cpu to run" without losing
information IMHO.

>  If this option is specified,
> +the intersection of the soft affinity mask, provided here, and the vcpu
> +pinning, provided via B<cpus=> (if any), is utilized to compute the
> +domain node-affinity, for driving memory allocations.
> +
>  =back
>  
>  =head3 CPU Scheduling
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index d6fda26..abd3e42 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -76,8 +76,9 @@ xlchild children[child_max];
>  static const char *common_domname;
>  static int fd_lock = -1;
>  
> -/* Stash for specific vcpu to pcpu mappping */
> +/* Stash for specific vcpu to pcpu hard and soft mappping */

You could fix the spelling of mapping while here.

>  static int *vcpu_to_pcpu;
> +static int *vcpu_to_pcpu_soft;
>  
>  static const char savefileheader_magic[32]=
>      "Xen saved domain, xl format\n \0 \r";
> @@ -687,6 +688,64 @@ static int vcpupin_parse(char *cpu, libxl_bitmap *cpumap)
>      return rc;
>  }
>  
> +static int *parse_config_cpumap_list(XLU_ConfigList *cpus,
> +                                     libxl_bitmap *cpumap,
> +                                     int max_vcpus)

I assume this is pure motion/refactoring? Or do I need to read it?

> +{
> +    int i, n_cpus = 0;
> +    int *to_pcpu;
> +    const char *buf;
> +
> +    if (libxl_cpu_bitmap_alloc(ctx, cpumap, 0)) {
> +        fprintf(stderr, "Unable to allocate cpumap\n");
> +        exit(1);
> +    }
> +
> +    /* Prepare the array for single vcpu to pcpu mappings */
> +    to_pcpu = xmalloc(sizeof(int) * max_vcpus);
> +    memset(to_pcpu, -1, sizeof(int) * max_vcpus);
> +
> +    /*
> +     * Idea here is to let libxl think all the domain's vcpus
> +     * have cpu affinity with all the pcpus on the list. Doing
> +     * that ensures memory is allocated on the proper NUMA nodes.
> +     * It is then us, here in xl, that matches each single vcpu
> +     * to its pcpu (and that's why we need to stash such info in
> +     * the to_pcpu array now) after the domain has been created.
> +     * This way, we avoid having to pass to libxl some big array
> +     * hosting the single mappings.
> +     */
> +    libxl_bitmap_set_none(cpumap);
> +    while ((buf = xlu_cfg_get_listitem(cpus, n_cpus)) != NULL) {
> +        i = atoi(buf);
> +        if (!libxl_bitmap_cpu_valid(cpumap, i)) {
> +            fprintf(stderr, "cpu %d illegal\n", i);
> +            exit(1);
> +        }
> +        libxl_bitmap_set(cpumap, i);
> +        if (n_cpus < max_vcpus)
> +            to_pcpu[n_cpus] = i;
> +        n_cpus++;
> +    }
> +
> +    return to_pcpu;
> +}
> +
> +static void parse_config_cpumap_string(const char *buf, libxl_bitmap *cpumap)

Likewise this is also motion/refactoring, correct?

> +{
> +        char *buf2 = strdup(buf);
> +
> +        if (libxl_cpu_bitmap_alloc(ctx, cpumap, 0)) {
> +            fprintf(stderr, "Unable to allocate cpumap\n");
> +            exit(1);
> +        }
> +
> +        libxl_bitmap_set_none(cpumap);
> +        if (vcpupin_parse(buf2, cpumap))
> +            exit(1);
> +        free(buf2);
> +}
> +
>  static void parse_config_data(const char *config_source,
>                                const char *config_data,
>                                int config_len,
> @@ -697,7 +756,8 @@ static void parse_config_data(const char *config_source,
>      const char *buf;
>      long l;
>      XLU_Config *config;
> -    XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms;
> +    XLU_ConfigList *cpus, *cpus_soft, *vbds, *nics, *pcis;
> +    XLU_ConfigList *cvfbs, *cpuids, *vtpms;
>      XLU_ConfigList *ioports, *irqs, *iomem;
>      int num_ioports, num_irqs, num_iomem;
>      int pci_power_mgmt = 0;
> @@ -820,57 +880,24 @@ static void parse_config_data(const char *config_source,
>          b_info->max_vcpus = l;
>  
>      if (!xlu_cfg_get_list (config, "cpus", &cpus, 0, 1)) {
> -        int n_cpus = 0;
> -
> -        if (libxl_cpu_bitmap_alloc(ctx, &b_info->cpumap, 0)) {
> -            fprintf(stderr, "Unable to allocate cpumap\n");
> -            exit(1);
> -        }
> -
> -        /* Prepare the array for single vcpu to pcpu mappings */
> -        vcpu_to_pcpu = xmalloc(sizeof(int) * b_info->max_vcpus);
> -        memset(vcpu_to_pcpu, -1, sizeof(int) * b_info->max_vcpus);
> -
> -        /*
> -         * Idea here is to let libxl think all the domain's vcpus
> -         * have cpu affinity with all the pcpus on the list.
> -         * It is then us, here in xl, that matches each single vcpu
> -         * to its pcpu (and that's why we need to stash such info in
> -         * the vcpu_to_pcpu array now) after the domain has been created.
> -         * Doing it like this saves the burden of passing to libxl
> -         * some big array hosting the single mappings. Also, using
> -         * the cpumap derived from the list ensures memory is being
> -         * allocated on the proper nodes anyway.
> -         */
> -        libxl_bitmap_set_none(&b_info->cpumap);
> -        while ((buf = xlu_cfg_get_listitem(cpus, n_cpus)) != NULL) {
> -            i = atoi(buf);
> -            if (!libxl_bitmap_cpu_valid(&b_info->cpumap, i)) {
> -                fprintf(stderr, "cpu %d illegal\n", i);
> -                exit(1);
> -            }
> -            libxl_bitmap_set(&b_info->cpumap, i);
> -            if (n_cpus < b_info->max_vcpus)
> -                vcpu_to_pcpu[n_cpus] = i;
> -            n_cpus++;
> -        }
> -
> +        vcpu_to_pcpu = parse_config_cpumap_list(cpus, &b_info->cpumap,
> +                                                b_info->max_vcpus);
>          /* We have a cpumap, disable automatic placement */
>          libxl_defbool_set(&b_info->numa_placement, false);
>      }
>      else if (!xlu_cfg_get_string (config, "cpus", &buf, 0)) {
> -        char *buf2 = strdup(buf);
> -
> -        if (libxl_cpu_bitmap_alloc(ctx, &b_info->cpumap, 0)) {
> -            fprintf(stderr, "Unable to allocate cpumap\n");
> -            exit(1);
> -        }
> -
> -        libxl_bitmap_set_none(&b_info->cpumap);
> -        if (vcpupin_parse(buf2, &b_info->cpumap))
> -            exit(1);
> -        free(buf2);
> +        parse_config_cpumap_string(buf, &b_info->cpumap);
> +        libxl_defbool_set(&b_info->numa_placement, false);
> +    }
>  
> +    if (!xlu_cfg_get_list (config, "cpus_soft", &cpus_soft, 0, 1)) {
> +        vcpu_to_pcpu_soft =
> +            parse_config_cpumap_list(cpus_soft, &b_info->cpumap_soft,
> +                                     b_info->max_vcpus);
> +        libxl_defbool_set(&b_info->numa_placement, false);
> +    }
> +    else if (!xlu_cfg_get_string (config, "cpus_soft", &buf, 0)) {
> +        parse_config_cpumap_string(buf, &b_info->cpumap_soft);
>          libxl_defbool_set(&b_info->numa_placement, false);
>      }

This is / else if pattern is the same for both types of bitmap, right?

Can you make it a function which takes config, "cpus_soft"/"cpus" and a
pointer to the map to fill in etc?

>  
> @@ -1991,6 +2018,40 @@ static void 
> evdisable_disk_ejects(libxl_evgen_disk_eject **diskws,
>      }
>  }
>  
> +static inline int set_vcpu_to_pcpu_affinity(uint32_t domid, int *to_pcpu,
> +                                            int max_vcpus, int soft)
> +{
> +    libxl_bitmap vcpu_cpumap;
> +    int i, ret = 0;
> +
> +    ret = libxl_cpu_bitmap_alloc(ctx, &vcpu_cpumap, 0);
> +    if (ret)
> +        return -1;
> +
> +    for (i = 0; i < max_vcpus; i++) {
> +        if (to_pcpu[i] != -1) {
> +            libxl_bitmap_set_none(&vcpu_cpumap);
> +            libxl_bitmap_set(&vcpu_cpumap, to_pcpu[i]);
> +        } else {
> +            libxl_bitmap_set_any(&vcpu_cpumap);
> +        }
> +        if (!soft &&
> +            libxl_set_vcpuaffinity(ctx, domid, i, &vcpu_cpumap, NULL)) {
> +            fprintf(stderr, "setting hard affinity failed on vcpu `%d'.\n", 
> i);
> +            ret = -1;
> +            break;
> +        } else if (soft &&
> +                   libxl_set_vcpuaffinity(ctx, domid, i, NULL, 
> &vcpu_cpumap)) {

Another place where using termporary hard and soft pointers initialised
from the soft boolean would work I think.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel