hrtimer_run_queues() calls tick_check_oneshot_change() to check if we
can switch to highres or nohz mode, but the current code looks very
confusing to me. In the highres=n or CONFIG_HIGH_RES_TIMERS=n cases
tick_check_oneshot_change() itself calls tick_nohz_switch_to_nohz()
and returns zero; that is why it needs the "allow_nohz" argument,
which imo also adds confusion.

This patch turns tick_check_oneshot_change() into a "pure" function
without arguments. hrtimer_run_queues() calls hrtimer_switch_to_hres()
or tick_nohz_switch_to_nohz() depending on hrtimer_is_hres_enabled().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
v2: fix build failure without CONFIG_NO_HZ_COMMON (thanks Frederic)
---
 kernel/time/hrtimer.c       |  7 +++++--
 kernel/time/tick-internal.h |  6 ++++--
 kernel/time/tick-sched.c    | 15 ++++-----------
 3 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 492c14aac642..806f352b095d 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1891,8 +1891,11 @@ void hrtimer_run_queues(void)
 	 * there only sets the check bit in the tick_oneshot code,
 	 * otherwise we might deadlock vs. xtime_lock.
 	 */
-	if (tick_check_oneshot_change(!hrtimer_is_hres_enabled())) {
-		hrtimer_switch_to_hres();
+	if (tick_check_oneshot_change()) {
+		if (hrtimer_is_hres_enabled())
+			hrtimer_switch_to_hres();
+		else
+			tick_nohz_switch_to_nohz();
 		return;
 	}
 
diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h
index 5f2105e637bd..961dca323649 100644
--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -111,7 +111,7 @@ extern void tick_resume_oneshot(void);
 static inline bool tick_oneshot_possible(void) { return true; }
 extern int tick_oneshot_mode_active(void);
 extern void tick_clock_notify(void);
-extern int tick_check_oneshot_change(int allow_nohz);
+extern int tick_check_oneshot_change(void);
 extern int tick_init_highres(void);
 #else /* !CONFIG_TICK_ONESHOT: */
 static inline
@@ -124,7 +124,7 @@ static inline void tick_oneshot_notify(void) { }
 static inline bool tick_oneshot_possible(void) { return false; }
 static inline int tick_oneshot_mode_active(void) { return 0; }
 static inline void tick_clock_notify(void) { }
-static inline int tick_check_oneshot_change(int allow_nohz) { return 0; }
+static inline int tick_check_oneshot_change(void) { return 0; }
 #endif /* !CONFIG_TICK_ONESHOT */
 
 /* Functions related to oneshot broadcasting */
@@ -157,6 +157,7 @@ static inline void tick_nohz_init(void) { }
 #endif
 
 #ifdef CONFIG_NO_HZ_COMMON
+extern void tick_nohz_switch_to_nohz(void);
 extern unsigned long tick_nohz_active;
 extern void timers_update_nohz(void);
 extern u64 get_jiffies_update(unsigned long *basej);
@@ -171,6 +172,7 @@ extern bool timer_base_is_idle(void);
 extern void timer_expire_remote(unsigned int cpu);
 # endif
 #else /* CONFIG_NO_HZ_COMMON */
+static inline void tick_nohz_switch_to_nohz(void) { }
 static inline void timers_update_nohz(void) { }
 #define tick_nohz_active (0)
 #endif
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 71a792cd8936..4fd70be50b7f 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -1514,7 +1514,7 @@ static inline void tick_nohz_activate(struct tick_sched *ts)
 /**
  * tick_nohz_switch_to_nohz - switch to NOHZ mode
  */
-static void tick_nohz_switch_to_nohz(void)
+void tick_nohz_switch_to_nohz(void)
 {
 	if (!tick_nohz_enabled)
 		return;
@@ -1552,7 +1552,6 @@ static inline void tick_nohz_irq_enter(void)
 
 #else
 
-static inline void tick_nohz_switch_to_nohz(void) { }
 static inline void tick_nohz_irq_enter(void) { }
 static inline void tick_nohz_activate(struct tick_sched *ts) { }
 
@@ -1670,11 +1669,9 @@ void tick_oneshot_notify(void)
  * Check if a change happened, which makes oneshot possible.
  *
  * Called cyclically from the hrtimer softirq (driven by the timer
- * softirq). 'allow_nohz' signals that we can switch into low-res NOHZ
- * mode, because high resolution timers are disabled (either compile
- * or runtime). Called with interrupts disabled.
+ * softirq). Called with interrupts disabled.
  */
-int tick_check_oneshot_change(int allow_nohz)
+int tick_check_oneshot_change(void)
 {
 	struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched);
 
@@ -1687,9 +1684,5 @@ int tick_check_oneshot_change(int allow_nohz)
 	if (!timekeeping_valid_for_hres() || !tick_is_oneshot_available())
 		return 0;
 
-	if (!allow_nohz)
-		return 1;
-
-	tick_nohz_switch_to_nohz();
-	return 0;
+	return 1;
 }
