Re: [Xen-devel] [PATCH v6 6/9] libxl: get and set soft affinity

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-06-05 at 14:59 +0200, Dario Faggioli wrote:
> > > +    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?

My main concern would be transiently invalid configurations between the
two calls, doing both at once allows the hypervisor to make an atomic
decision.

I'm not sure why the error handling is impacted, is there some sort of
deficiency in the new interface? I thought you got back the effective
map for both hard and soft (if you give them) and can therefore make all
the same checks etc as you make today?

> 
> > > +            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.

Do cpumap_hard and &cpumap not each contain their own limits though?

This function is really some sort of libxl_bitmap_prefix_equal (although
prefix isn't quite the right word I think).

> 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'.

Because alloc and copy are libxl_cpumap_foo instead of bitmap foo, so
they can derive this internally? Could libxl_cpumap_equal be made and
behave the same?
> 
> 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.

A zero size cpumap would indicate no restrictions.

>  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?

OK.
> > 
> > > +    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) */

Could be part of the function doc comment rather than buried in the
implementation?

>     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;

What if bb->size > ba->size? This test will then pass, incorrectly I
think.

If your intention is that bitmaps which are not exactly equal in size be
considered strictly different then I think you should just compare
ba->size and bb->size directly.

Of course this is not the only possible semantic. You could also declare
that bits past the end of the bitmap are considered to have some
specific value (probably that should be 0) and compare on that basis. 

I mention the second possible semantic because this matches the
behaviour of libxl_bitmap_test I think.

The question is should e.g. [0,1,0] and [0,1] be considered equal. Given
the existing semantics of libxl_bitmap_test it seems like perhaps they
should be.

> > > 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. :-)

A mention of this change in the commit log and/or an update to the doc
comment (if it existed) would be sufficient.

Ian.


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