Re: [PATCH v3 09/14] xen: sched: DOMCTL_*vcpuaffinity works with hard and soft affinity

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

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

So, about this, in the new version of this series I'm releasing right
now.

On mar, 2013-11-19 at 16:39 +0000, Jan Beulich wrote:
> >>> On 18.11.13 at 19:17, Dario Faggioli <dario.faggioli@citrix.com> wrote:
> > --- a/xen/common/domctl.c
> > +++ b/xen/common/domctl.c
> > @@ -617,19 +617,65 @@ 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);
> > +            if ( op->u.vcpuaffinity.flags & XEN_VCPUAFFINITY_SOFT )
> > +                ret = vcpu_set_soft_affinity(v, new_affinity);
> 
> You're discarding an eventual error indicator from
> vcpu_set_hard_affinity() here.
> 
I fixed this.

> > +
> > +            if ( ret )
> > +                goto setvcpuaffinity_out;
> 
> Considering that you're going to return an error here, the caller
> may expect that the call did nothing, even if
> vcpu_set_hard_affinity() succeeded and vcpu_set_soft_affinity()
> failed. I know this is ugly to handle...
> 
And this too.

> > +
> > +            /*
> > +             * Report back to the caller what the "effective affinity", that
> > +             * is the intersection of cpupool's pcpus, the (new?) hard
> > +             * affinity and the (new?) soft-affinity.
> > +             */
> > +            if ( !guest_handle_is_null(op->u.vcpuaffinity.eff_cpumap.bitmap) )
> >              {
> > -                ret = vcpu_set_affinity(v, new_affinity);
> > -                free_cpumask_var(new_affinity);
> > +                online = cpupool_online_cpumask(v->domain->cpupool);
> > +                cpumask_and(new_affinity, online, v->cpu_hard_affinity);
> > +                if ( op->u.vcpuaffinity.flags & XEN_VCPUAFFINITY_SOFT)
> > +                    cpumask_and(new_affinity, new_affinity,
> > +                                v->cpu_soft_affinity);
> > +
> > +                ret = cpumask_to_xenctl_bitmap(
> > +                    &op->u.vcpuaffinity.eff_cpumap, new_affinity);
> 
> Considering that you have two bitmaps available from the caller,
> can't you just return both when both flags are set?
> 
Well, it's true that there are two cpumaps, but only one is meant to be
an output parameter. Also, I think this is more useful like this, i.e.,
returning either:
 - hard-affinity&online
 - hard-affinity&soft-affinity&online,

As it can be seen in the libxl patch (in the new series).

Therefore, I kept this interface as it was here, also considering that:
 - it's pretty late to re-re-redesign;
 - neither this nor the xc one are stable interfaces, so we can come
   back and revisit this later, if we want to.

Do you think this could be acceptable?

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