-- 
2.25.1.362.g51ebf55

On Sun, Jun 02 2024 at 12:20, Oleg Nesterov wrote:
> @@ -1891,8 +1891,11 @@ void hrtimer_run_queues(void)
>  	 * there only sets the check bit in the tick_oneshot code,
>  	 * otherwise we might deadlock vs. xtime_lock.
>  	 */
> -	if (tick_check_oneshot_change(!hrtimer_is_hres_enabled())) {
> -		hrtimer_switch_to_hres();
> +	if (tick_check_oneshot_change()) {
> +		if (hrtimer_is_hres_enabled())
> +			hrtimer_switch_to_hres();
> +		else
> +			tick_nohz_switch_to_nohz();

hrtimers have no business with tick_nohz_switch_to_nohz(),
really. That's a strict tick/nohz specific thing. hrtimers do not care
about NOHZ much. They care about whether they can switch to high
resolution mode.

Thanks,

        tglx

On 06/03, Thomas Gleixner wrote:
>
> On Sun, Jun 02 2024 at 12:20, Oleg Nesterov wrote:
> > @@ -1891,8 +1891,11 @@ void hrtimer_run_queues(void)
> >  	 * there only sets the check bit in the tick_oneshot code,
> >  	 * otherwise we might deadlock vs. xtime_lock.
> >  	 */
> > -	if (tick_check_oneshot_change(!hrtimer_is_hres_enabled())) {
> > -		hrtimer_switch_to_hres();
> > +	if (tick_check_oneshot_change()) {
> > +		if (hrtimer_is_hres_enabled())
> > +			hrtimer_switch_to_hres();
> > +		else
> > +			tick_nohz_switch_to_nohz();
>
> hrtimers have no business with tick_nohz_switch_to_nohz(),
> really. That's a strict tick/nohz specific thing. hrtimers do not care
> about NOHZ much. They care about whether they can switch to high
> resolution mode.

OK, lets forget this patch then.

But note that the comment above the tick_check_oneshot_change() says
"switch to highres and / or nohz mode".

Thanks,

Oleg.

On 06/03, Oleg Nesterov wrote:
>
> On 06/03, Thomas Gleixner wrote:
> >
> > On Sun, Jun 02 2024 at 12:20, Oleg Nesterov wrote:
> > > @@ -1891,8 +1891,11 @@ void hrtimer_run_queues(void)
> > >  	 * there only sets the check bit in the tick_oneshot code,
> > >  	 * otherwise we might deadlock vs. xtime_lock.
> > >  	 */
> > > -	if (tick_check_oneshot_change(!hrtimer_is_hres_enabled())) {
> > > -		hrtimer_switch_to_hres();
> > > +	if (tick_check_oneshot_change()) {
> > > +		if (hrtimer_is_hres_enabled())
> > > +			hrtimer_switch_to_hres();
> > > +		else
> > > +			tick_nohz_switch_to_nohz();
> >
> > hrtimers have no business with tick_nohz_switch_to_nohz(),
> > really. That's a strict tick/nohz specific thing. hrtimers do not care
> > about NOHZ much. They care about whether they can switch to high
> > resolution mode.
>
> OK, lets forget this patch then.
>
> But note that the comment above the tick_check_oneshot_change() says
> "switch to highres and / or nohz mode".

But yes, please forget this patch ;)

Currently hrtimer_run_queues() doesn't return if tick_check_oneshot_change()
itself calls tick_nohz_switch_to_nohz() and returns 0.

Oleg.