Re: [Xen-devel] [PATCH V5 03/32] xl / libxl: push VCPU affinity pinning down to libxl

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-05-13 at 22:53 +0100, Wei Liu wrote:

diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 0dfafe7..7b0901c 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -305,6 +305,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
>      ("avail_vcpus",     libxl_bitmap),
>      ("cpumap",          libxl_bitmap),
>      ("nodemap",         libxl_bitmap),
> +    ("vcpu_affinity",   libxl_key_value_list),
> 
Is a key value list really the best way to represent this? At first
glance it seems like an array would be more suitable?

I've glanced through the rest on the assumption you have a convincing
reason why it should be a kvp list.

> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 661999c..b818815 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -263,6 +263,54 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>      libxl_domain_set_nodeaffinity(ctx, domid, &info->nodemap);
>      libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus, &info->cpumap);
>  
> +    /* If we have vcpu affinity list, pin vcpu to pcpu. */
> +    if (d_config->b_info.vcpu_affinity) {
> +        int i;
> +        libxl_bitmap vcpu_cpumap;
> +        int *vcpu_to_pcpu, sz = sizeof(int) * d_config->b_info.max_vcpus;
> +
> +        vcpu_to_pcpu = libxl__zalloc(gc, sz);

In theory this could be a stack allocation, either with alloca or just
        int vcpu_to_pcpu[sz];

> +        memset(vcpu_to_pcpu, -1, sz);
> +
> +        for (i = 0; i < d_config->b_info.max_vcpus; i++) {
> +            libxl_key_value_list kvl = d_config->b_info.vcpu_affinity;
> +            const char *key, *val;
> +            int k, v;
> +
> +            key = kvl[i * 2];

Need to bounds check kvl here. I think you might be better off iterating
over the kvl and validating the k against max_vcpus.

>      ("numa_placement",  libxl_defbool),
>      ("tsc_mode",        libxl_tsc_mode),
>      ("max_memkb",       MemKB),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index a1cb5b8..3200d40 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -88,9 +88,6 @@ xlchild children[child_max];
>  static const char *common_domname;
>  static int fd_lock = -1;
>  
> -/* Stash for specific vcpu to pcpu mappping */

mappppppping.
> @@ -833,11 +823,30 @@ static void parse_config_data(const char *config_source,
>                  exit(1);
>              }
>              libxl_bitmap_set(&b_info->cpumap, i);
> -            if (n_cpus < b_info->max_vcpus)
> -                vcpu_to_pcpu[n_cpus] = i;
> +            if (n_cpus < b_info->max_vcpus) {
> +                kvl = xrealloc(kvl, sizeof(char *) * (len * 2 + 2));

Can't you get the length of the input list and just allocate the right
size to start with?

Ian.


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