[PATCH v1 4/4] libxl: set vcpu affinity during domain creation

[Jim Fehlig]

On 5/3/21 4:56 AM, Olaf Hering wrote:
> Since Xen 4.5 libxl allows to set affinities during domain creation.
> This enables Xen to allocate the domain memory on NUMA systems close to
> the specified pcpus.
> 
> Libvirt can now handle <domain/cputune/vcpupin> in domU.xml correctly.
> 
> Without this change, Xen will create the domU and assign NUMA memory and
> vcpu affinities on its own. Later libvirt will adjust the affinity,
> which may move the vcpus away from the assigned NUMA node.

That's not nice. Thanks for looking into it and providing a fix!

> Signed-off-by: Olaf Hering <olaf at aepfle.de>
> ---
>   src/libxl/libxl_conf.c | 53 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 53 insertions(+)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 3ccb00ec35..3a969c38cd 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -283,6 +283,56 @@ libxlMakeChrdevStr(virDomainChrDef *def, char **buf)
>       return 0;
>   }
>   
> +static int
> +libxlDomainSetVcpuAffinities(virDomainDef *def,
> +                             libxl_ctx *ctx,
> +                             libxl_domain_build_info *b_info)

We should tweak the name of this function after moving it from libxl_domain.c. 
Dropping 'Domain' is probably enough, e.g. libxlSetVcpuAffinities.

> +{
> +    libxl_bitmap *vcpu_affinity_array;
> +    unsigned int vcpuid;
> +    unsigned int vcpu_idx = 0;

libvirt typically uses size_t.

> +    virDomainVcpuDef *vcpu;
> +    bool has_vcpu_pin = false;
> +
> +    /* Get highest vcpuid with cpumask */
> +    for (vcpuid = 0; vcpuid < b_info->max_vcpus; vcpuid++) {
> +        vcpu = virDomainDefGetVcpu(def, vcpuid);
> +        if (!vcpu)
> +            continue;
> +        if (!vcpu->cpumask)
> +            continue;
> +        vcpu_idx = vcpuid;
> +        has_vcpu_pin = true;
> +    }
> +    /* Nothing to do */
> +    if (!has_vcpu_pin)
> +        return 0;
> +
> +    /* Adjust index */
> +    vcpu_idx++;
> +
> +    b_info->num_vcpu_hard_affinity = vcpu_idx;
> +    /* Will be released by libxl_domain_config_dispose */
> +    b_info->vcpu_hard_affinity = calloc(vcpu_idx, sizeof(libxl_bitmap));

Fails syntax-check. You'll need to use g_new0.

Patch 3 looks good, but I'll wait to push it until this one is ready. BTW, any 
reason for splitting 3 and 4? It's nice for review, but unless I'm missing 
something I think they should be squashed together.

Jim

> +    vcpu_affinity_array = b_info->vcpu_hard_affinity;
> +
> +    for (vcpuid = 0; vcpuid < vcpu_idx; vcpuid++) {
> +        libxl_bitmap *map = &vcpu_affinity_array[vcpuid];
> +        libxl_bitmap_init(map);
> +        /* libxl owns the bitmap */
> +        if (libxl_cpu_bitmap_alloc(ctx, map, 0))
> +            return -1;
> +        vcpu = virDomainDefGetVcpu(def, vcpuid);
> +        /* Apply the given mask, or allow unhandled vcpus to run anywhere */
> +        if (vcpu && vcpu->cpumask)
> +            virBitmapToDataBuf(vcpu->cpumask, map->map, map->size);
> +        else
> +            libxl_bitmap_set_any(map);
> +    }
> +    libxl_defbool_set(&b_info->numa_placement, false);
> +    return 0;
> +}
> +
>   static int
>   libxlMakeDomBuildInfo(virDomainDef *def,
>                         libxlDriverConfig *cfg,
> @@ -320,6 +370,9 @@ libxlMakeDomBuildInfo(virDomainDef *def,
>       for (i = 0; i < virDomainDefGetVcpus(def); i++)
>           libxl_bitmap_set((&b_info->avail_vcpus), i);
>   
> +    if (libxlDomainSetVcpuAffinities(def, ctx, b_info))
> +        return -1;
> +
>       switch ((virDomainClockOffsetType) clock.offset) {
>       case VIR_DOMAIN_CLOCK_OFFSET_VARIABLE:
>           if (clock.data.variable.basis == VIR_DOMAIN_CLOCK_BASIS_LOCALTIME)
>