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

[George Dunlap <george.dunlap@eu.citrix.com>]

On 13/11/13 19:12, Dario Faggioli wrote:
> by adding a flag for the caller to specify which one he cares about.
>
> Add also another cpumap there. This way, in case of
> DOMCTL_setvcpuaffinity, Xen can return back to the caller the
> "effective affinity" of the vcpu. We call the effective affinity
> the intersection between cpupool's cpus, the new hard affinity
> and (if asking to set soft affinity) the new soft affinity.
>
> The purpose of this is allowing the toolstack to figure out whether
> or not the requested change in affinity produced sensible results,
> when combined with the other settings that are already in place.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
>   tools/libxc/xc_domain.c     |    4 +++-
>   xen/arch/x86/traps.c        |    4 ++--
>   xen/common/domctl.c         |   38 ++++++++++++++++++++++++++++++++++----
>   xen/common/schedule.c       |   35 ++++++++++++++++++++++++-----------
>   xen/common/wait.c           |    6 +++---
>   xen/include/public/domctl.h |   15 +++++++++++++--
>   xen/include/xen/sched.h     |    3 ++-
>   7 files changed, 81 insertions(+), 24 deletions(-)
>
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index 1ccafc5..f9ae4bf 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -215,7 +215,9 @@ int xc_vcpu_setaffinity(xc_interface *xch,
>   
>       domctl.cmd = XEN_DOMCTL_setvcpuaffinity;
>       domctl.domain = (domid_t)domid;
> -    domctl.u.vcpuaffinity.vcpu    = vcpu;
> +    domctl.u.vcpuaffinity.vcpu = vcpu;
> +    /* Soft affinity is there, but not used anywhere for now, so... */
> +    domctl.u.vcpuaffinity.flags = XEN_VCPUAFFINITY_HARD;
>   
>       memcpy(local, cpumap, cpusize);
>   
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 4279cad..196ff68 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -3093,7 +3093,7 @@ static void nmi_mce_softirq(void)
>            * Make sure to wakeup the vcpu on the
>            * specified processor.
>            */
> -        vcpu_set_affinity(st->vcpu, cpumask_of(st->processor));
> +        vcpu_set_hard_affinity(st->vcpu, cpumask_of(st->processor));
>   
>           /* Affinity is restored in the iret hypercall. */
>       }
> @@ -3122,7 +3122,7 @@ void async_exception_cleanup(struct vcpu *curr)
>       if ( !cpumask_empty(curr->cpu_hard_affinity_tmp) &&
>            !cpumask_equal(curr->cpu_hard_affinity_tmp, curr->cpu_hard_affinity) )
>       {
> -        vcpu_set_affinity(curr, curr->cpu_hard_affinity_tmp);
> +        vcpu_set_hard_affinity(curr, curr->cpu_hard_affinity_tmp);
>           cpumask_clear(curr->cpu_hard_affinity_tmp);
>       }
>   
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 5e0ac5c..b3f779e 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -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?

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

> +
> +            if ( ret )
>               {
> -                ret = vcpu_set_affinity(v, new_affinity);
>                   free_cpumask_var(new_affinity);
> +                break;
>               }
> +
> +            /*
> +             * 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.
> +             */
> +            online = cpupool_online_cpumask(v->domain->cpupool);
> +            cpumask_and(new_affinity, online, v->cpu_hard_affinity);
> +            cpumask_and(new_affinity, new_affinity , v->cpu_soft_affinity);
> +
> +            ret = cpumask_to_xenctl_bitmap(
> +                &op->u.vcpuaffinity.effective_affinity, new_affinity);
> +
> +            free_cpumask_var(new_affinity);
>           }
>           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.

Other than that, I think this looks good.

