Re: [PATCH v2 3/6] xen: sched: clarify use cases of schedule_cpu_switch()

[Juergen Gross <jgross@suse.com>]

On 10/14/2015 05:54 PM, Dario Faggioli wrote:
> schedule_cpu_switch() is meant to be only used for moving
> pCPUs from a cpupool to no cpupool, and from there back
> to a cpupool, *not* to move them directly from one cpupool
> to another.
>
> This is something that is reflected in the way it is
> implemented, and should be kept in mind when looking at
> it. However, that is not that clear, by just the look of
> it.
>
> Make it more evident by adding commentary and ASSERT()s.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> ---
> Changes from v1:
>   * new patch, was not there in v1.
> ---
>   xen/common/schedule.c |   28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
>
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 9aa209d..5ebfa33 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1486,12 +1486,40 @@ void __init scheduler_init(void)
>           BUG();
>   }
>
> +/*
> + * Move a pCPU outside of the influence of the scheduler of its current
> + * cpupool, or subject it to the scheduler of a new cpupool.
> + *
> + * For the pCPUs that are removed from their cpupool, their scheduler becomes
> + * &ops (the default scheduler, selected at boot, which also services the
> + * default cpupool). However, as these pCPUs are not really part of any pool,
> + * there won't be any scheduling event on them, not even from the default
> + * scheduler. Basically, they will just sit idle until they are explicitly
> + * added back to a cpupool.
> + */
>   int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
>   {
>       struct vcpu *idle;
>       void *ppriv, *ppriv_old, *vpriv, *vpriv_old;
>       struct scheduler *old_ops = per_cpu(scheduler, cpu);
>       struct scheduler *new_ops = (c == NULL) ? &ops : c->sched;
> +    struct cpupool *pool = per_cpu(cpupool, cpu);
> +
> +    /*
> +     * pCPUs only move from a valid cpupool to free (i.e., out of any pool),
> +     * or from free to a valid cpupool. In the former case (which happens when
> +     * c is NULL), we want the CPU to have been marked as free already, as
> +     * well as to not be valid for the source pool any longer, when we get to
> +     * here. In the latter case (which happens when c is a valid cpupool), we
> +     * want the CPU to still be marked as free, as well as to not yet be valid
> +     * for the destination pool.
> +     * This all is because we do not want any scheduling activity on the CPU
> +     * while, in here, we switch things over.

While this is correct I think you should mention that in the "adding to
a pool" case per_cpu(cpupool, cpu) already contains the new pool
pointer. So c == pool then and both are not NULL.

In fact pool is never NULL, c might be.

Maybe it would be a good idea to move setting of per_cpu(cpupool, cpu)
into schedule_cpu_switch(). Originally I didn't do that to avoid
spreading too much cpupool related actions outside of cpupool.c. But
with those ASSERT()s added hiding that action will cause more confusion
than having the modification of per_cpu(cpupool, cpu) here.

When doing the code movement the current behaviour regarding sequence
of changes must be kept, of course. So when adding the cpu to a pool
the cpupool information must be set _before_ taking the scheduler lock,
while when removing this must happen after release of the lock.

> +     */
> +    ASSERT(c != NULL || pool != NULL);
> +    ASSERT(cpumask_test_cpu(cpu, &cpupool_free_cpus));
> +    ASSERT((c == NULL && !cpumask_test_cpu(cpu, pool->cpu_valid)) ||
> +           (c != NULL && !cpumask_test_cpu(cpu, c->cpu_valid)));
>
>       if ( old_ops == new_ops )
>           return 0;


Juergen