Re: [PATCH v5 12/17] xen/libxc: sched: DOMCTL_*vcpuaffinity works with hard and soft affinity

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 9162 bytes --]

On mar, 2013-12-03 at 10:02 +0000, Jan Beulich wrote:
> >>> On 02.12.13 at 19:29, Dario Faggioli <dario.faggioli@citrix.com> wrote:
> > +static inline
> > +int vcpuaffinity_params_invalid(const xen_domctl_vcpuaffinity_t *vcpuaff)
> > +{
> > +    return vcpuaff->flags == 0 ||
> > +           (vcpuaff->flags & XEN_VCPUAFFINITY_HARD &&
> > +            guest_handle_is_null(vcpuaff->cpumap_hard.bitmap)) ||
> > +           (vcpuaff->flags & XEN_VCPUAFFINITY_SOFT &&
> 
> The binary &-s clearly want to be parenthesized.
> 
Ok.

> > @@ -605,31 +615,112 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> >      case XEN_DOMCTL_getvcpuaffinity:
> >      {
> >          struct vcpu *v;
> > +        xen_domctl_vcpuaffinity_t *vcpuaff = &op->u.vcpuaffinity;
> >  
> >          ret = -EINVAL;
> > -        if ( op->u.vcpuaffinity.vcpu >= d->max_vcpus )
> > +        if ( vcpuaff->vcpu >= d->max_vcpus )
> >              break;
> >  
> >          ret = -ESRCH;
> > -        if ( (v = d->vcpu[op->u.vcpuaffinity.vcpu]) == NULL )
> > +        if ( (v = d->vcpu[vcpuaff->vcpu]) == NULL )
> >              break;
> >  
> >          if ( op->cmd == XEN_DOMCTL_setvcpuaffinity )
> >          {
> > -            cpumask_var_t new_affinity;
> > +            cpumask_var_t new_affinity, old_affinity;
> > +            cpumask_t *online = cpupool_online_cpumask(v->domain->cpupool);;
> > +
> > +            /*
> > +             * We want to be able to restore hard affinity if we are trying
> > +             * setting both and changing soft affinity (which happens later,
> > +             * when hard affinity has been succesfully chaged already) fails.
> > +             */
> > +            if ( !alloc_cpumask_var(&old_affinity) )
> > +            {
> > +                ret = -ENOMEM;
> > +                break;
> > +            }
> > +            cpumask_copy(old_affinity, v->cpu_hard_affinity);
> >  
> > -            ret = xenctl_bitmap_to_cpumask(
> > -                &new_affinity, &op->u.vcpuaffinity.cpumap);
> > -            if ( !ret )
> > +            if ( !alloc_cpumask_var(&new_affinity) )
> >              {
> > -                ret = vcpu_set_affinity(v, new_affinity);
> > -                free_cpumask_var(new_affinity);
> > +                free_cpumask_var(old_affinity);
> > +                ret = -ENOMEM;
> > +                break;
> >              }
> > +
> > +            ret = -EINVAL;
> > +            if (vcpuaffinity_params_invalid(vcpuaff))
> 
> Coding style.
> 
Ah, sure, sorry.

> Further - can't this be done in the code shared between "get" and
> "set"?
> 
It certainly can be, yes.

> > +                goto setvcpuaffinity_out;
> > +
> > +            /*
> > +             * We both set a new affinity and report back to the caller what
> > +             * the scheduler will be effectively using.
> > +             */
> > +            if ( vcpuaff->flags & XEN_VCPUAFFINITY_HARD )
> > +            {
> > +                ret = xenctl_bitmap_to_bitmap(cpumask_bits(new_affinity),
> > +                                              &vcpuaff->cpumap_hard,
> > +                                              vcpuaff->cpumap_hard.nr_bits);
> 
> There's no code above range checking vcpuaff->cpumap_hard.nr_bits,
> yet xenctl_bitmap_to_bitmap() uses the passed in value to write into
> the array pointed to by the first argument. Why is this not
> xenctl_bitmap_to_cpumask() in the first place?
>
Because xenctl_bitmap_to_cpumask() allocate the cpumask, which I don't
want, since I already did that above.

What xenctl_bitmap_to_cpumask() does is allocate the cpumask and then
call xenctl_bitmap_to_bitmap() although, yes, it uses nr_cpu_ids for
nbits, not what comes from outside, which I overlooked while adapting
this to the new design we agreed upon.

Thanks for pointing this out, I'll fix it.

> > +                if ( !ret )
> > +                    ret = vcpu_set_hard_affinity(v, new_affinity);
> > +                if ( ret )
> > +                    goto setvcpuaffinity_out;
> > +
> > +                /*
> > +                 * For hard affinity, what we return is the intersection of
> > +                 * cpupool's online mask and the new hard affinity.
> > +                 */
> > +                cpumask_and(new_affinity, online, v->cpu_hard_affinity);
> > +                ret = cpumask_to_xenctl_bitmap(&vcpuaff->cpumap_hard,
> > +                                               new_affinity);
> 
> So what you return back here ...
> 
> > +            }
> > +            if ( vcpuaff->flags & XEN_VCPUAFFINITY_SOFT )
> > +            {
> > +                ret = xenctl_bitmap_to_bitmap(cpumask_bits(new_affinity),
> > +                                              &vcpuaff->cpumap_soft,
> > +                                              vcpuaff->cpumap_soft.nr_bits);
> > +                if ( !ret)
> > +                    ret = vcpu_set_soft_affinity(v, new_affinity);
> > +                if ( ret )
> > +                {
> > +                    /*
> > +                     * Since we're returning error, the caller expects nothing
> > +                     * happened, so we rollback the changes to hard affinity
> > +                     * (if any).
> > +                     */
> > +                    if ( vcpuaff->flags & XEN_VCPUAFFINITY_HARD )
> > +                        vcpu_set_hard_affinity(v, old_affinity);
> > +                    goto setvcpuaffinity_out;
> > +                }
> > +
> > +                /*
> > +                 * For soft affinity, we return the intersection between the
> > +                 * new soft affinity, the cpupool's online map and the (new)
> > +                 * hard affinity.
> > +                 */
> > +                cpumask_and(new_affinity, new_affinity, online);
> > +                cpumask_and(new_affinity, new_affinity, v->cpu_hard_affinity);
> > +                ret = cpumask_to_xenctl_bitmap(&vcpuaff->cpumap_soft,
> > +                                               new_affinity);
> 
> ... and here differs from ...
> 
> > +            }
> > +
> > + setvcpuaffinity_out:
> > +            free_cpumask_var(new_affinity);
> > +            free_cpumask_var(old_affinity);
> >          }
> >          else
> >          {
> > -            ret = cpumask_to_xenctl_bitmap(
> > -                &op->u.vcpuaffinity.cpumap, v->cpu_hard_affinity);
> > +            ret = -EINVAL;
> > +            if (vcpuaffinity_params_invalid(vcpuaff))
> > +                break;
> > +
> > +            if ( vcpuaff->flags & XEN_VCPUAFFINITY_HARD )
> > +                ret = cpumask_to_xenctl_bitmap(&vcpuaff->cpumap_hard,
> > +                                               v->cpu_hard_affinity);
> > +            if ( vcpuaff->flags & XEN_VCPUAFFINITY_SOFT )
> > +                ret = cpumask_to_xenctl_bitmap(&vcpuaff->cpumap_soft,
> > +                                               v->cpu_soft_affinity);
> 
> ... the simple "get". Why? And if really intended, this should be
> mentioned in the public header.
> 
The main reason is that I asked you what you think I should be returning
(in setvcpuaffinity) and that's what you told me. :-)

http://bugs.xenproject.org/xen/mid/%3C52932DCB020000780010673E@nat28.tlf.novell.com%3E

Well, actually, I only asked what we should be returning from
DOMCTL_vcpu_setaffinity, and never mentioned DOMCTL_vcpu_getaffinity
because I seriously thought there was no need to discuss what the latter
should be returning, and whether or not the two return values should
match.

I thought, and still think, it's pretty obvious there must be a way for
the user to retrieve what he has set, independently on the various
intersections between online, hard affinity, etc, mustn't it? Well, that
way is using DOMCTL_get_vcpuaffinity.

Actually, if DOMCTL_vcpu_setaffinity an DOMCTL_vcpu_getaffinity would be
returning the exact same thing, why are we bothering having both doing
that? It would have been pretty easy to just, in the toolstack, issue a
call to _get_ right after a _set_ and check. No, the point was to have
_set_ return something actually useful, i.e., something that can't be
retrieved in any other way, as an indication of how effective the set
operation itself was. That, by definition, makes what _set_ and _get_
returns different, and it may be my fault to no have mentioned this
clearly in our earlier discussions, but I really was giving it for
granted. :-/

That being said, I of course can say something about this in some
docs/header. Actually, the following patch does that quite thoroughly, I
think, in tools/libxc/xenctrl.h. I can certainly add something similar
in Xen's public .h files.

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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