>           }
>       }
>       break;
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 28099d6..bb52366 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -653,22 +653,14 @@ void sched_set_node_affinity(struct domain *d, nodemask_t *mask)
>       SCHED_OP(DOM2OP(d), set_node_affinity, d, mask);
>   }
>   
> -int vcpu_set_affinity(struct vcpu *v, const cpumask_t *affinity)
> +static int vcpu_set_affinity(
> +    struct vcpu *v, const cpumask_t *affinity, cpumask_t **which)
>   {
> -    cpumask_t online_affinity;
> -    cpumask_t *online;
>       spinlock_t *lock;
>   
> -    if ( v->domain->is_pinned )
> -        return -EINVAL;
> -    online = VCPU2ONLINE(v);
> -    cpumask_and(&online_affinity, affinity, online);
> -    if ( cpumask_empty(&online_affinity) )
> -        return -EINVAL;
> -
>       lock = vcpu_schedule_lock_irq(v);
>   
> -    cpumask_copy(v->cpu_hard_affinity, affinity);
> +    cpumask_copy(*which, affinity);
>   
>       /* Always ask the scheduler to re-evaluate placement
>        * when changing the affinity */
> @@ -687,6 +679,27 @@ int vcpu_set_affinity(struct vcpu *v, const cpumask_t *affinity)
>       return 0;
>   }
>   
> +int vcpu_set_hard_affinity(struct vcpu *v, const cpumask_t *affinity)
> +{
> +    cpumask_t online_affinity;
> +    cpumask_t *online;
> +
> +    if ( v->domain->is_pinned )
> +        return -EINVAL;
> +
> +    online = VCPU2ONLINE(v);
> +    cpumask_and(&online_affinity, affinity, online);
> +    if ( cpumask_empty(&online_affinity) )
> +        return -EINVAL;
> +
> +    return vcpu_set_affinity(v, affinity, &v->cpu_hard_affinity);
> +}
> +
> +int vcpu_set_soft_affinity(struct vcpu *v, const cpumask_t *affinity)
> +{
> +    return vcpu_set_affinity(v, affinity, &v->cpu_soft_affinity);
> +}
> +
>   /* Block the currently-executing domain until a pertinent event occurs. */
>   void vcpu_block(void)
>   {
> diff --git a/xen/common/wait.c b/xen/common/wait.c
> index 3f6ff41..1f6b597 100644
> --- a/xen/common/wait.c
> +++ b/xen/common/wait.c
> @@ -135,7 +135,7 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
>       /* Save current VCPU affinity; force wakeup on *this* CPU only. */
>       wqv->wakeup_cpu = smp_processor_id();
>       cpumask_copy(&wqv->saved_affinity, curr->cpu_hard_affinity);
> -    if ( vcpu_set_affinity(curr, cpumask_of(wqv->wakeup_cpu)) )
> +    if ( vcpu_set_hard_affinity(curr, cpumask_of(wqv->wakeup_cpu)) )
>       {
>           gdprintk(XENLOG_ERR, "Unable to set vcpu affinity\n");
>           domain_crash_synchronous();
> @@ -166,7 +166,7 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
>   static void __finish_wait(struct waitqueue_vcpu *wqv)
>   {
>       wqv->esp = NULL;
> -    (void)vcpu_set_affinity(current, &wqv->saved_affinity);
> +    (void)vcpu_set_hard_affinity(current, &wqv->saved_affinity);
>   }
>   
>   void check_wakeup_from_wait(void)
> @@ -184,7 +184,7 @@ void check_wakeup_from_wait(void)
>           /* Re-set VCPU affinity and re-enter the scheduler. */
>           struct vcpu *curr = current;
>           cpumask_copy(&wqv->saved_affinity, curr->cpu_hard_affinity);
> -        if ( vcpu_set_affinity(curr, cpumask_of(wqv->wakeup_cpu)) )
> +        if ( vcpu_set_hard_affinity(curr, cpumask_of(wqv->wakeup_cpu)) )
>           {
>               gdprintk(XENLOG_ERR, "Unable to set vcpu affinity\n");
>               domain_crash_synchronous();
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 01a3652..aed9cd4 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -300,8 +300,19 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_nodeaffinity_t);
>   /* XEN_DOMCTL_setvcpuaffinity */
>   /* XEN_DOMCTL_getvcpuaffinity */
>   struct xen_domctl_vcpuaffinity {
> -    uint32_t  vcpu;              /* IN */
> -    struct xenctl_bitmap cpumap; /* IN/OUT */
> +    /* IN variables. */
> +    uint32_t  vcpu;
> + /* Set/get the hard affinity for vcpu */
> +#define _XEN_VCPUAFFINITY_HARD  0
> +#define XEN_VCPUAFFINITY_HARD   (1U<<_XEN_VCPUAFFINITY_HARD)
> + /* Set/get the soft affinity for vcpu */
> +#define _XEN_VCPUAFFINITY_SOFT  1
> +#define XEN_VCPUAFFINITY_SOFT   (1U<<_XEN_VCPUAFFINITY_SOFT)
> +    uint32_t flags;
> +    /* IN/OUT variables. */
> +    struct xenctl_bitmap cpumap;
> +    /* OUT variables. */
> +    struct xenctl_bitmap effective_affinity;
>   };
>   typedef struct xen_domctl_vcpuaffinity xen_domctl_vcpuaffinity_t;
>   DEFINE_XEN_GUEST_HANDLE(xen_domctl_vcpuaffinity_t);
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 3575312..0f728b3 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -755,7 +755,8 @@ void scheduler_free(struct scheduler *sched);
>   int schedule_cpu_switch(unsigned int cpu, struct cpupool *c);
>   void vcpu_force_reschedule(struct vcpu *v);
>   int cpu_disable_scheduler(unsigned int cpu);
> -int vcpu_set_affinity(struct vcpu *v, const cpumask_t *affinity);
> +int vcpu_set_hard_affinity(struct vcpu *v, const cpumask_t *affinity);
> +int vcpu_set_soft_affinity(struct vcpu *v, const cpumask_t *affinity);
>   void restore_vcpu_affinity(struct domain *d);
>   
>   void vcpu_runstate_get(struct vcpu *v, struct vcpu_runstate_info *runstate);
>