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

In reply to Frederic Weisbecker

Quoted 17 lines · Thomas Gleixner

Oleg!
Quoted 14 lines · Oleg Nesterov
Quoted 13 lines · Oleg Nesterov
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 ?

Quoted 8 lines

Quoted 1 line
Quoted 1 line
But I don't even understand why exactly we need smp_call_function()...
It's not required at all.
Quoted 2 lines
Quoted 2 lines
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.

Actually... The boot CPU is nohz_full and nothing prevents it from stopping its tick once IRQs are enabled and before calling tick_nohz_idle_enter(). When that happens, tick_nohz_full_update_tick() doesn't go through can_stop_idle_tick() and therefore doesn't check if it is the timekeeper. And then it goes through tick_nohz_stop_tick() which can set tick_do_timer_cpu = TICK_DO_TIMER_NONE.

Quoted 19 lines

Quoted 5 lines
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;
        ...

Quoted 4 lines

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

I know, I wish I had the time to Nack that nohz_full boot CPU patch back then. But now we have to maintain it, even though it's broken and uglifies the situation.

Anyway, we probably need to prevent from stopping the tick as long as a CPU is the timekeeper and some CPU (could be the same) is nohz_full somewhere.

That needs to be a seperate change (I'll try to fix that after the week-end with a new brain) and then Oleg's patch can go on top of it.

Thanks.

Quoted 33 lines

kernel/time/tick-common.c static void tick_setup_device(struct tick_device *td, · line 215
Quoted 7 lines
So, once again, could you explain why the patch below is wrong?
kernel/time/tick-common.c +1 -2
-			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.
kernel/time/tick-sched.c static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu) · line 1019
Quoted 11 lines
kernel/time/tick-sched.c +3 -0
index 71a792cd8936..3b1d011d45e1 100644
@@ -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?
kernel/time/tick-sched.c static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu) · line 1022
Quoted 3 lines
kernel/time/tick-sched.c +0 -0
 		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

Thomas Gleixner

2024-05-24 17:16 UTC #6 lore raw

Quoted 4 lines · Frederic Weisbecker

Quoted 3 lines · Thomas Gleixner
Quoted 2 lines
Quoted 2 lines
Race with tick_nohz_stop_tick() on boot CPU which can set tick_do_timer_cpu = TICK_DO_TIMER_NONE? Is it really bad?

Quoted 8 lines

Quoted 1 line
This can't happen.
Actually... The boot CPU is nohz_full and nothing prevents it from stopping its tick once IRQs are enabled and before calling tick_nohz_idle_enter(). When that happens, tick_nohz_full_update_tick() doesn't go through can_stop_idle_tick() and therefore doesn't check if it is the timekeeper. And then it goes through tick_nohz_stop_tick() which can set tick_do_timer_cpu = TICK_DO_TIMER_NONE.

OMG...

Oleg Nesterov

author

2024-05-24 18:38 UTC #7 lore raw

Frederic. Thanks for looking at this!

I've already had a few beers today, I know I'll regret about this email tomorrow, but I can't resist ;)

Quoted 7 lines · Frederic Weisbecker

Actually... The boot CPU is nohz_full and nothing prevents it from stopping its tick once IRQs are enabled and before calling tick_nohz_idle_enter(). When that happens, tick_nohz_full_update_tick() doesn't go through can_stop_idle_tick() and therefore doesn't check if it is the timekeeper. And then it goes through tick_nohz_stop_tick() which can set tick_do_timer_cpu = TICK_DO_TIMER_NONE.

Heh. May be you mean, say, tick_nohz_full_update_tick() or tick_nohz_idle_update_tick() which check tick_nohz_full_cpu(cpu). Yes this doesn't help during the boot, and this was another source for confusion to me.

But again, again. tick_sched_do_timer() says

* If nohz_full is enabled, this should not happen because the
* 'tick_do_timer_cpu' CPU never relinquishes.

so I guess it is not supposed to happen?

And. My main question was: how can smp_call_function_single() help??? Why do we actually need it?

Thanks ;)

Oleg.

Thomas Gleixner

2024-05-24 22:06 UTC #8 lore raw

Quoted 2 lines · Oleg Nesterov

I've already had a few beers today, I know I'll regret about this email tomorrow, but I can't resist ;)

You won't regret it. :)

Quoted 7 lines · Frederic Weisbecker

But again, again. tick_sched_do_timer() says
* If nohz_full is enabled, this should not happen because the
* 'tick_do_timer_cpu' CPU never relinquishes.
so I guess it is not supposed to happen?

Right. It does not happen because the kernel starts with jiffies as clocksource except on S390. The jiffies clocksource is not qualified to switch over to NOHZ mode for obvious reasons. But even on S390 which has a truly usable and useful clocksource the tick stays periodic to begin with. Why?

The NOHZ ready notification happens late in the boot process via:
fs_initcall(clocksource_done_booting)

So by the time that happens, the secondary CPUs are up and have taken over the do timer duty.

[    0.600381] smp: Bringing up secondary CPUs ...

....

[    1.917842] clocksource: Switched to clocksource kvm-clock
[    1.918548] clocksource_done_booting: Switched to NOHZ // debug printk

This is the point where tick_nohz_activate() is called first time and that does:

  tick_sched_flag_set(ts, TS_FLAG_NOHZ);

So up to this point the tick is never stopped neither on housekeeping nor on NOHZ FULL CPUs:

tick_nohz_full_update_tick()
if (!tick_sched_flag_test(ts, TS_FLAG_NOHZ))

    return;

Quoted 1 line

And. My main question was: how can smp_call_function_single() help???

It's useless.

Quoted 1 line

Why do we actually need it?

We do not.

As explained above there is also nothing extra to fix contrary to Frederics fears.

Even in the case that a command line limitation restricts the number of CPUs such that there is no housekeeping CPU onlined during smp_init(). That is checked in the isolation init code which clears nohz_full_running in that case. Nothing to see there either.

So all this needs is the simple:

kernel/time/tick-common.c +2 -4

index d88b13076b79..dab17d756fd8 100644
@@ -229,11 +209,9 @@ static void tick_setup_device(struct tick_device *td,
 			if (tick_nohz_full_cpu(cpu))
 				tick_do_timer_boot_cpu = cpu;
 
-		} else if (tick_do_timer_boot_cpu != -1 &&
-						!tick_nohz_full_cpu(cpu)) {
-			tick_take_do_timer_from_boot();
+		} else if (tick_do_timer_boot_cpu != -1 && !tick_nohz_full_cpu(cpu)) {
+			WRITE_ONCE(tick_do_timer_cpu, cpu);
 			tick_do_timer_boot_cpu = -1;
-			WARN_ON(READ_ONCE(tick_do_timer_cpu) != cpu);
 #endif
 		}

along with the removal of the SMP function call voodoo programming gunk, a lengthy changelog and a bunch of useful comments.

Changing the horribly lazy and incomprehensible '-1' to an actual meaningful define, e.g. TICK_DO_TIMER_NONE, would definitely help along with renaming the variable to tick_do_timer_nohz_full_boot_cpu.

There is no race other than the boot CPU reading tick_do_timer_cpu concurrently to the update, but that's completely harmless whatever it sees there. If it's the boot CPU, i.e. 0, or the secondary does not matter. The secondary immediately schedules the tick unconditionally so timekeeping and jiffies will just work.

If the secondary CPU fails to come up after it installed the clock event device then the missing tick is the least of the problems.

That has absolutely nothing to do with the issue at hand. If the CPU which owns tick_do_timer_cpu dies or gets stuck then all bets are off independent of NOHZ FULL. See the changes which went in during the merge window to handle the case where the hypervisor fails to inject the timer interrupts or keeps the time keeper duty CPU scheduled out for a long period of time....

Thanks,

tglx

Oleg Nesterov

author

2024-05-25 13:52 UTC #9 lore raw

Thomas, thanks a lot!

Let me grep a bit more to better understand your explanations.

Just one note for now.

Quoted 17 lines · Thomas Gleixner

kernel/time/tick-common.c +2 -4

@@ -229,11 +209,9 @@ static void tick_setup_device(struct tick_device *td,
 			if (tick_nohz_full_cpu(cpu))
 				tick_do_timer_boot_cpu = cpu;
 
-		} else if (tick_do_timer_boot_cpu != -1 &&
-						!tick_nohz_full_cpu(cpu)) {
-			tick_take_do_timer_from_boot();
+		} else if (tick_do_timer_boot_cpu != -1 && !tick_nohz_full_cpu(cpu)) {
+			WRITE_ONCE(tick_do_timer_cpu, cpu);
 			tick_do_timer_boot_cpu = -1;
-			WARN_ON(READ_ONCE(tick_do_timer_cpu) != cpu);
 #endif
 		}

Quoted 1 line

along with the removal of the SMP function call voodoo programming gunk,

Yes,

Quoted 3 lines

Changing the horribly lazy and incomprehensible '-1' to an actual meaningful define, e.g. TICK_DO_TIMER_NONE, would definitely help along with renaming the variable to tick_do_timer_nohz_full_boot_cpu.

