Re: sched/isolation: tick_take_do_timer_from_boot() calls smp_call_function_single() with irqs disabled

[Frederic Weisbecker]

Le Fri, May 24, 2024 at 04:10:52PM +0200, Oleg Nesterov a écrit :
> Hi Thomas,
> 
> On 05/24, Thomas Gleixner wrote:
> >
> > >> But I don't even understand why exactly we need smp_call_function()...
> >
> > It's not required at all.
> >
> > >> Race with tick_nohz_stop_tick() on boot CPU which can set
> > >> tick_do_timer_cpu = TICK_DO_TIMER_NONE? Is it really bad?
> >
> > This can't happen.
> >
> > > And is it supposed to happen if tick_nohz_full_running ?
> > >
> > > tick_sched_do_timer() and can_stop_idle_tick() claim that
> > > TICK_DO_TIMER_NONE is not possible in this case...
> >
> > What happens during boot is:
> >
> >   1) The boot CPU takes the do_timer duty when it installs its
> >      clockevent device
> >
> >   2) The boot CPU does not give up the duty because of this
> >      condition in can_stop_idle_tick():
> >
> >      if (tick_nohz_full_enabled()) {
> >      	if (tick_cpu == cpu)
> >            return false;
> 
> Yes, I have looked at this code too. But I failed to understand its
> callers, even tick_nohz_idle_stop_tick() which doesn't even call this
> function when ts->timer_expires != 0.
> 
> This code is too tricky for me, I still don't follow the logic.
> Damn, I can't even remember the names of all these functions ;)
> 
> > > So, once again, could you explain why the patch below is wrong?
> >
> > > -			tick_take_do_timer_from_boot();
> > >  			tick_do_timer_boot_cpu = -1;
> > > -			WARN_ON(READ_ONCE(tick_do_timer_cpu) != cpu);
> > > +			WRITE_ONCE(tick_do_timer_cpu, cpu);
> >
> > This part is perfectly fine.
> 
> Great, thanks! I'll write the changelog and send the patch tomorrow.
> 
> > > --- a/kernel/time/tick-sched.c
> > > +++ b/kernel/time/tick-sched.c
> > > @@ -1014,6 +1014,9 @@ static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu)
> > >  	 */
> > >  	tick_cpu = READ_ONCE(tick_do_timer_cpu);
> > >  	if (tick_cpu == cpu) {
> > > +#ifdef CONFIG_NO_HZ_FULL
> > > +		WARN_ON_ONCE(tick_nohz_full_running);
> > > +#endif
> >
> >                 WARN_ON_ONCE(tick_nohz_full_enabled());
> >
> > which spares the ugly #ifdef?
> 
> Yes but tick_nohz_full_enabled() depends on context_tracking_key, and
> context_tracking_enabled() is false without CONFIG_CONTEXT_TRACKING_USER.
> I didn't even try to check if it is selected by NO_HZ_FULL and how do
> they play together.

Yes it is! You can rely on it.

Thanks.