Re: [PATCH 4/9] xen: sched: add .init_pdata hook to the scheduler interface

[Juergen Gross <jgross@suse.com>]

On 09/29/2015 06:55 PM, Dario Faggioli wrote:
> with the purpose of decoupling the allocation phase and
> the initialization one, for per-pCPU data of the schedulers.
>
> This makes it possible to perform the initialization later
> in the pCPU bringup/assignement process, when more information
> (for instance, the host CPU topology) are available. This,
> for now, is important only for Credit2, but it can well be
> useful to other schedulers.
>
> This also fixes a latent bug. In fact, when moving a pCPU
> from a Credit2 cpupool to another, whose scheduler also
> remaps runqueue spin locks (e.g., RTDS), we experience the
> following Oops:
>
> (XEN) Initializing RTDS scheduler
> (XEN) WARNING: This is experimental software in development.
> (XEN) Use at your own risk.
> (XEN) Removing cpu 6 from runqueue 0
> (XEN) Assertion 'sd->schedule_lock == &rqd->lock' failed at sched_credit2.c:2102
> (XEN) ----[ Xen-4.7-unstable  x86_64  debug=y  Not tainted ]----
> ... ... ...
> (XEN) Xen call trace:
> (XEN)    [<ffff82d0801256fa>] csched2_free_pdata+0x135/0x180
> (XEN)    [<ffff82d08012ccad>] schedule_cpu_switch+0x1ee/0x217
> (XEN)    [<ffff82d080101c34>] cpupool_assign_cpu_locked+0x4d/0x152
> (XEN)    [<ffff82d0801024ad>] cpupool_do_sysctl+0x20b/0x69d
> (XEN)    [<ffff82d08012f693>] do_sysctl+0x665/0x10f8
> (XEN)    [<ffff82d08023cb86>] lstar_enter+0x106/0x160
>
> This happens because, right now, the scheduler of the
> target pool remaps the runqueue lock during (rt_)alloc_pdata,
> which is called at the very beginning of schedule_cpu_switch().
> Credit2's csched2_free_pdata(), however, wants to find the spin
> lock the same way it was put by csched2_alloc_pdata(), and
> explodes, it not being the case.
>
> This patch also fixes this as, now, spin lock remapping
> happens in the init_pdata hook of the target scheduler for
> the pCPU, which we can easily make sure it is ivoked *after*
> the free_pdata hook of the old scheduler (which is exactly
> what is done in this patch), without needing to restructure
> schedule_cpu_switch().
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Juergen Gross <jgross@ssue.com>
> ---
>   xen/common/schedule.c      |   16 +++++++++++++---
>   xen/include/xen/sched-if.h |    1 +
>   2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 4a89222..83244d7 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1407,6 +1407,9 @@ static int cpu_schedule_callback(
>
>       switch ( action )
>       {
> +    case CPU_STARTING:
> +        SCHED_OP(&ops, init_pdata, cpu);
> +        break;
>       case CPU_UP_PREPARE:
>           rc = cpu_schedule_up(cpu);
>           break;
> @@ -1484,6 +1487,7 @@ void __init scheduler_init(void)
>       if ( ops.alloc_pdata &&
>            !(this_cpu(schedule_data).sched_priv = ops.alloc_pdata(&ops, 0)) )
>           BUG();
> +    SCHED_OP(&ops, init_pdata, 0);

You can't call this without having it set in all schedulers.

I guess using the init_pdata hook has to be introduced after or in
patch 5.


Juergen

>   }
>
>   int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
> @@ -1513,12 +1517,18 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
>       per_cpu(scheduler, cpu) = new_ops;
>       ppriv_old = per_cpu(schedule_data, cpu).sched_priv;
>       per_cpu(schedule_data, cpu).sched_priv = ppriv;
> -    SCHED_OP(new_ops, tick_resume, cpu);
> -    SCHED_OP(new_ops, insert_vcpu, idle);
> -
>       SCHED_OP(old_ops, free_vdata, vpriv_old);
>       SCHED_OP(old_ops, free_pdata, ppriv_old, cpu);
>
> +    /*
> +     * Now that the new pcpu data have been allocated and all pointers
> +     * correctly redirected to them, we can perform all the necessary
> +     * initializations.
> +     */
> +    SCHED_OP(new_ops, init_pdata, cpu);
> +    SCHED_OP(new_ops, tick_resume, cpu);
> +    SCHED_OP(new_ops, insert_vcpu, idle);
> +
>       return 0;
>   }
>
> diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h
> index dbe7cab..14392f3 100644
> --- a/xen/include/xen/sched-if.h
> +++ b/xen/include/xen/sched-if.h
> @@ -133,6 +133,7 @@ struct scheduler {
>                                       void *);
>       void         (*free_pdata)     (const struct scheduler *, void *, int);
>       void *       (*alloc_pdata)    (const struct scheduler *, int);
> +    void         (*init_pdata)     (const struct scheduler *, int);
>       void         (*free_domdata)   (const struct scheduler *, void *);
>       void *       (*alloc_domdata)  (const struct scheduler *, struct domain *);
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>