Better yet, we can make it a boolean, we do not need cpu number. And perhaps we can simply kill it along with #ifdef CONFIG_NO_HZ_FULL ?

	if (!td->evtdev) {
		tick_cpu = READ_ONCE(tick_do_timer_cpu);
		/*
		 * If no cpu took the do_timer update, assign it to
		 * this cpu:
		 */
		if (tick_cpu == TICK_DO_TIMER_BOOT) {
			WRITE_ONCE(tick_do_timer_cpu, cpu);
			tick_next_period = ktime_get();
			/*
			 * The boot CPU may be nohz_full, in which case the
			 * first housekeeping secondary will take do_timer
			 * from us.
			 */
		} else if (tick_nohz_full_cpu(tick_cpu) &&
			  !tick_nohz_full_cpu(cpu)) {
			WRITE_ONCE(tick_do_timer_cpu, cpu);
		}

Oleg.

Oleg Nesterov

author

2024-05-25 14:14 UTC #10 lore raw

Quoted 13 lines · Oleg Nesterov

perhaps we can simply kill it along with #ifdef CONFIG_NO_HZ_FULL ?

	if (!td->evtdev) {
		tick_cpu = READ_ONCE(tick_do_timer_cpu);
		/*
		 * If no cpu took the do_timer update, assign it to
		 * this cpu:
		 */
		if (tick_cpu == TICK_DO_TIMER_BOOT) {
			WRITE_ONCE(tick_do_timer_cpu, cpu);
			tick_next_period = ktime_get();
			/*

Quoted 8 lines

			 * The boot CPU may be nohz_full, in which case the
			 * first housekeeping secondary will take do_timer
			 * from us.
			 */
		} else if (tick_nohz_full_cpu(tick_cpu) &&
			  !tick_nohz_full_cpu(cpu)) {
			WRITE_ONCE(tick_do_timer_cpu, cpu);
		}

although tick_nohz_full_cpu(tick_cpu) above depends on the fact that tick_cpu = TICK_DO_TIMER_NONE should not be possible if tick_nohz_full_enabled(), not good.

Oleg.

Oleg Nesterov

author

2024-05-26 19:29 UTC #11 lore raw

The more I grep the more I confused.

Quoted 4 lines · Thomas Gleixner

Right. It does not happen because the kernel starts with jiffies as clocksource except on S390. The jiffies clocksource is not qualified to switch over to NOHZ mode for obvious reasons.

Not obvious for those who never looked at this code ;)

OK, clocksource_jiffies doesn't have CLOCK_SOURCE_VALID_FOR_HRES,

Quoted 1 line

fs_initcall(clocksource_done_booting)

So clocksource_done_booting() -> clocksource_select() should find another CLOCK_SOURCE_VALID_FOR_HRES cs, then timekeeping_notify(best)

- sets tk_core.timekeeper.tkr_mono.clock = best

- calls tick_clock_notify() which sets .check_clocks on every cpu.

This makes tick_check_oneshot_change(false) return true. (I am ignoring the highres=n case, tick_nohz_switch_to_nohz() doesn't really differ).

Now, on every CPU the next ->event_handler == tick_handle_periodic path call hrtimer_switch_to_hres() which

- sets ->event_handler == hrtimer_interrupt (tick_init_highres) so tick_periodic/do_timer will be never called again

- calls tick_setup_sched_timer() -> tick_nohz_activate() which sets TS_FLAG_NOHZ

Quoted 6 lines

[    1.918548] clocksource_done_booting: Switched to NOHZ // debug printk
This is the point where tick_nohz_activate() is called first time and that does:
  tick_sched_flag_set(ts, TS_FLAG_NOHZ);

See above, but I got lost, most probably I misunderstood these nontrivial code paths.

Quoted 6 lines

So up to this point the tick is never stopped neither on housekeeping nor on NOHZ FULL CPUs:
tick_nohz_full_update_tick()
if (!tick_sched_flag_test(ts, TS_FLAG_NOHZ))
    return;

OK... But tick_nohz_idle_update_tick() doesn't check TS_FLAG_NOHZ and the tick_nohz_full_cpu() check can't help at boot time.

And I still don't understand why we can rely on can_stop_idle_tick() even in tick_nohz_idle_stop_tick().

I'll try to read this code again tomorrow, but it will never fit my poor little brain ;)

Oleg.

Frederic Weisbecker

maintainer

2024-05-26 20:52 UTC #12 lore raw

Quoted 9 lines · Oleg Nesterov

Quoted 6 lines
So up to this point the tick is never stopped neither on housekeeping nor on NOHZ FULL CPUs:
tick_nohz_full_update_tick()
if (!tick_sched_flag_test(ts, TS_FLAG_NOHZ))
    return;
OK... But tick_nohz_idle_update_tick() doesn't check TS_FLAG_NOHZ and the tick_nohz_full_cpu() check can't help at boot time.

Yes but tick_nohz_idle_update_tick() is only called when the tick is already stopped. And for the tick to be already stopped, TS_FLAG_NOHZ must have been set.

Quoted 2 lines

And I still don't understand why we can rely on can_stop_idle_tick() even in tick_nohz_idle_stop_tick().

Not sure I follow you on this one...

Quoted 3 lines

I'll try to read this code again tomorrow, but it will never fit my poor little brain ;)

You understood more than I ever did in just a few hours :-)

But yes the tick code was much more simple before I put my own hands inside. nohz_full and cpuidle did not arrange it.

Thanks.

Oleg Nesterov

author

2024-05-27 15:58 UTC #13 lore raw

Quoted 9 lines · Frederic Weisbecker

Quoted 7 lines · Oleg Nesterov
Quoted 6 lines
So up to this point the tick is never stopped neither on housekeeping nor on NOHZ FULL CPUs:
tick_nohz_full_update_tick()
if (!tick_sched_flag_test(ts, TS_FLAG_NOHZ))
    return;

Quoted 6 lines

Quoted 2 lines
OK... But tick_nohz_idle_update_tick() doesn't check TS_FLAG_NOHZ and the tick_nohz_full_cpu() check can't help at boot time.
Yes but tick_nohz_idle_update_tick() is only called when the tick is already stopped. And for the tick to be already stopped, TS_FLAG_NOHZ must have been set.

Ah, TS_FLAG_NOHZ! Indeed, thanks.

Quoted 4 lines

Quoted 2 lines
And I still don't understand why we can rely on can_stop_idle_tick() even in tick_nohz_idle_stop_tick().
Not sure I follow you on this one...

can_stop_idle_tick() has no effect if ->timer_expires_base || tick_nohz_next_event() but I see another email from Nicholas.

Thanks!

Oleg.

Nicholas Piggin

2024-05-27 11:01 UTC #14 lore raw

In reply to Nicholas Piggin

Quoted 11 lines · Oleg Nesterov

The more I grep the more I confused.
Quoted 4 lines · Thomas Gleixner
Right. It does not happen because the kernel starts with jiffies as clocksource except on S390. The jiffies clocksource is not qualified to switch over to NOHZ mode for obvious reasons.
Not obvious for those who never looked at this code ;)
OK, clocksource_jiffies doesn't have CLOCK_SOURCE_VALID_FOR_HRES,

jiffies clocksource requires a timer to run it I suppose.

[snip]

Quoted 7 lines

Quoted 6 lines
So up to this point the tick is never stopped neither on housekeeping nor on NOHZ FULL CPUs:
tick_nohz_full_update_tick()
if (!tick_sched_flag_test(ts, TS_FLAG_NOHZ))
    return;

Quoted 5 lines

OK... But tick_nohz_idle_update_tick() doesn't check TS_FLAG_NOHZ and the tick_nohz_full_cpu() check can't help at boot time.
And I still don't understand why we can rely on can_stop_idle_tick() even in tick_nohz_idle_stop_tick().

AFAIKS timer_expires_base would be 0 unless tick_nohz_next_event() were called, but that is only called from places that checked can_stop_idle_tick() or is already a tick_nohz_full() CPU.

And timer tick CPU can't be unplugged with nohz full.

Thanks,
Nick

Oleg Nesterov

author

2024-05-27 15:59 UTC #15 lore raw

Quoted 10 lines · Nicholas Piggin

Quoted 8 lines · Oleg Nesterov
The more I grep the more I confused.
Quoted 4 lines · Thomas Gleixner
Right. It does not happen because the kernel starts with jiffies as clocksource except on S390. The jiffies clocksource is not qualified to switch over to NOHZ mode for obvious reasons.

Quoted 5 lines

Quoted 3 lines
Not obvious for those who never looked at this code ;)
OK, clocksource_jiffies doesn't have CLOCK_SOURCE_VALID_FOR_HRES,
jiffies clocksource requires a timer to run it I suppose.

I meant, this is enough to ensure that clocksource_done_booting() paths should find another cs, best != curr_clocksource, and call timekeeping_notify().

Nevermind, quite possibly I misread this code.

Quoted 6 lines

Quoted 2 lines
And I still don't understand why we can rely on can_stop_idle_tick() even in tick_nohz_idle_stop_tick().
AFAIKS timer_expires_base would be 0 unless tick_nohz_next_event() were called, but that is only called from places that checked can_stop_idle_tick() or is already a tick_nohz_full() CPU.

OK, thanks.

So, Frederic, Nicholas, any objections to the trivial change below?

We can cleanup the tick_do_timer_boot_cpu logic on top of it.

Oleg.

kernel/time/tick-common.c +1 ~~-22~~

@@ -178,26 +178,6 @@ void tick_setup_periodic(struct clock_event_device *dev, int broadcast)
 	}
 }
 
-#ifdef CONFIG_NO_HZ_FULL
-static void giveup_do_timer(void *info)
-{
-	int cpu = *(unsigned int *)info;
-
-	WARN_ON(tick_do_timer_cpu != smp_processor_id());
-
-	tick_do_timer_cpu = cpu;
-}
-
-static void tick_take_do_timer_from_boot(void)
-{
-	int cpu = smp_processor_id();
-	int from = tick_do_timer_boot_cpu;
-
-	if (from >= 0 && from != cpu)
-		smp_call_function_single(from, giveup_do_timer, &cpu, 1);
-}
-#endif
-
 /*
  * Setup the tick device
  */

