Received-SPF: pass (google.com: domain of linux-kernel+bounces-188554-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: Thomas Gleixner <tglx@linutronix.de>
To: Oleg Nesterov <oleg@redhat.com>, 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>
Cc: 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
In-Reply-To: <20240523132358.GA1965@redhat.com>
References: <20240522151742.GA10400@redhat.com>
 <20240523132358.GA1965@redhat.com>
Date: Fri, 24 May 2024 11:31:12 +0200
Message-ID: <87h6eneeu7.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

Oleg!

On Thu, May 23 2024 at 15:23, Oleg Nesterov wrote:
> On 05/22, Oleg Nesterov wrote:
>>
>> After the recent comment 5097cbcb38e6 ("sched/isolation: Prevent boot crash
>> when the boot CPU is nohz_full") the kernel no longer crashes, but there is
>> another problem.
>>
>> In this case tick_setup_device() does tick_take_do_timer_from_boot() to
>> update tick_do_timer_cpu and this triggers WARN_ON_ONCE(irqs_disabled())
>> in smp_call_function_single().
>>
>> I don't understand this code even remotely, I failed to find the fix.
>>
>> Perhaps we can use smp_call_function_single_async() as a workaround ?
>>
>> 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;
        ...

So there is no race because the boot CPU _cannot_ reach
tick_nohz_stop_tick() as long as no secondary has taken over.

It's far from obvious. What a horrible maze...

> 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.

> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 71a792cd8936..3b1d011d45e1 100644
> --- 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?

>  		WRITE_ONCE(tick_do_timer_cpu, TICK_DO_TIMER_NONE);
>  		tick_sched_flag_set(ts, TS_FLAG_DO_TIMER_LAST);
>  	} else if (tick_cpu != TICK_DO_TIMER_NONE) {

Thanks,

        tglx