Received-SPF: pass (google.com: domain of linux-kernel+bounces-188774-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 24 May 2024 16:10:52 +0200
From: Oleg Nesterov <oleg@redhat.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Frederic Weisbecker <frederic@kernel.org>,
	Ingo Molnar <mingo@redhat.com>, Nicholas Piggin <npiggin@gmail.com>,
	Peter Zijlstra <peterz@infradead.org>, Phil Auld <pauld@redhat.com>,
	Chris von Recklinghausen <crecklin@redhat.com>,
	linux-kernel@vger.kernel.org
Subject: Re: sched/isolation: tick_take_do_timer_from_boot() calls
 smp_call_function_single() with irqs disabled
Message-ID: <20240524141018.GA14261@redhat.com>
References: <20240522151742.GA10400@redhat.com>
 <20240523132358.GA1965@redhat.com>
 <87h6eneeu7.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87h6eneeu7.ffs@tglx>
User-Agent: Mutt/1.5.24 (2015-08-30)

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.

But you know, I won't include this WARN_ON_ONCE(), I have added it for
(very basic) testing. We have another WARN_ON(tick_nohz_full_running)
in tick_sched_do_timer(), I guess it should be enough.

Thanks!

Oleg.