@@ -231,9 +211,8 @@ static void tick_setup_device(struct tick_device *td,
 
 		} else if (tick_do_timer_boot_cpu != -1 &&
 						!tick_nohz_full_cpu(cpu)) {
-			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);
 #endif
 		}

Nicholas Piggin

2024-05-28 01:02 UTC #16 lore raw

Quoted 25 lines · Oleg Nesterov

Quoted 10 lines · Nicholas Piggin
Quoted 8 lines · Oleg Nesterov
The more I grep the more I confused.
Quoted 4 lines · Thomas Gleixner
Right. It does not happen because the kernel starts with jiffies as clocksource except on S390. The jiffies clocksource is not qualified to switch over to NOHZ mode for obvious reasons.
Quoted 5 lines
Quoted 3 lines
Not obvious for those who never looked at this code ;)
OK, clocksource_jiffies doesn't have CLOCK_SOURCE_VALID_FOR_HRES,
jiffies clocksource requires a timer to run it I suppose.
I meant, this is enough to ensure that clocksource_done_booting() paths should find another cs, best != curr_clocksource, and call timekeeping_notify().
Nevermind, quite possibly I misread this code.
Quoted 3 lines
Quoted 2 lines
And I still don't understand why we can rely on can_stop_idle_tick() even in tick_nohz_idle_stop_tick().

Quoted 7 lines

Quoted 3 lines
AFAIKS timer_expires_base would be 0 unless tick_nohz_next_event() were called, but that is only called from places that checked can_stop_idle_tick() or is already a tick_nohz_full() CPU.
OK, thanks.
So, Frederic, Nicholas, any objections to the trivial change below?

Since Thomas says it's alright, then no. I guess I added it because I was not certain about taking the tick_do_timer_cpu while the boot CPU could be running a timer interrupt.

I would take some of his comment to explain the race is harmless and put it in that if block.

Out of curiosity, you are getting this going on x86? Or another arch? Any particular use-case?

Thanks,
Nick

Quoted 45 lines

We can cleanup the tick_do_timer_boot_cpu logic on top of it.

Oleg.

kernel/time/tick-common.c +1 ~~-22~~

@@ -178,26 +178,6 @@ void tick_setup_periodic(struct clock_event_device *dev, int broadcast)
 	}
 }
 
-#ifdef CONFIG_NO_HZ_FULL
-static void giveup_do_timer(void *info)
-{
-	int cpu = *(unsigned int *)info;
-
-	WARN_ON(tick_do_timer_cpu != smp_processor_id());
-
-	tick_do_timer_cpu = cpu;
-}
-
-static void tick_take_do_timer_from_boot(void)
-{
-	int cpu = smp_processor_id();
-	int from = tick_do_timer_boot_cpu;
-
-	if (from >= 0 && from != cpu)
-		smp_call_function_single(from, giveup_do_timer, &cpu, 1);
-}
-#endif
-
 /*
  * Setup the tick device
  */

@@ -231,9 +211,8 @@ static void tick_setup_device(struct tick_device *td,
 
 		} else if (tick_do_timer_boot_cpu != -1 &&
 						!tick_nohz_full_cpu(cpu)) {
-			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);
 #endif
 		}

Oleg Nesterov

author

2024-05-28 12:20 UTC #17 lore raw

Quoted 7 lines · Nicholas Piggin

Quoted 3 lines
So, Frederic, Nicholas, any objections to the trivial change below?
Since Thomas says it's alright, then no. I guess I added it because I was not certain about taking the tick_do_timer_cpu while the boot CPU could be running a timer interrupt.

I thought about it too, but didn't see anything wrong...

Suppose that WRITE_ONCE(tick_do_timer_cpu, cpu) happens right after tick_periodic() on the boot CPU sees READ_ONCE(tick_do_timer_cpu) == cpu. Does this really differ from the case when tick_take_do_timer_from_boot() waits for the boot CPU to return from timer_interrupt() ?

Quoted 2 lines

I would take some of his comment to explain the race is harmless and put it in that if block.

Yes, yes, sure. See the patch I'll send in a minute.

Quoted 1 line

Out of curiosity, you are getting this going on x86?

Yes, and I didn't check other arch'es.

Quoted 1 line

Any particular use-case?

I have no idea. I noticed this problem when I was working on 5097cbcb38e6 ("sched/isolation: Prevent boot crash when the boot CPU is nohz_full"), see
https://lore.kernel.org/all/[email protected]/

Perhaps Chris who reported that problem can add more details.

Oleg.

Thomas Gleixner

2024-05-27 16:13 UTC #18 lore raw

Quoted 9 lines · Nicholas Piggin

Quoted 8 lines · Oleg Nesterov
The more I grep the more I confused.
Quoted 4 lines · Thomas Gleixner
Right. It does not happen because the kernel starts with jiffies as clocksource except on S390. The jiffies clocksource is not qualified to switch over to NOHZ mode for obvious reasons.

Quoted 5 lines

Quoted 3 lines
Not obvious for those who never looked at this code ;)
OK, clocksource_jiffies doesn't have CLOCK_SOURCE_VALID_FOR_HRES,
jiffies clocksource requires a timer to run it I suppose.

Yes, but it's all periodic (nohz disabled) mode until clocksource_done_booting() selects a high resolution clocksource and kicks the NOHZ stuff alive. That's a fs_initcall() and happens late in the boot process.

Thanks,

tglx

Frederic Weisbecker

maintainer

2024-05-26 20:57 UTC #19 lore raw

Quoted 37 lines · Thomas Gleixner

Quoted 2 lines · Oleg Nesterov
I've already had a few beers today, I know I'll regret about this email tomorrow, but I can't resist ;)
You won't regret it. :)
Quoted 7 lines · Frederic Weisbecker
But again, again. tick_sched_do_timer() says
* If nohz_full is enabled, this should not happen because the
* 'tick_do_timer_cpu' CPU never relinquishes.
so I guess it is not supposed to happen?
Right. It does not happen because the kernel starts with jiffies as clocksource except on S390. The jiffies clocksource is not qualified to switch over to NOHZ mode for obvious reasons. But even on S390 which has a truly usable and useful clocksource the tick stays periodic to begin with. Why?
The NOHZ ready notification happens late in the boot process via:
fs_initcall(clocksource_done_booting)
So by the time that happens, the secondary CPUs are up and have taken over the do timer duty.
[    0.600381] smp: Bringing up secondary CPUs ...
....
[    1.917842] clocksource: Switched to clocksource kvm-clock
[    1.918548] clocksource_done_booting: Switched to NOHZ // debug printk
This is the point where tick_nohz_activate() is called first time and that does:

Quoted 8 lines

  tick_sched_flag_set(ts, TS_FLAG_NOHZ);
So up to this point the tick is never stopped neither on housekeeping nor on NOHZ FULL CPUs:
tick_nohz_full_update_tick()
if (!tick_sched_flag_test(ts, TS_FLAG_NOHZ))
    return;

Oh nice! I indeed completely skipped the clocksource part of the picture!

Thanks.

Nicholas Piggin

2024-05-27 09:10 UTC #20 lore raw

Quoted 82 lines · Thomas Gleixner

Quoted 2 lines · Oleg Nesterov
I've already had a few beers today, I know I'll regret about this email tomorrow, but I can't resist ;)
You won't regret it. :)
Quoted 7 lines · Frederic Weisbecker
But again, again. tick_sched_do_timer() says
* If nohz_full is enabled, this should not happen because the
* 'tick_do_timer_cpu' CPU never relinquishes.
so I guess it is not supposed to happen?
Right. It does not happen because the kernel starts with jiffies as clocksource except on S390. The jiffies clocksource is not qualified to switch over to NOHZ mode for obvious reasons. But even on S390 which has a truly usable and useful clocksource the tick stays periodic to begin with. Why?
The NOHZ ready notification happens late in the boot process via:
fs_initcall(clocksource_done_booting)
So by the time that happens, the secondary CPUs are up and have taken over the do timer duty.
[    0.600381] smp: Bringing up secondary CPUs ...
....
[    1.917842] clocksource: Switched to clocksource kvm-clock
[    1.918548] clocksource_done_booting: Switched to NOHZ // debug printk
This is the point where tick_nohz_activate() is called first time and that does:
  tick_sched_flag_set(ts, TS_FLAG_NOHZ);
So up to this point the tick is never stopped neither on housekeeping nor on NOHZ FULL CPUs:
tick_nohz_full_update_tick()
if (!tick_sched_flag_test(ts, TS_FLAG_NOHZ))
    return;
Quoted 1 line
And. My main question was: how can smp_call_function_single() help???
It's useless.
Quoted 1 line
Why do we actually need it?
We do not.
As explained above there is also nothing extra to fix contrary to Frederics fears.
Even in the case that a command line limitation restricts the number of CPUs such that there is no housekeeping CPU onlined during smp_init(). That is checked in the isolation init code which clears nohz_full_running in that case. Nothing to see there either.
So all this needs is the simple:
kernel/time/tick-common.c +2 -4
index d88b13076b79..dab17d756fd8 100644
@@ -229,11 +209,9 @@ static void tick_setup_device(struct tick_device *td,
 			if (tick_nohz_full_cpu(cpu))
 				tick_do_timer_boot_cpu = cpu;
 
-		} else if (tick_do_timer_boot_cpu != -1 &&
-						!tick_nohz_full_cpu(cpu)) {
-			tick_take_do_timer_from_boot();
+		} else if (tick_do_timer_boot_cpu != -1 && !tick_nohz_full_cpu(cpu)) {
+			WRITE_ONCE(tick_do_timer_cpu, cpu);
 			tick_do_timer_boot_cpu = -1;
-			WARN_ON(READ_ONCE(tick_do_timer_cpu) != cpu);
 #endif
 		}
 

