|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6 6/9] libxl: get and set soft affinity
On Wed, 2014-05-28 at 01:42 +0100, Dario Faggioli wrote:
> Make space for two new cpumap-s, one in vcpu_info (for getting
> soft affinity) and build_info (for setting it) and amend the
> API for setting vCPU affinity.
>
> libxl_set_vcpuaffinity() now takes two cpumaps, one for hard
> and one for soft affinity (LIBXL_API_VERSION is exploited to
> retain source level backword compatibility). Either of the
> two cpumap can be NULL, in which case, only the affinity
> corresponding to the non-NULL cpumap will be affected.
>
> Getting soft affinity happens indirectly, via `xl vcpu-list'
> (as it is already for hard affinity).
>
> This commit also introduces some logic to check whether the
> affinity which will be used by Xen to schedule the vCPU(s)
> does actually match with the cpumaps provided. In fact, we
> want to allow every possible combination of hard and soft
> affinity to be set, but we warn the user upon particularly
> weird situations (e.g., hard and soft being disjoint sets
> of pCPUs).
>
> This is the first change breaking the libxl ABI, so it bumps
> the MAJOR.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> ---
> Changes from v4:
> * get rid of inline stubs inside the LIBXL_API_VERSION_XXX
> block and just use define, as suggested during review
> * adapt to the new xc interface;
> * avoid leaking cpumap_soft in libxl_list_vcpu on error, as
> requested during review;
> * fix bogous `return 0' in libxl_set_vcpuaffinity, as
> requested during review;
> * clarify the comment for LIBXL_HAVE_SOFT_AFFINITY, as
> suggested during review;
> * renamed libxl_bitmap_valid() to libxl_bitmap_bit_valid(),
> as suggested uring review.
>
> Changes from v3:
> * only introduce one LIBXL_HAVE_ symbol for soft affinity,
> as requested during review;
> * use LIBXL_API_VERSION instead of having multiple version
> of the same function, as suggested during review;
> * use libxl_get_nr_cpus() rather than libxl_get_cputopology(),
> as suggested during review;
> * use LOGE() instead of LIBXL__LOG_ERRNO(), as requested
> during review;
> * kill the flags and use just one _set_vcpuaffinity()
> function with two cpumaps, allowing either of them to
> be NULL, as suggested during review;
> * avoid overflowing the bitmaps in libxl_bitmap_equal(),
> as suggested during review.
>
> Changes from v2:
> * interface completely redesigned, as discussed during
> review.
> ---
> tools/libxl/libxl.c | 85
> ++++++++++++++++++++++++++++++++++++++-----
> tools/libxl/libxl.h | 26 ++++++++++++-
> tools/libxl/libxl_create.c | 6 +++
> tools/libxl/libxl_dom.c | 3 +-
> tools/libxl/libxl_types.idl | 4 +-
> tools/libxl/libxl_utils.h | 25 ++++++++++++-
> tools/libxl/xl_cmdimpl.c | 6 +--
> 7 files changed, 137 insertions(+), 18 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index ec79645..309cc6f 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4587,6 +4590,12 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx,
> uint32_t domid,
> LOGE(ERROR, "getting vcpu affinity");
> goto err;
> }
> + if (xc_vcpu_getaffinity(ctx->xch, domid, *nr_vcpus_out, NULL,
> + ptr->cpumap_soft.map,
> + XEN_VCPUAFFINITY_SOFT) == -1) {
Can't this be combined with the call right before it which fetches the
hard aff'ty?
> + LOGE(ERROR, "getting vcpu affinity");
> + goto err;
> + }
> ptr->vcpuid = *nr_vcpus_out;
> ptr->cpu = vcpuinfo.cpu;
> ptr->online = !!vcpuinfo.online;
> int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
> - libxl_bitmap *cpumap)
> + const libxl_bitmap *cpumap_hard,
> + const libxl_bitmap *cpumap_soft)
> {
> - if (xc_vcpu_setaffinity(ctx->xch, domid, vcpuid, cpumap->map, NULL,
> - XEN_VCPUAFFINITY_HARD)) {
> - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "setting vcpu affinity");
> - return ERROR_FAIL;
> + GC_INIT(ctx);
> + libxl_bitmap cpumap;
> + int nr_cpus = 0, rc;
> +
> + if (!cpumap_hard && !cpumap_soft)
> + return ERROR_INVAL;
> +
> + nr_cpus = libxl_get_online_cpus(ctx);
> + if (nr_cpus < 0)
> + return nr_cpus;
> +
> + rc = libxl_cpu_bitmap_alloc(ctx, &cpumap, 0);
> + if (rc)
> + return rc;
> +
> + if (cpumap_hard) {
> + libxl_bitmap_copy(ctx, &cpumap, cpumap_hard);
> +
> + if (xc_vcpu_setaffinity(ctx->xch, domid, vcpuid, cpumap.map, NULL,
> + XEN_VCPUAFFINITY_HARD)) {
Again can't the soft and hard calls be combined?
> + LOGE(ERROR, "setting vcpu hard affinity");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> + if (!libxl_bitmap_equal(cpumap_hard, &cpumap, nr_cpus))
nr_cpus wasn't needed for the bitmap alloc or the bitmap copy, can we
not avoid it here too?
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index d015cf4..49a01a7 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -193,6 +193,12 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> libxl_bitmap_set_any(&b_info->cpumap);
> }
>
> + if (!b_info->cpumap_soft.size) {
> + if (libxl_cpu_bitmap_alloc(CTX, &b_info->cpumap_soft, 0))
> + return ERROR_FAIL;
> + libxl_bitmap_set_any(&b_info->cpumap_soft);
Could we not treat .size == 0 as being "any", indicating no need to do
anything later on?
> diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
> index e37fb89..4d743c9 100644
> --- a/tools/libxl/libxl_utils.h
> +++ b/tools/libxl/libxl_utils.h
> @@ -90,7 +90,7 @@ static inline void libxl_bitmap_set_none(libxl_bitmap
> *bitmap)
> {
> memset(bitmap->map, 0, bitmap->size);
> }
> -static inline int libxl_bitmap_cpu_valid(libxl_bitmap *bitmap, int bit)
> +static inline int libxl_bitmap_bit_valid(const libxl_bitmap *bitmap, int bit)
> {
> return bit >= 0 && bit < (bitmap->size * 8);
> }
> @@ -98,6 +98,29 @@ 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;
if (nr_bits <= 0)
nr_bits = ba->size * 8;
I'm not sure <= 0 is useful in this interface, just == would do. nr_bits
could be unsigned too I guess.
> + for (i = 0; i < nr_bits; i++) {
> + /* If overflowing one of the bitmaps, we call them different */
Could this not be checked a priori by looking at ba->size and bb->size
up front rather than every time round the loop?
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5195914..fa5ab8e 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -2269,7 +2269,7 @@ start:
> } else {
> libxl_bitmap_set_any(&vcpu_cpumap);
> }
> - if (libxl_set_vcpuaffinity(ctx, domid, i, &vcpu_cpumap)) {
> + if (libxl_set_vcpuaffinity(ctx, domid, i, &vcpu_cpumap, NULL)) {
> fprintf(stderr, "setting affinity failed on vcpu `%d'.\n",
> i);
> libxl_bitmap_dispose(&vcpu_cpumap);
> free(vcpu_to_pcpu);
> @@ -4700,7 +4700,7 @@ static int vcpupin(uint32_t domid, const char *vcpu,
> char *cpu)
> }
>
> if (vcpuid != -1) {
> - if (libxl_set_vcpuaffinity(ctx, domid, vcpuid, &cpumap) == -1) {
> + if (libxl_set_vcpuaffinity(ctx, domid, vcpuid, &cpumap, NULL)) {
Are you deliberately changing the error handling here? (and in the next
hunk)
Ian.
> fprintf(stderr, "Could not set affinity for vcpu `%u'.\n",
> vcpuid);
> goto out;
> }
> @@ -4712,7 +4712,7 @@ static int vcpupin(uint32_t domid, const char *vcpu,
> char *cpu)
> }
> for (i = 0; i < nb_vcpu; i++) {
> if (libxl_set_vcpuaffinity(ctx, domid, vcpuinfo[i].vcpuid,
> - &cpumap) == -1) {
> + &cpumap, NULL)) {
> fprintf(stderr, "libxl_set_vcpuaffinity failed"
> " on vcpu `%u'.\n", vcpuinfo[i].vcpuid);
> }
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |