|
[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
So, I was almost ready to resend this series... but thinking a bit more
it looks to me that there are some comments in this e-mail that are
worth a little bit more discussion...
On mer, 2014-05-28 at 16:23 +0100, Ian Campbell wrote:
> 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>
>
> > 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?
>
In this case, yes, I think it can.
> > + 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?
>
Here, OTOH, I rather keep the two calls. Of course it became (thanks to
the previous patch) possible to set both the affinities at once, but
given the way both the arguments (to the xc_xxx calls) and the error is
handled, I think it's better to keep the two cases separate.
Would that be fine?
> > + 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?
>
Hehe... Using it here is the only reason why I need it. In fact (if you
remember), we discussed quite a bit about the fact that, when wanting to
compare two cpumaps, we need to know the actual number of online pCPUs,
to know where to stop comparing.
The reason why no such thing is required in alloc and copy is that,
there, 0 is used to mean 'as big as possible' and/or 'all of it'.
Does this make sense?
> > 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?
>
I'm not sure I understand. The above is the exact same way the cpumap
for the hard (and only, up to this patch) affinity is handled. Digging
with annotate, this seems to date back to the commit that firstly
introduced libxl__domain_build_info_setdefault().
I guess it would be possible to change how we want it to be interpreted
and handled, but that would involve quite a few spots, I think... So,
perhaps a different patch?
In fact, I think there are a few places where it is expected for (both)
the cpumap(s) to be allocated already, and that go ahead using them,
instead of checking .size.
Did I get what you actually were asking? Do you agree that this would be
a separate patch?
> > 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?
>
This is how this looks now in my tree:
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) */
if (nr_bits == 0)
nr_bits = ba->size * 8;
/* If overflowing one of the bitmaps, we call them different */
if (!libxl_bitmap_bit_valid(ba, nr_bits-1) ||
!libxl_bitmap_bit_valid(bb, nr_bits-1))
return 0;
for (i = 0; i < nr_bits; i++) {
if (libxl_bitmap_test(ba, i) != libxl_bitmap_test(bb, i))
return 0;
}
return 1;
}
Any better?
> > 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)
>
Well, I am, as I don't see why only '-1' should be checked, instead of a
more general failure. It might seem out of scope here, but this very
patch basically rewrite libxl_set_vcpuaffinity(), so it does make sense
to me to do this here... Advise otherwise, and I'll do it as you
ask. :-)
Thanks for the detailed review. These are the only points separating me
from a repost. I'll send v7 of the series as soon as we reach consensus
on how to deal with them.
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
Attachment:
signature.asc _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |