Re: [Xen-devel] [PATCH v2 09/16] xen: sched: DOMCTL_*vcpuaffinity works with hard and soft affinity

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On gio, 2013-11-14 at 14:42 +0000, George Dunlap wrote:
> On 13/11/13 19:12, Dario Faggioli wrote:
> > @@ -617,19 +617,49 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> > u_domctl)
> >           if ( op->cmd == XEN_DOMCTL_setvcpuaffinity )
> >           {
> >               cpumask_var_t new_affinity;
> > +            cpumask_t *online;
> >   
> >               ret = xenctl_bitmap_to_cpumask(
> >                   &new_affinity, &op->u.vcpuaffinity.cpumap);
> > -            if ( !ret )
> > +            if ( ret )
> > +                break;
> > +
> > +            ret = -EINVAL;
> > +            if ( op->u.vcpuaffinity.flags & XEN_VCPUAFFINITY_HARD )
> > +                ret = vcpu_set_hard_affinity(v, new_affinity);
> > +            else if ( op->u.vcpuaffinity.flags & XEN_VCPUAFFINITY_SOFT )
> > +                ret = vcpu_set_soft_affinity(v, new_affinity);
> 
> Wait, why are we making this a bitmask of flags, if we can only set one 
> at a time?  Shouldn't it instead be a simple enum?
> 
Right. I had a split mind about which one way to go between the ones you
outline here and, apparently, ended up with the worst possible
combination, i.e., something in the middle! :-P

> Or alternately (which is what I was expecting when I saw it was 
> 'flags'), shouldn't it allow you to set both at the same time? (i.e., 
> take away the 'else' here?)
> 
Indeed. I'll take this path.

> >           else
> >           {
> > -            ret = cpumask_to_xenctl_bitmap(
> > -                &op->u.vcpuaffinity.cpumap, v->cpu_hard_affinity);
> > +            if ( op->u.vcpuaffinity.flags & XEN_VCPUAFFINITY_HARD )
> > +                ret = cpumask_to_xenctl_bitmap(
> > +                    &op->u.vcpuaffinity.cpumap, v->cpu_hard_affinity);
> > +            else if ( op->u.vcpuaffinity.flags & XEN_VCPUAFFINITY_SOFT )
> > +                ret = cpumask_to_xenctl_bitmap(
> > +                    &op->u.vcpuaffinity.cpumap, v->cpu_soft_affinity);
> > +            else
> > +               ret = -EINVAL;
> 
> Asking for both the hard and soft affinities at the same time shouldn't 
> return just the hard affinity.  It should either return an error, or do 
> something interesting like return the intersection of the two.
> 
Right again. Given what we said above, I think I'll go for the
intersection.

> Other than that, I think this looks good.
> 
Cool, I'll respin with these changes ASAP.

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