Quoted 2 lines

along with the removal of the SMP function call voodoo programming gunk, a lengthy changelog and a bunch of useful comments.

I might not have tested that path on powerpc since it should not switch clockevent driver (or clocksource either I think) at least on 64-bit. Explains the smp_call_function warning if you are testing on x86 :/

Thanks,
Nick

Thomas Gleixner

2024-05-27 10:23 UTC #21 lore raw

Quoted 8 lines · Nicholas Piggin

Quoted 2 lines · Thomas Gleixner
along with the removal of the SMP function call voodoo programming gunk, a lengthy changelog and a bunch of useful comments.
I might not have tested that path on powerpc since it should not switch clockevent driver (or clocksource either I think) at least on 64-bit. Explains the smp_call_function warning if you are testing on x86 :/

Even on PowerPC the per CPU clockevent driver is registered during early boot for the boot CPU and for the APs when they are onlined. Before your change the boot CPU was unconditionally taking over the do_timer duty and never gave up on it in the NOHZ full case.

The logic you added allows that the duty is taken by the first housekeeping CPU in the case that the boot CPU is marked NOHZ full.

So yes, that function call _is_ invoked on PowerPC too if the boot CPU is NOHZ full. There is absolutely nothing x86 specific.

The difference is that PowerPC registers the per CPU clockevent _before_ setting the CPU online and x86 does it afterwards.

So the warning does not trigger on PowerPC because:

   WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled() && ...);

Thanks,

tglx

Nicholas Piggin

2024-05-27 11:17 UTC #22 lore raw

Quoted 15 lines · Thomas Gleixner

Quoted 8 lines · Nicholas Piggin
Quoted 2 lines · Thomas Gleixner
along with the removal of the SMP function call voodoo programming gunk, a lengthy changelog and a bunch of useful comments.
I might not have tested that path on powerpc since it should not switch clockevent driver (or clocksource either I think) at least on 64-bit. Explains the smp_call_function warning if you are testing on x86 :/
Even on PowerPC the per CPU clockevent driver is registered during early boot for the boot CPU and for the APs when they are onlined. Before your change the boot CPU was unconditionally taking over the do_timer duty and never gave up on it in the NOHZ full case.

Quoted 5 lines

The logic you added allows that the duty is taken by the first housekeeping CPU in the case that the boot CPU is marked NOHZ full.
So yes, that function call _is_ invoked on PowerPC too if the boot CPU is NOHZ full. There is absolutely nothing x86 specific.

Okay, right you are. Indeed it does trigger.

Quoted 6 lines

The difference is that PowerPC registers the per CPU clockevent _before_ setting the CPU online and x86 does it afterwards.
So the warning does not trigger on PowerPC because:
   WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled() && ...);

That explains it.

Thanks,
Nick

Oleg Nesterov

author

2024-05-28 12:21 UTC #23 lore raw

PATCH 1/1 tick/nohz_full: don't abuse smp_call_function_single() in tick_setup_device()

Link https://lore.kernel.org/all/[email protected] open here

After the recent commit 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() calls tick_take_do_timer_from_boot() to update tick_do_timer_cpu and this triggers the WARN_ON_ONCE(irqs_disabled) in smp_call_function_single().

Kill tick_take_do_timer_from_boot() and just use WRITE_ONCE(), the new comment tries to explain why this is safe (thanks Thomas!).

Fixes 08ae95f4fd3b ("nohz_full: Allow the boot CPU to be nohz_full")

Link https://lore.kernel.org/all/[email protected] open here

---
 kernel/time/tick-common.c | 39 +++++++++++++--------------------------
 1 file changed, 13 insertions(+), 26 deletions(-)

kernel/time/tick-common.c +13 ~~-26~~

index d88b13076b79..27d0018c8b05 100644
@@ -178,26 +178,6 @@ void tick_setup_periodic(struct clock_event_device *dev, int broadcast)
 	}
 }
 
-#ifdef CONFIG_NO_HZ_FULL
-static void giveup_do_timer(void *info)
-{
-	int cpu = *(unsigned int *)info;
-
-	WARN_ON(tick_do_timer_cpu != smp_processor_id());
-
-	tick_do_timer_cpu = cpu;
-}
-
-static void tick_take_do_timer_from_boot(void)
-{
-	int cpu = smp_processor_id();
-	int from = tick_do_timer_boot_cpu;
-
-	if (from >= 0 && from != cpu)
-		smp_call_function_single(from, giveup_do_timer, &cpu, 1);
-}
-#endif
-
 /*
  * Setup the tick device
  */

@@ -221,19 +201,26 @@ static void tick_setup_device(struct tick_device *td,
 			tick_next_period = ktime_get();
 #ifdef CONFIG_NO_HZ_FULL
 			/*
-			 * The boot CPU may be nohz_full, in which case set
-			 * tick_do_timer_boot_cpu so the first housekeeping
-			 * secondary that comes up will take do_timer from
-			 * us.
+			 * The boot CPU may be nohz_full, in which case the
+			 * first housekeeping secondary will take do_timer()
+			 * from us.
 			 */
 			if (tick_nohz_full_cpu(cpu))
 				tick_do_timer_boot_cpu = cpu;
 
 		} else if (tick_do_timer_boot_cpu != -1 &&
 						!tick_nohz_full_cpu(cpu)) {
-			tick_take_do_timer_from_boot();
 			tick_do_timer_boot_cpu = -1;
-			WARN_ON(READ_ONCE(tick_do_timer_cpu) != cpu);
+			/*
+			 * The boot CPU will stay in periodic (NOHZ disabled)
+			 * mode until clocksource_done_booting() called after
+			 * smp_init() selects a high resolution clocksource and
+			 * timekeeping_notify() kicks the NOHZ stuff alive.
+			 *
+			 * So this WRITE_ONCE can only race with the READ_ONCE
+			 * check in tick_periodic() but this race is harmless.
+			 */
+			WRITE_ONCE(tick_do_timer_cpu, cpu);
 #endif
 		}

Signature · 1 line

2.25.1.362.g51ebf55

Oleg Nesterov

author

2024-05-28 12:24 UTC #24 lore raw

Re: [PATCH] tick/nohz_full: don't abuse smp_call_function_single() in tick_setup_device()

If this patch/changelog/comment is fine, I'll send another trivial one which turns tick_do_timer_boot_cpu into "bool boot_cpu_is_nohz_full".

Quoted 86 lines · Oleg Nesterov

After the recent commit 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() calls tick_take_do_timer_from_boot() to update tick_do_timer_cpu and this triggers the WARN_ON_ONCE(irqs_disabled) in smp_call_function_single().

Kill tick_take_do_timer_from_boot() and just use WRITE_ONCE(), the new comment tries to explain why this is safe (thanks Thomas!).

Fixes 08ae95f4fd3b ("nohz_full: Allow the boot CPU to be nohz_full")

---
 kernel/time/tick-common.c | 39 +++++++++++++--------------------------
 1 file changed, 13 insertions(+), 26 deletions(-)

kernel/time/tick-common.c +13 ~~-26~~

index d88b13076b79..27d0018c8b05 100644
@@ -178,26 +178,6 @@ void tick_setup_periodic(struct clock_event_device *dev, int broadcast)
 	}
 }
 
-#ifdef CONFIG_NO_HZ_FULL
-static void giveup_do_timer(void *info)
-{
-	int cpu = *(unsigned int *)info;
-
-	WARN_ON(tick_do_timer_cpu != smp_processor_id());
-
-	tick_do_timer_cpu = cpu;
-}
-
-static void tick_take_do_timer_from_boot(void)
-{
-	int cpu = smp_processor_id();
-	int from = tick_do_timer_boot_cpu;
-
-	if (from >= 0 && from != cpu)
-		smp_call_function_single(from, giveup_do_timer, &cpu, 1);
-}
-#endif
-
 /*
  * Setup the tick device
  */

@@ -221,19 +201,26 @@ static void tick_setup_device(struct tick_device *td,
 			tick_next_period = ktime_get();
 #ifdef CONFIG_NO_HZ_FULL
 			/*
-			 * The boot CPU may be nohz_full, in which case set
-			 * tick_do_timer_boot_cpu so the first housekeeping
-			 * secondary that comes up will take do_timer from
-			 * us.
+			 * The boot CPU may be nohz_full, in which case the
+			 * first housekeeping secondary will take do_timer()
+			 * from us.
 			 */
 			if (tick_nohz_full_cpu(cpu))
 				tick_do_timer_boot_cpu = cpu;
 
 		} else if (tick_do_timer_boot_cpu != -1 &&
 						!tick_nohz_full_cpu(cpu)) {
-			tick_take_do_timer_from_boot();
 			tick_do_timer_boot_cpu = -1;
-			WARN_ON(READ_ONCE(tick_do_timer_cpu) != cpu);
+			/*
+			 * The boot CPU will stay in periodic (NOHZ disabled)
+			 * mode until clocksource_done_booting() called after
+			 * smp_init() selects a high resolution clocksource and
+			 * timekeeping_notify() kicks the NOHZ stuff alive.
+			 *
+			 * So this WRITE_ONCE can only race with the READ_ONCE
+			 * check in tick_periodic() but this race is harmless.
+			 */
+			WRITE_ONCE(tick_do_timer_cpu, cpu);
 #endif
 		}

