Re: [Xen-devel] [PATCH v2] xen/sched: let credit scheduler control its timer all alone

[Jürgen Groß <jgross@xxxxxxxx>]

On 07.10.19 11:05, Dario Faggioli wrote:
> On Mon, 2019-10-07 at 08:35 +0200, Juergen Gross wrote:
> > The credit scheduler is the only scheduler with tick_suspend and
> > tick_resume callbacks. Today those callbacks are invoked without
> > being
> > guarded by the scheduler lock which is critical when at the same the
> > cpu those callbacks are active is being moved to or from a cpupool.
> >
> > Crashes like the following are possible due to that race:
> >
> > (XEN) ----[ Xen-4.13.0-8.0.12-d  x86_64  debug=y   Not tainted ]----
> > (XEN) CPU:    79
> > (XEN) RIP:    e008:[<ffff82d0802467dc>] set_timer+0x39/0x1f7
> > (XEN) RFLAGS: 0000000000010002   CONTEXT: hypervisor
> > <snip>
> > (XEN) Xen call trace:
> > (XEN)    [<ffff82d0802467dc>] set_timer+0x39/0x1f7
> > (XEN)    [<ffff82d08022c1f4>]
> > sched_credit.c#csched_tick_resume+0x54/0x59
> > (XEN)    [<ffff82d080241dfe>] sched_tick_resume+0x67/0x86
> > (XEN)    [<ffff82d0802eda52>] mwait-idle.c#mwait_idle+0x32b/0x357
> > (XEN)    [<ffff82d08027939e>] domain.c#idle_loop+0xa6/0xc2
> > (XEN)
> > (XEN) Pagetable walk from 0000000000000048:
> > (XEN)  L4[0x000] = 00000082cfb9c063 ffffffffffffffff
> > (XEN)  L3[0x000] = 00000082cfb9b063 ffffffffffffffff
> > (XEN)  L2[0x000] = 00000082cfb9a063 ffffffffffffffff
> > (XEN)  L1[0x000] = 0000000000000000 ffffffffffffffff
> > (XEN)
> > (XEN) ****************************************
> > (XEN) Panic on CPU 79:
> > (XEN) FATAL PAGE FAULT
> > (XEN) [error_code=0000]
> > (XEN) Faulting linear address: 0000000000000048
> > (XEN) ****************************************
> >
> > The callbacks are used when the cpu is going to or coming from idle
> > in
> > order to allow higher C-states.
> >
> > The credit scheduler knows when it is going to schedule an idle
> > scheduling unit or another one after idle, so it can easily stop or
> > resume the timer itself removing the need to do so via the callback.
> > As this timer handling is done in the main scheduling function the
> > scheduler lock is still held, so the race with cpupool operations can
> > no longer occur. Note that calling the callbacks from
> > schedule_cpu_rm()
> > and schedule_cpu_add() is no longer needed, as the transitions to and
> > from idle in the cpupool with credit active will automatically occur
> > and do the right thing.
> >
> > With the last user of the callbacks gone those can be removed.
> Which is great! :-0
>
> > Suggested-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> Well, unless I'm missing something, I guess that, at this point:
>
> > --- a/xen/common/schedule.c
> > +++ b/xen/common/schedule.c
> > @@ -3082,32 +3078,14 @@ void schedule_dump(struct cpupool *c)
> >   
> >   void sched_tick_suspend(void)
> >   {
> > -    struct scheduler *sched;
> > -    unsigned int cpu = smp_processor_id();
> > -
> > -    rcu_read_lock(&sched_res_rculock);
> > -
> > -    sched = get_sched_res(cpu)->scheduler;
> > -    sched_do_tick_suspend(sched, cpu);
> > -    rcu_idle_enter(cpu);
> > +    rcu_idle_enter(smp_processor_id());
> >       rcu_idle_timer_start();
> > -
> > -    rcu_read_unlock(&sched_res_rculock);
> >   }
> sched_tick_suspend() could go away and rcu_idle_enter() be called
> directly (with rcu_idle_timer_start() becoming static, and called
> directly by rcu_idle_timer_enter() itself)
>
> And the same for sched_tick_resume(), rcu_idle_timer_stop() and
> rcu_idle_exit().
>
> I'll give my:
>
> Reviewed-by: Dario Faggioli <dfaggioli@xxxxxxxx>
>
> To this patch, though, as I appreciate we want it in to be able to
> continue testing core-scheduling during 4.13 rc-phase.
>
> It'd be cool if the adjustments described above (if agreed upon), could
> come as a follow-up.
Noted on my "scheduling cleanup" todo list.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel