Re: [PATCH v3 11/14] libxl: get and set soft affinity

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

On 11/18/2013 06:18 PM, Dario Faggioli wrote:
> Make space for two new cpumap-s, one in vcpu_info (for getting
> soft affinity) and build_info (for setting it). Provide two
> new API calls:
>
>   * libxl_set_vcpuaffinity2, taking a cpumap and setting either
>     hard, soft or both affinity to it, depending on 'flags';
>   * libxl_set_vcpuaffinity3, taking two cpumap, one for hard
>     and one for soft affinity.
>
> The bheavior of the existing libxl_set_vcpuaffinity is left
> unchanged, i.e., it only set hard affinity.
>
> Getting soft affinity happens indirectly, via `xl vcpu-list'
> (as it is already for hard affinity).
>
> The new calls include logic to check whether the affinity which
> will be used by Xen to schedule the vCPU(s) does actually match
> with the cpumap provided. In fact, we want to allow every possible
> combination of hard and soft affinities to be set, but we warn
> the user upon particularly weird combinations (e.g., hard and
> soft being disjoint sets of pCPUs).
>
> Also, this is the first change breaking the libxl ABI, so it
> bumps the MAJOR.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

The interface is fine with me (I would probably just have 2 and not 3, 
but I'm OK with both).  Just a few minor comments:

> @@ -973,6 +987,22 @@ int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
>                              libxl_bitmap *cpumap);
>   int libxl_set_vcpuaffinity_all(libxl_ctx *ctx, uint32_t domid,
>                                  unsigned int max_vcpus, libxl_bitmap *cpumap);
> +int libxl_set_vcpuaffinity2(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
> +                            const libxl_bitmap *cpumap, int flags);
> +int libxl_set_vcpuaffinity_all2(libxl_ctx *ctx, uint32_t domid,
> +                                unsigned int max_vcpus,
> +                                const libxl_bitmap *cpumap, int flags);

Should we have a bit more documentation about the behavior of "flags" 
somewhere?

> diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
> index b11cf28..fc3afee 100644
> --- a/tools/libxl/libxl_utils.h
> +++ b/tools/libxl/libxl_utils.h
> @@ -98,6 +98,21 @@ static inline int libxl_bitmap_cpu_valid(libxl_bitmap *bitmap, int bit)
>   #define libxl_for_each_set_bit(v, m) for (v = 0; v < (m).size * 8; v++) \
>                                                if (libxl_bitmap_test(&(m), v))
>
> +static inline int libxl_bitmap_equal(const libxl_bitmap *ba,
> +                                     const libxl_bitmap *bb,
> +                                     int nr_bits)
> +{
> +    int i;
> +
> +    /* Only check nr_bits (all bits if <= 0) */
> +    nr_bits = nr_bits <=0 ? ba->size * 8 : nr_bits;

The conditional should really have parenthesis around it, I'm pretty 
sure; and the spacing is inconsistent.

  -George