Signature · 1 line

2.25.1.362.g51ebf55

Oleg Nesterov

author

2024-05-30 12:42 UTC #25 lore raw

PATCH 1/1 tick/nohz_full: turn tick_do_timer_boot_cpu into boot_cpu_is_nohz_full

We don't need to record the cpu number of the nohz_full boot CPU, a simple boolean is enough.

We could even kill it along with #ifdef, the "else if" branch could check tick_nohz_full_cpu(tick_do_timer_cpu) && !tick_nohz_full_cpu(cpu).

---
 kernel/time/tick-common.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

kernel/time/tick-common.c +8 ~~-16~~

index 27d0018c8b05..9829a2cda072 100644
@@ -49,15 +49,6 @@ ktime_t tick_next_period;
  *    procedure also covers cpu hotplug.
  */
 int tick_do_timer_cpu __read_mostly = TICK_DO_TIMER_BOOT;
-#ifdef CONFIG_NO_HZ_FULL
-/*
- * tick_do_timer_boot_cpu indicates the boot CPU temporarily owns
- * tick_do_timer_cpu and it should be taken over by an eligible secondary
- * when one comes online.
- */
-static int tick_do_timer_boot_cpu __read_mostly = -1;
-#endif
-
 /*
  * Debugging: see timer_list.c
  */

@@ -178,9 +169,13 @@ void tick_setup_periodic(struct clock_event_device *dev, int broadcast)
 	}
 }
 
+#ifdef CONFIG_NO_HZ_FULL
 /*
- * Setup the tick device
+ * Iindicates that the boot CPU temporarily owns tick_do_timer_cpu.
  */
+static bool boot_cpu_is_nohz_full __read_mostly;
+#endif
+
 static void tick_setup_device(struct tick_device *td,
 			      struct clock_event_device *newdev, int cpu,
 			      const struct cpumask *cpumask)

