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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.12.13 at 19:29, Dario Faggioli <dario.faggioli@xxxxxxxxxx> 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.

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

Further - can't this be done in the code shared between "get" and
"set"?

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

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

Jan

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