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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Fri, 2014-06-06 at 09:46 +0100, Ian Campbell wrote:
> 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.
> 
Ok. No, I was not thinking about that. Well, let's say I was not
considering the described situation so much of an issue. However, I
think I agree. If there is more than one entity changing the affinity of
a vcpu at the same time, the result will always be  quite unpredictable,
but having at least the changes requested from each side performed
atomically looks more consistent.

I'll do it that way, thanks.

> 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?
> 
Indeed I can (I've got it ready already, I've tested it, and it works.
Will send the patches on Monday). I was talking about how the error
handling was implemented in v6, meaning that, if you don't consider
having a transient state a big deal, it's easier to implement it with
two calls. However, as I said, I now agree it is kind of a big deal, so
I've done it with just one, and it all works (error handling included).

> > 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?
>
When I read this, I liked it. However, it turned out there is no real
need for it (see below).

> > 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.
> 
So, I went for this second semantic. It's more compact and easy to read,
it's consistent, and it does not require to neither specify nor retrieve
via hypercalls the number of online pcpus... It just works. :-)

Thanks and 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
Description: This is a digitally signed message part

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