@@ -205,12 +200,9 @@ static void tick_setup_device(struct tick_device *td,
 			 * first housekeeping secondary will take do_timer()
 			 * from us.
 			 */
-			if (tick_nohz_full_cpu(cpu))
-				tick_do_timer_boot_cpu = cpu;
-
-		} else if (tick_do_timer_boot_cpu != -1 &&
-						!tick_nohz_full_cpu(cpu)) {
-			tick_do_timer_boot_cpu = -1;
+			boot_cpu_is_nohz_full = tick_nohz_full_cpu(cpu);
+		} else if (boot_cpu_is_nohz_full && !tick_nohz_full_cpu(cpu)) {
+			boot_cpu_is_nohz_full = false;
 			/*
 			 * The boot CPU will stay in periodic (NOHZ disabled)
 			 * mode until clocksource_done_booting() called after

Signature · 1 line

2.25.1.362.g51ebf55

Oleg Nesterov

author

2024-06-03 15:37 UTC #26 lore raw

PATCH v2 1/1 tick/nohz_full: turn tick_do_timer_boot_cpu into boot_cpu_is_nohz_full

We don't need to record the cpu number of the nohz_full boot CPU, a simple boolean is enough. We could even kill it, the "else if" branch could check tick_nohz_full_cpu(tick_do_timer_cpu) && !tick_nohz_full_cpu(cpu).

---
 kernel/time/tick-common.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

kernel/time/tick-common.c +4 ~~-17~~

index 27d0018c8b05..82c1b2206631 100644
@@ -49,15 +49,6 @@ ktime_t tick_next_period;
  *    procedure also covers cpu hotplug.
  */
 int tick_do_timer_cpu __read_mostly = TICK_DO_TIMER_BOOT;
-#ifdef CONFIG_NO_HZ_FULL
-/*
- * tick_do_timer_boot_cpu indicates the boot CPU temporarily owns
- * tick_do_timer_cpu and it should be taken over by an eligible secondary
- * when one comes online.
- */
-static int tick_do_timer_boot_cpu __read_mostly = -1;
-#endif
-
 /*
  * Debugging: see timer_list.c
  */

@@ -192,6 +183,7 @@ static void tick_setup_device(struct tick_device *td,
 	 * First device setup ?
 	 */
 	if (!td->evtdev) {
+		static bool boot_cpu_is_nohz_full __read_mostly;
 		/*
 		 * If no cpu took the do_timer update, assign it to
 		 * this cpu:

@@ -199,18 +191,14 @@ static void tick_setup_device(struct tick_device *td,
 		if (READ_ONCE(tick_do_timer_cpu) == TICK_DO_TIMER_BOOT) {
 			WRITE_ONCE(tick_do_timer_cpu, cpu);
 			tick_next_period = ktime_get();
-#ifdef CONFIG_NO_HZ_FULL
 			/*
 			 * The boot CPU may be nohz_full, in which case the
 			 * first housekeeping secondary will take do_timer()
 			 * from us.
 			 */
-			if (tick_nohz_full_cpu(cpu))
-				tick_do_timer_boot_cpu = cpu;
-
-		} else if (tick_do_timer_boot_cpu != -1 &&
-						!tick_nohz_full_cpu(cpu)) {
-			tick_do_timer_boot_cpu = -1;
+			boot_cpu_is_nohz_full = tick_nohz_full_cpu(cpu);
+		} else if (boot_cpu_is_nohz_full && !tick_nohz_full_cpu(cpu)) {
+			boot_cpu_is_nohz_full = false;
 			/*
 			 * The boot CPU will stay in periodic (NOHZ disabled)
 			 * mode until clocksource_done_booting() called after

@@ -221,7 +209,6 @@ static void tick_setup_device(struct tick_device *td,
 			 * check in tick_periodic() but this race is harmless.
 			 */
 			WRITE_ONCE(tick_do_timer_cpu, cpu);
-#endif
 		}
 
 		/*

Signature · 1 line

2.25.1.362.g51ebf55

Frederic Weisbecker

maintainer Acked-by on v2 1/1

2024-06-03 21:44 UTC #27 lore raw

Re: [PATCH v2] tick/nohz_full: turn tick_do_timer_boot_cpu into boot_cpu_is_nohz_full

Quoted 5 lines · Oleg Nesterov

We don't need to record the cpu number of the nohz_full boot CPU, a simple boolean is enough. We could even kill it, the "else if" branch could check tick_nohz_full_cpu(tick_do_timer_cpu) && !tick_nohz_full_cpu(cpu).
Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Frederic Weisbecker <[email protected]>

Nicholas Piggin

Reviewed-by on v2 1/1

2024-06-04 05:08 UTC #28 lore raw

Re: [PATCH v2] tick/nohz_full: turn tick_do_timer_boot_cpu into boot_cpu_is_nohz_full

Quoted 5 lines · Oleg Nesterov

We don't need to record the cpu number of the nohz_full boot CPU, a simple boolean is enough. We could even kill it, the "else if" branch could check tick_nohz_full_cpu(tick_do_timer_cpu) && !tick_nohz_full_cpu(cpu).
Signed-off-by Oleg Nesterov <[email protected]>

Reviewed-by Nicholas Piggin <[email protected]>

Quoted 62 lines

---
 kernel/time/tick-common.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

kernel/time/tick-common.c +4 ~~-17~~

index 27d0018c8b05..82c1b2206631 100644
@@ -49,15 +49,6 @@ ktime_t tick_next_period;
  *    procedure also covers cpu hotplug.
  */
 int tick_do_timer_cpu __read_mostly = TICK_DO_TIMER_BOOT;
-#ifdef CONFIG_NO_HZ_FULL
-/*
- * tick_do_timer_boot_cpu indicates the boot CPU temporarily owns
- * tick_do_timer_cpu and it should be taken over by an eligible secondary
- * when one comes online.
- */
-static int tick_do_timer_boot_cpu __read_mostly = -1;
-#endif
-
 /*
  * Debugging: see timer_list.c
  */

@@ -192,6 +183,7 @@ static void tick_setup_device(struct tick_device *td,
 	 * First device setup ?
 	 */
 	if (!td->evtdev) {
+		static bool boot_cpu_is_nohz_full __read_mostly;
 		/*
 		 * If no cpu took the do_timer update, assign it to
 		 * this cpu:

@@ -199,18 +191,14 @@ static void tick_setup_device(struct tick_device *td,
 		if (READ_ONCE(tick_do_timer_cpu) == TICK_DO_TIMER_BOOT) {
 			WRITE_ONCE(tick_do_timer_cpu, cpu);
 			tick_next_period = ktime_get();
-#ifdef CONFIG_NO_HZ_FULL
 			/*
 			 * The boot CPU may be nohz_full, in which case the
 			 * first housekeeping secondary will take do_timer()
 			 * from us.
 			 */
-			if (tick_nohz_full_cpu(cpu))
-				tick_do_timer_boot_cpu = cpu;
-
-		} else if (tick_do_timer_boot_cpu != -1 &&
-						!tick_nohz_full_cpu(cpu)) {
-			tick_do_timer_boot_cpu = -1;
+			boot_cpu_is_nohz_full = tick_nohz_full_cpu(cpu);
+		} else if (boot_cpu_is_nohz_full && !tick_nohz_full_cpu(cpu)) {
+			boot_cpu_is_nohz_full = false;
 			/*
 			 * The boot CPU will stay in periodic (NOHZ disabled)
 			 * mode until clocksource_done_booting() called after

@@ -221,7 +209,6 @@ static void tick_setup_device(struct tick_device *td,
 			 * check in tick_periodic() but this race is harmless.
 			 */
 			WRITE_ONCE(tick_do_timer_cpu, cpu);
-#endif
 		}
 
 		/*

Frederic Weisbecker

maintainer

2024-05-30 14:52 UTC #29 lore raw

Re: [PATCH] tick/nohz_full: don't abuse smp_call_function_single() in tick_setup_device()

Link https://lore.kernel.org/all/[email protected] open here

Quoted 69 lines · Oleg Nesterov

After the recent commit 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() calls tick_take_do_timer_from_boot() to update tick_do_timer_cpu and this triggers the WARN_ON_ONCE(irqs_disabled) in smp_call_function_single().

Kill tick_take_do_timer_from_boot() and just use WRITE_ONCE(), the new comment tries to explain why this is safe (thanks Thomas!).

Fixes 08ae95f4fd3b ("nohz_full: Allow the boot CPU to be nohz_full")

In reply to Frederic Weisbecker

---
 kernel/time/tick-common.c | 39 +++++++++++++--------------------------
 1 file changed, 13 insertions(+), 26 deletions(-)

kernel/time/tick-common.c +3 ~~-26~~

index d88b13076b79..27d0018c8b05 100644
@@ -178,26 +178,6 @@ void tick_setup_periodic(struct clock_event_device *dev, int broadcast)
 	}
 }
 
-#ifdef CONFIG_NO_HZ_FULL
-static void giveup_do_timer(void *info)
-{
-	int cpu = *(unsigned int *)info;
-
-	WARN_ON(tick_do_timer_cpu != smp_processor_id());
-
-	tick_do_timer_cpu = cpu;
-}
-
-static void tick_take_do_timer_from_boot(void)
-{
-	int cpu = smp_processor_id();
-	int from = tick_do_timer_boot_cpu;
-
-	if (from >= 0 && from != cpu)
-		smp_call_function_single(from, giveup_do_timer, &cpu, 1);
-}
-#endif
-
 /*
  * Setup the tick device
  */

@@ -221,19 +201,26 @@ static void tick_setup_device(struct tick_device *td,
 			tick_next_period = ktime_get();
 #ifdef CONFIG_NO_HZ_FULL
 			/*
-			 * The boot CPU may be nohz_full, in which case set
-			 * tick_do_timer_boot_cpu so the first housekeeping
-			 * secondary that comes up will take do_timer from
-			 * us.
+			 * The boot CPU may be nohz_full, in which case the
+			 * first housekeeping secondary will take do_timer()
+			 * from us.
 			 */
 			if (tick_nohz_full_cpu(cpu))
 				tick_do_timer_boot_cpu = cpu;
 
 		} else if (tick_do_timer_boot_cpu != -1 &&
 						!tick_nohz_full_cpu(cpu)) {
-			tick_take_do_timer_from_boot();
 			tick_do_timer_boot_cpu = -1;
-			WARN_ON(READ_ONCE(tick_do_timer_cpu) != cpu);

kernel/time/tick-common.c static void tick_setup_device(struct tick_device *td, · line 223

Quoted 10 lines

kernel/time/tick-common.c +10 -0

+			/*
+			 * The boot CPU will stay in periodic (NOHZ disabled)
+			 * mode until clocksource_done_booting() called after
+			 * smp_init() selects a high resolution clocksource and
+			 * timekeeping_notify() kicks the NOHZ stuff alive.
+			 *
+			 * So this WRITE_ONCE can only race with the READ_ONCE
+			 * check in tick_periodic() but this race is harmless.
+			 */
+			WRITE_ONCE(tick_do_timer_cpu, cpu);

Looks good, but can we have a WARN_ONCE(tick_do_timer_cpu != tick_do_timer_boot_cpu) right before that, just to make sure our assumptions above are right forever and the boot CPU hasn't stopped the tick up to that point?

And after all, pushing a bit further your subsequent patch, can we get rid of tick_do_timer_boot_cpu and ifdefery altogether? Such as:

kernel/time/tick-common.c +14 ~~-43~~

index fb0fdec8719a..63a7bd405de7 100644
@@ -48,14 +48,6 @@ ktime_t tick_next_period;
  *    procedure also covers cpu hotplug.
  */
 int tick_do_timer_cpu __read_mostly = TICK_DO_TIMER_BOOT;
-#ifdef CONFIG_NO_HZ_FULL
-/*
- * tick_do_timer_boot_cpu indicates the boot CPU temporarily owns
- * tick_do_timer_cpu and it should be taken over by an eligible secondary
- * when one comes online.
- */
-static int tick_do_timer_boot_cpu __read_mostly = -1;
-#endif
 
 /*
  * Debugging: see timer_list.c

@@ -177,26 +169,6 @@ void tick_setup_periodic(struct clock_event_device *dev, int broadcast)
 	}
 }
 
-#ifdef CONFIG_NO_HZ_FULL
-static void giveup_do_timer(void *info)
-{
-	int cpu = *(unsigned int *)info;
-
-	WARN_ON(tick_do_timer_cpu != smp_processor_id());
-
-	tick_do_timer_cpu = cpu;
-}
-
-static void tick_take_do_timer_from_boot(void)
-{
-	int cpu = smp_processor_id();
-	int from = tick_do_timer_boot_cpu;
-
-	if (from >= 0 && from != cpu)
-		smp_call_function_single(from, giveup_do_timer, &cpu, 1);
-}
-#endif
-
 /*
  * Setup the tick device
  */

@@ -211,29 +183,28 @@ static void tick_setup_device(struct tick_device *td,
 	 * First device setup ?
 	 */
 	if (!td->evtdev) {
+		int timekeeper = READ_ONCE(tick_do_timer_cpu);
 		/*
 		 * If no cpu took the do_timer update, assign it to
 		 * this cpu:
 		 */
-		if (tick_do_timer_cpu == TICK_DO_TIMER_BOOT) {
+		if (timekeeper == TICK_DO_TIMER_BOOT) {
 			tick_do_timer_cpu = cpu;
 			tick_next_period = ktime_get();
-#ifdef CONFIG_NO_HZ_FULL
+		} else if (timekeeper == TICK_DO_TIMER_NONE) {
+			if (WARN_ON_ONCE(tick_nohz_full_enabled()))
+				WRITE_ONCE(tick_do_timer_cpu, cpu);
+		} else if (tick_nohz_full_cpu(timekeeper) && !tick_nohz_full_cpu(cpu)) {
 			/*
-			 * The boot CPU may be nohz_full, in which case set
-			 * tick_do_timer_boot_cpu so the first housekeeping
-			 * secondary that comes up will take do_timer from
-			 * us.
+			 * The boot CPU will stay in periodic (NOHZ disabled)
+			 * mode until clocksource_done_booting() called after
+			 * smp_init() selects a high resolution clocksource and
+			 * timekeeping_notify() kicks the NOHZ stuff alive.
+			 *
+			 * So this WRITE_ONCE can only race with the READ_ONCE
+			 * check in tick_periodic() but this race is harmless.
 			 */
-			if (tick_nohz_full_cpu(cpu))
-				tick_do_timer_boot_cpu = cpu;
-
-		} else if (tick_do_timer_boot_cpu != -1 &&
-						!tick_nohz_full_cpu(cpu)) {
-			tick_take_do_timer_from_boot();
-			tick_do_timer_boot_cpu = -1;
-			WARN_ON(tick_do_timer_cpu != cpu);
-#endif
+			WRITE_ONCE(tick_do_timer_cpu, cpu);
 		}
 
 		/*

Oleg Nesterov

2024-05-30 16:53 UTC #30 lore raw

Re: [PATCH] tick/nohz_full: don't abuse smp_call_function_single() in tick_setup_device()

Frederic,

Thanks for review.

Quoted 4 lines · Frederic Weisbecker

Looks good, but can we have a WARN_ONCE(tick_do_timer_cpu != tick_do_timer_boot_cpu) right before that, just to make sure our assumptions above are right forever and the boot CPU hasn't stopped the tick up to that point?

Sure, I thought about the additional sanity checks too. Although I had something different in mind.

Frederic, et al, I am on private trip again without my working laptop, can't read the code. I'll reply on Saturday, OK?

Oleg.

Quoted 95 lines

And after all, pushing a bit further your subsequent patch, can we get rid of tick_do_timer_boot_cpu and ifdefery altogether? Such as:

kernel/time/tick-common.c +14 ~~-43~~

index fb0fdec8719a..63a7bd405de7 100644
@@ -48,14 +48,6 @@ ktime_t tick_next_period;
  *    procedure also covers cpu hotplug.
  */
 int tick_do_timer_cpu __read_mostly = TICK_DO_TIMER_BOOT;
-#ifdef CONFIG_NO_HZ_FULL
-/*
- * tick_do_timer_boot_cpu indicates the boot CPU temporarily owns
- * tick_do_timer_cpu and it should be taken over by an eligible secondary
- * when one comes online.
- */
-static int tick_do_timer_boot_cpu __read_mostly = -1;
-#endif
 
 /*
  * Debugging: see timer_list.c

@@ -177,26 +169,6 @@ void tick_setup_periodic(struct clock_event_device *dev, int broadcast)
 	}
 }
 
-#ifdef CONFIG_NO_HZ_FULL
-static void giveup_do_timer(void *info)
-{
-	int cpu = *(unsigned int *)info;
-
-	WARN_ON(tick_do_timer_cpu != smp_processor_id());
-
-	tick_do_timer_cpu = cpu;
-}
-
-static void tick_take_do_timer_from_boot(void)
-{
-	int cpu = smp_processor_id();
-	int from = tick_do_timer_boot_cpu;
-
-	if (from >= 0 && from != cpu)
-		smp_call_function_single(from, giveup_do_timer, &cpu, 1);
-}
-#endif
-
 /*
  * Setup the tick device
  */

@@ -211,29 +183,28 @@ static void tick_setup_device(struct tick_device *td,
 	 * First device setup ?
 	 */
 	if (!td->evtdev) {
+		int timekeeper = READ_ONCE(tick_do_timer_cpu);
 		/*
 		 * If no cpu took the do_timer update, assign it to
 		 * this cpu:
 		 */
-		if (tick_do_timer_cpu == TICK_DO_TIMER_BOOT) {
+		if (timekeeper == TICK_DO_TIMER_BOOT) {
 			tick_do_timer_cpu = cpu;
 			tick_next_period = ktime_get();
-#ifdef CONFIG_NO_HZ_FULL
+		} else if (timekeeper == TICK_DO_TIMER_NONE) {
+			if (WARN_ON_ONCE(tick_nohz_full_enabled()))
+				WRITE_ONCE(tick_do_timer_cpu, cpu);
+		} else if (tick_nohz_full_cpu(timekeeper) && !tick_nohz_full_cpu(cpu)) {
 			/*
-			 * The boot CPU may be nohz_full, in which case set
-			 * tick_do_timer_boot_cpu so the first housekeeping
-			 * secondary that comes up will take do_timer from
-			 * us.
+			 * The boot CPU will stay in periodic (NOHZ disabled)
+			 * mode until clocksource_done_booting() called after
+			 * smp_init() selects a high resolution clocksource and
+			 * timekeeping_notify() kicks the NOHZ stuff alive.
+			 *
+			 * So this WRITE_ONCE can only race with the READ_ONCE
+			 * check in tick_periodic() but this race is harmless.
 			 */
-			if (tick_nohz_full_cpu(cpu))
-				tick_do_timer_boot_cpu = cpu;
-
-		} else if (tick_do_timer_boot_cpu != -1 &&
-						!tick_nohz_full_cpu(cpu)) {
-			tick_take_do_timer_from_boot();
-			tick_do_timer_boot_cpu = -1;
-			WARN_ON(tick_do_timer_cpu != cpu);
-#endif
+			WRITE_ONCE(tick_do_timer_cpu, cpu);
 		}
 
 		/*

Oleg Nesterov

author

2024-05-30 17:01 UTC #31 lore raw

Re: [PATCH] tick/nohz_full: don't abuse smp_call_function_single() in tick_setup_device()

Quoted 4 lines · Frederic Weisbecker

Looks good, but can we have a WARN_ONCE(tick_do_timer_cpu != tick_do_timer_boot_cpu) right before that, just to make sure our assumptions above are right forever and the boot CPU hasn't stopped the tick up to that point?

Sure, I thought about the additional sanity checks too. Although I had something different in mind.

Frederic, et al, I am on private trip again without my working laptop, can't read the code. I'll reply on Saturday, OK?

Thanks for review!

Oleg.

Oleg Nesterov

author

2024-06-01 14:05 UTC #32 lore raw

Re: [PATCH] tick/nohz_full: don't abuse smp_call_function_single() in tick_setup_device()

In reply to Frederic Weisbecker

Hi Frederic,

First of all, can we please make the additional changes you suggest on top of this patch? I'd prefer to keep it as simple as possible, I will need to backport it and I'd like to simplify the internal review.

Quoted 3 lines · Frederic Weisbecker

And after all, pushing a bit further your subsequent patch, can we get rid of tick_do_timer_boot_cpu and ifdefery altogether? Such as:

Sure, I thought about this from the very beginning, see
https://lore.kernel.org/all/[email protected]/
and the changelog in
[PATCH] tick/nohz_full: turn tick_do_timer_boot_cpu into boot_cpu_is_nohz_full
https://lore.kernel.org/all/[email protected]/
on top of this patch.

And yes, in this case it is better to check that tick_do_timer_cpu != _NONE to ensure that tick_nohz_full_cpu(tick_cpu) can't crash.

So I considered the change which is very close to yours, except

kernel/time/tick-common.c static void tick_setup_device(struct tick_device *td, · line 196

Quoted 3 lines

kernel/time/tick-common.c +3 -0

+		} else if (timekeeper == TICK_DO_TIMER_NONE) {
+			if (WARN_ON_ONCE(tick_nohz_full_enabled()))
+				WRITE_ONCE(tick_do_timer_cpu, cpu);

I don't think we need to change tick_do_timer_cpu in this case. And I am not sure we need to check tick_nohz_full_enabled() here. IOW, I was thinking about

	if (!td->evtdev) {
		int tick_cpu = READ_ONCE(tick_do_timer_cpu);
		/*
		 * If no cpu took the do_timer update, assign it to
		 * this cpu:
		 */
		if (tick_cpu == TICK_DO_TIMER_BOOT) {
			WRITE_ONCE(tick_do_timer_cpu, cpu);
			tick_next_period = ktime_get();
			/*
			 * The boot CPU may be nohz_full, in which case the
			 * first housekeeping secondary will take do_timer()
			 * from us.
			 */
		} else if (!WARN_ON_ONCE(tick_cpu == TICK_DO_TIMER_NONE)) &&
			   tick_nohz_full_cpu(tick_cpu) &&
			   !tick_nohz_full_cpu(cpu)) {
			/*
			 * The boot CPU will stay in periodic (NOHZ disabled)
			 * mode until clocksource_done_booting() called after
			 * smp_init() selects a high resolution clocksource and
			 * timekeeping_notify() kicks the NOHZ stuff alive.
			 *
			 * So this WRITE_ONCE can only race with the READ_ONCE
			 * check in tick_periodic() but this race is harmless.
			 */
			WRITE_ONCE(tick_do_timer_cpu, cpu);
		}

But you know, somehow I like
[PATCH] tick/nohz_full: turn tick_do_timer_boot_cpu into boot_cpu_is_nohz_full
https://lore.kernel.org/all/[email protected]/
a bit more, to me the code looks more understandable this way.

Note that this patch doesn't really need to keep #ifdef CONFIG_NO_HZ_FULL,

	if (!td->evtdev) {
		static bool boot_cpu_is_nohz_full;
		/*
		 * If no cpu took the do_timer update, assign it to
		 * this cpu:
		 */
		if (READ_ONCE(tick_do_timer_cpu) == TICK_DO_TIMER_BOOT) {
			WRITE_ONCE(tick_do_timer_cpu, cpu);
			tick_next_period = ktime_get();
			/*
			 * The boot CPU may be nohz_full, in which case the
			 * first housekeeping secondary will take do_timer()
			 * from us.
			 */
			boot_cpu_is_nohz_full = tick_nohz_full_cpu(cpu);
		} else if (boot_cpu_is_nohz_full && !tick_nohz_full_cpu(cpu)) {
			boot_cpu_is_nohz_full = false;
			/*
			 * The boot CPU will stay in periodic (NOHZ disabled)
			 * mode until clocksource_done_booting() called after
			 * smp_init() selects a high resolution clocksource and
			 * timekeeping_notify() kicks the NOHZ stuff alive.
			 *
			 * So this WRITE_ONCE can only race with the READ_ONCE
			 * check in tick_periodic() but this race is harmless.
			 */
			WRITE_ONCE(tick_do_timer_cpu, cpu);
		}

should work without #ifdef.

In this case I don't think we need the _NONE check, tick_sched_do_timer() will complain.

But I won't argue. I will be happy to make V2 which follows your recommendations but again, can I do this on top of this patch?

Oleg.

Frederic Weisbecker

maintainer

2024-06-02 21:29 UTC #33 lore raw

Re: [PATCH] tick/nohz_full: don't abuse smp_call_function_single() in tick_setup_device()

Quoted 5 lines · Oleg Nesterov

Hi Frederic,
First of all, can we please make the additional changes you suggest on top of this patch? I'd prefer to keep it as simple as possible, I will need to backport it and I'd like to simplify the internal review.

Sure!

Quoted 18 lines · Frederic Weisbecker

Quoted 3 lines
And after all, pushing a bit further your subsequent patch, can we get rid of tick_do_timer_boot_cpu and ifdefery altogether? Such as:
Sure, I thought about this from the very beginning, see
https://lore.kernel.org/all/[email protected]/
and the changelog in
[PATCH] tick/nohz_full: turn tick_do_timer_boot_cpu into boot_cpu_is_nohz_full
https://lore.kernel.org/all/[email protected]/
on top of this patch.
And yes, in this case it is better to check that tick_do_timer_cpu != _NONE to ensure that tick_nohz_full_cpu(tick_cpu) can't crash.
So I considered the change which is very close to yours, except

Quoted 7 lines

kernel/time/tick-common.c static void tick_setup_device(struct tick_device *td, · line 196
Quoted 3 lines
kernel/time/tick-common.c +3 -0
+		} else if (timekeeper == TICK_DO_TIMER_NONE) {
+			if (WARN_ON_ONCE(tick_nohz_full_enabled()))
+				WRITE_ONCE(tick_do_timer_cpu, cpu);
I don't think we need to change tick_do_timer_cpu in this case. And I am not sure we need to check tick_nohz_full_enabled() here. IOW, I was thinking about

Hmm, in case of cpu-hotplug operations (that is after boot), we may be past nohz enablement and therefore it might be TICK_DO_TIMER_NONE.

Quoted 66 lines

	if (!td->evtdev) {
		int tick_cpu = READ_ONCE(tick_do_timer_cpu);
		/*
		 * If no cpu took the do_timer update, assign it to
		 * this cpu:
		 */
		if (tick_cpu == TICK_DO_TIMER_BOOT) {
			WRITE_ONCE(tick_do_timer_cpu, cpu);
			tick_next_period = ktime_get();
			/*
			 * The boot CPU may be nohz_full, in which case the
			 * first housekeeping secondary will take do_timer()
			 * from us.
			 */
		} else if (!WARN_ON_ONCE(tick_cpu == TICK_DO_TIMER_NONE)) &&
			   tick_nohz_full_cpu(tick_cpu) &&
			   !tick_nohz_full_cpu(cpu)) {
			/*
			 * The boot CPU will stay in periodic (NOHZ disabled)
			 * mode until clocksource_done_booting() called after
			 * smp_init() selects a high resolution clocksource and
			 * timekeeping_notify() kicks the NOHZ stuff alive.
			 *
			 * So this WRITE_ONCE can only race with the READ_ONCE
			 * check in tick_periodic() but this race is harmless.
			 */
			WRITE_ONCE(tick_do_timer_cpu, cpu);
		}

Note that this patch doesn't really need to keep #ifdef CONFIG_NO_HZ_FULL,

	if (!td->evtdev) {
		static bool boot_cpu_is_nohz_full;
		/*
		 * If no cpu took the do_timer update, assign it to
		 * this cpu:
		 */
		if (READ_ONCE(tick_do_timer_cpu) == TICK_DO_TIMER_BOOT) {
			WRITE_ONCE(tick_do_timer_cpu, cpu);
			tick_next_period = ktime_get();
			/*
			 * The boot CPU may be nohz_full, in which case the
			 * first housekeeping secondary will take do_timer()
			 * from us.
			 */
			boot_cpu_is_nohz_full = tick_nohz_full_cpu(cpu);
		} else if (boot_cpu_is_nohz_full && !tick_nohz_full_cpu(cpu)) {
			boot_cpu_is_nohz_full = false;
			/*
			 * The boot CPU will stay in periodic (NOHZ disabled)
			 * mode until clocksource_done_booting() called after
			 * smp_init() selects a high resolution clocksource and
			 * timekeeping_notify() kicks the NOHZ stuff alive.
			 *
			 * So this WRITE_ONCE can only race with the READ_ONCE
			 * check in tick_periodic() but this race is harmless.
			 */
			WRITE_ONCE(tick_do_timer_cpu, cpu);
		}

Quoted 4 lines

should work without #ifdef.
In this case I don't think we need the _NONE check, tick_sched_do_timer() will complain.

Right...

Quoted 3 lines

But I won't argue. I will be happy to make V2 which follows your recommendations but again, can I do this on top of this patch?

I guess the static version above should work to remove the ifdef. And yes on top is fine.

Thanks!

Oleg Nesterov

author

2024-06-03 15:43 UTC #34 lore raw

Re: [PATCH] tick/nohz_full: don't abuse smp_call_function_single() in tick_setup_device()

Quoted 2 lines · Frederic Weisbecker

I guess the static version above should work to remove the ifdef. And yes on top is fine.

OK, I've sent v2.

But again, I won't argue if you prefer to keep tick_do_timer_boot_cpu and add WARN_ON_ONCE(tick_cpu != tick_do_timer_boot_cpu) before WRITE_ONCE(tick_do_timer_cpu). In this case another patch makes no sense, I'll update this one.

Just tell me what you like more. Sorry for the chaotic emails.

Oleg.

Frederic Weisbecker

maintainer

2024-06-03 21:45 UTC #35 lore raw

Re: [PATCH] tick/nohz_full: don't abuse smp_call_function_single() in tick_setup_device()

Quoted 11 lines · Oleg Nesterov

Quoted 2 lines · Frederic Weisbecker
I guess the static version above should work to remove the ifdef. And yes on top is fine.
OK, I've sent v2.
But again, I won't argue if you prefer to keep tick_do_timer_boot_cpu and add WARN_ON_ONCE(tick_cpu != tick_do_timer_boot_cpu) before WRITE_ONCE(tick_do_timer_cpu). In this case another patch makes no sense, I'll update this one.
Just tell me what you like more. Sorry for the chaotic emails.

I'm fine with the last one posted :-)

Thanks.

Oleg Nesterov

author

2024-06-10 15:56 UTC #36 lore raw

[PING ;)] Re: [PATCH] tick/nohz_full: don't abuse smp_call_function_single() in tick_setup_device()

Link https://lore.kernel.org/all/[email protected] open here

Thomas, et al,

should I redo and/or resend this patch (and the next one on top of it) ?

Or what else can I do to fix this problem?

Thanks,

Oleg.

Quoted 86 lines · Oleg Nesterov

After the recent commit 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() calls tick_take_do_timer_from_boot() to update tick_do_timer_cpu and this triggers the WARN_ON_ONCE(irqs_disabled) in smp_call_function_single().

Kill tick_take_do_timer_from_boot() and just use WRITE_ONCE(), the new comment tries to explain why this is safe (thanks Thomas!).

Fixes 08ae95f4fd3b ("nohz_full: Allow the boot CPU to be nohz_full")

---
 kernel/time/tick-common.c | 39 +++++++++++++--------------------------
 1 file changed, 13 insertions(+), 26 deletions(-)

kernel/time/tick-common.c +13 ~~-26~~

index d88b13076b79..27d0018c8b05 100644
@@ -178,26 +178,6 @@ void tick_setup_periodic(struct clock_event_device *dev, int broadcast)
 	}
 }
 
-#ifdef CONFIG_NO_HZ_FULL
-static void giveup_do_timer(void *info)
-{
-	int cpu = *(unsigned int *)info;
-
-	WARN_ON(tick_do_timer_cpu != smp_processor_id());
-
-	tick_do_timer_cpu = cpu;
-}
-
-static void tick_take_do_timer_from_boot(void)
-{
-	int cpu = smp_processor_id();
-	int from = tick_do_timer_boot_cpu;
-
-	if (from >= 0 && from != cpu)
-		smp_call_function_single(from, giveup_do_timer, &cpu, 1);
-}
-#endif
-
 /*
  * Setup the tick device
  */

@@ -221,19 +201,26 @@ static void tick_setup_device(struct tick_device *td,
 			tick_next_period = ktime_get();
 #ifdef CONFIG_NO_HZ_FULL
 			/*
-			 * The boot CPU may be nohz_full, in which case set
-			 * tick_do_timer_boot_cpu so the first housekeeping
-			 * secondary that comes up will take do_timer from
-			 * us.
+			 * The boot CPU may be nohz_full, in which case the
+			 * first housekeeping secondary will take do_timer()
+			 * from us.
 			 */
 			if (tick_nohz_full_cpu(cpu))
 				tick_do_timer_boot_cpu = cpu;
 
 		} else if (tick_do_timer_boot_cpu != -1 &&
 						!tick_nohz_full_cpu(cpu)) {
-			tick_take_do_timer_from_boot();
 			tick_do_timer_boot_cpu = -1;
-			WARN_ON(READ_ONCE(tick_do_timer_cpu) != cpu);
+			/*
+			 * The boot CPU will stay in periodic (NOHZ disabled)
+			 * mode until clocksource_done_booting() called after
+			 * smp_init() selects a high resolution clocksource and
+			 * timekeeping_notify() kicks the NOHZ stuff alive.
+			 *
+			 * So this WRITE_ONCE can only race with the READ_ONCE
+			 * check in tick_periodic() but this race is harmless.
+			 */
+			WRITE_ONCE(tick_do_timer_cpu, cpu);
 #endif
 		}

Signature · 1 line

2.25.1.362.g51ebf55

Thomas Gleixner

2024-06-10 18:15 UTC #37 lore raw

Re: [PING ;)] Re: [PATCH] tick/nohz_full: don't abuse smp_call_function_single() in tick_setup_device()

Quoted 1 line · Oleg Nesterov

should I redo and/or resend this patch (and the next one on top of it) ?

I lost track of the maze in this discussion. As the patch is obviously correct, I just apply it as is and any cosmetic changes can go on top.

Thanks,

tglx

tip-bot2 for Oleg Nesterov

merge bot Applied

2024-06-10 18:26 UTC #38 lore raw

[tip: timers/urgent] tick/nohz_full: Don't abuse smp_call_function_single() in tick_setup_device()

The following commit has been merged into the timers/urgent branch of tip:

Commit-ID:     07c54cc5988f19c9642fd463c2dbdac7fc52f777
Gitweb:        https://git.kernel.org/tip/07c54cc5988f19c9642fd463c2dbdac7fc52f777
Author:        Oleg Nesterov <[email protected]>
AuthorDate:    Tue, 28 May 2024 14:20:19 +02:00
Committer:     Thomas Gleixner <[email protected]>
CommitterDate: Mon, 10 Jun 2024 20:18:13 +02:00

tick/nohz_full: Don't abuse smp_call_function_single() in tick_setup_device()

After the recent commit 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() calls tick_take_do_timer_from_boot() to update tick_do_timer_cpu and this triggers the WARN_ON_ONCE(irqs_disabled) in smp_call_function_single().

Kill tick_take_do_timer_from_boot() and just use WRITE_ONCE(), the new comment explains why this is safe (thanks Thomas!).

Fixes 08ae95f4fd3b ("nohz_full: Allow the boot CPU to be nohz_full")