Re: [Xen-devel] [PATCH v8 10/13] libxl/xl: deprecate the build_info->cpumap field

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Jun 13, 2014 at 03:10:28PM +0200, Dario Faggioli wrote:
> as, thanks to previous change ("libxl/xl: push VCPU affinity
> pinning down to libxl"), we now have an array of libxl_bitmap-s
> that can be used to transfer to libxl the vcpu (hard) affinity
> of each vcpu of the domain. Therefore, the cpumap field is no
> longer necessary: if we want all the vcpus to have the same
> affinity, we just put it in all the elements of the array.
> 
> This makes the libxl code simpler and easier to understand
> and maintain (only one place where to read the affinity), and
> does not complicate things much on the xl side, that is why
> we go for it.
> 
> Another benefit is that, by unifying the parsing (at the xl
> level) and the place where the information is consumed and the
> affinity are actually set (at the libxl level), it becomes
> possible to do things like:
> 
>   cpus = ["3-4", "2-6"]
> 
> meaning we want vcpu 0 to be pinned to pcpu 3,4 and vcpu 1 to
> be pinned to pcpu 2,3,4,5,6. Before this change, in fact, the
> list variant (["xx", "yy"]) supported only single values.
> 
> BEWARE that, although still being there, for backward
> compatibility reasons, the cpumap field in build_info is no
> longer used anywhere in libxl.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> ---
>  docs/man/xl.cfg.pod.5       |    8 +++---
>  tools/libxl/libxl_create.c  |    6 ----
>  tools/libxl/libxl_dom.c     |    4 +--
>  tools/libxl/libxl_types.idl |    6 ++++
>  tools/libxl/xl_cmdimpl.c    |   61 
> +++++++++++++++++--------------------------
>  5 files changed, 34 insertions(+), 51 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index c087cbc..af48622 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -143,11 +143,11 @@ 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])
> +=item ["2", "3-8,^5"]
>  
> -To ask for specific vcpu mapping. That means (in this example), vcpu #0
> -of the guest will run on cpu #2 of the host and vcpu #1 of the guest will
> -run on cpu #3 of the host.
> +To ask for specific vcpu mapping. That means (in this example), vcpu 0
> +of the guest will run on cpu 2 of the host and vcpu 1 of the guest will
> +run on cpus 3,4,6,7,8 of the host.
>  
>  =back
>  
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index d015cf4..443fe7d 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -187,12 +187,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>      } else if (b_info->avail_vcpus.size > HVM_MAX_VCPUS)
>          return ERROR_FAIL;
>  
> -    if (!b_info->cpumap.size) {
> -        if (libxl_cpu_bitmap_alloc(CTX, &b_info->cpumap, 0))
> -            return ERROR_FAIL;
> -        libxl_bitmap_set_any(&b_info->cpumap);
> -    }
> -
>      libxl_defbool_setdefault(&b_info->numa_placement, true);
>  
>      if (!b_info->nodemap.size) {
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 1767659..0b00470 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -250,7 +250,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>       * whatever that turns out to be.
>       */
>      if (libxl_defbool_val(info->numa_placement)) {
> -        if (!libxl_bitmap_is_full(&info->cpumap)) {
> +        if (d_config->b_info.num_vcpu_hard_affinity) {
>              LOG(ERROR, "Can run NUMA placement only if no vcpu "
>                         "affinity is specified");
>              return ERROR_INVAL;
> @@ -261,8 +261,6 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>              return rc;
>      }
>      libxl_domain_set_nodeaffinity(ctx, domid, &info->nodemap);
> -    libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus,
> -                               &info->cpumap, NULL);
>  

So you actually remove cpumap handling logic in libxl as well? IMHO this
is not right.

Consider older application can set this map and now with the new library
CPU is not pinned anymore. I think you can effectively transform cpumap
to vcpu_hard_affinity list in libxl level to avoid functional regression
if cpumap is set and vcpu_hard_affinity is not set by application. If
both cpumap and vcpu_hard_affinity are set, vcpu_hard_affinity takes
precedence.

Wei.

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