[v1] tick: shift tick_nohz_switch_to_nohz() from tick_check_oneshot_change() to hrtimer_run_queues()

[PATCH] tick: shift tick_nohz_switch_to_nohz() from tick_check_oneshot_change() to hrtimer_run_queues()

Posted by Oleg Nesterov 1 year, 6 months ago

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>
---
 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..6764fbd18afd 100644
--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -111,7 +111,8 @@ 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 void tick_nohz_switch_to_nohz(void);
 extern int tick_init_highres(void);
 #else /* !CONFIG_TICK_ONESHOT: */
 static inline
@@ -124,7 +125,8 @@ 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; }
+static inline void tick_nohz_switch_to_nohz(void) { }
 #endif /* !CONFIG_TICK_ONESHOT */
 
 /* Functions related to oneshot broadcasting */
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

Re: [PATCH] tick: shift tick_nohz_switch_to_nohz() from tick_check_oneshot_change() to hrtimer_run_queues()

Posted by kernel test robot 1 year, 6 months ago

Hi Oleg,

kernel test robot noticed the following build errors:

[auto build test ERROR on tip/timers/core]
[also build test ERROR on linus/master v6.10-rc1 next-20240529]
[cannot apply to tip/timers/nohz]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Oleg-Nesterov/tick-shift-tick_nohz_switch_to_nohz-from-tick_check_oneshot_change-to-hrtimer_run_queues/20240530-204940
base:   tip/timers/core
patch link:    https://lore.kernel.org/r/20240530124203.GA26990%40redhat.com
patch subject: [PATCH] tick: shift tick_nohz_switch_to_nohz() from tick_check_oneshot_change() to hrtimer_run_queues()
config: i386-buildonly-randconfig-005-20240531 (https://download.01.org/0day-ci/archive/20240531/202405310959.9U6sUim5-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240531/202405310959.9U6sUim5-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405310959.9U6sUim5-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: kernel/time/hrtimer.o: in function `hrtimer_run_queues':
>> hrtimer.c:(.text+0x1851): undefined reference to `tick_nohz_switch_to_nohz'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] tick: shift tick_nohz_switch_to_nohz() from tick_check_oneshot_change() to hrtimer_run_queues()

Posted by kernel test robot 1 year, 6 months ago

Hi Oleg,

kernel test robot noticed the following build errors:

[auto build test ERROR on tip/timers/core]
[also build test ERROR on linus/master v6.10-rc1 next-20240529]
[cannot apply to tip/timers/nohz]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Oleg-Nesterov/tick-shift-tick_nohz_switch_to_nohz-from-tick_check_oneshot_change-to-hrtimer_run_queues/20240530-204940
base:   tip/timers/core
patch link:    https://lore.kernel.org/r/20240530124203.GA26990%40redhat.com
patch subject: [PATCH] tick: shift tick_nohz_switch_to_nohz() from tick_check_oneshot_change() to hrtimer_run_queues()
config: i386-randconfig-001-20240531 (https://download.01.org/0day-ci/archive/20240531/202405310818.JPmNHEjC-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240531/202405310818.JPmNHEjC-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405310818.JPmNHEjC-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: kernel/time/hrtimer.o: in function `hrtimer_run_queues':
>> kernel/time/hrtimer.c:1898:(.text+0x1e71): undefined reference to `tick_nohz_switch_to_nohz'


vim +1898 kernel/time/hrtimer.c

  1874	
  1875	/*
  1876	 * Called from run_local_timers in hardirq context every jiffy
  1877	 */
  1878	void hrtimer_run_queues(void)
  1879	{
  1880		struct hrtimer_cpu_base *cpu_base = this_cpu_ptr(&hrtimer_bases);
  1881		unsigned long flags;
  1882		ktime_t now;
  1883	
  1884		if (hrtimer_hres_active(cpu_base))
  1885			return;
  1886	
  1887		/*
  1888		 * This _is_ ugly: We have to check periodically, whether we
  1889		 * can switch to highres and / or nohz mode. The clocksource
  1890		 * switch happens with xtime_lock held. Notification from
  1891		 * there only sets the check bit in the tick_oneshot code,
  1892		 * otherwise we might deadlock vs. xtime_lock.
  1893		 */
  1894		if (tick_check_oneshot_change()) {
  1895			if (hrtimer_is_hres_enabled())
  1896				hrtimer_switch_to_hres();
  1897			else
> 1898				tick_nohz_switch_to_nohz();
  1899			return;
  1900		}
  1901	
  1902		raw_spin_lock_irqsave(&cpu_base->lock, flags);
  1903		now = hrtimer_update_base(cpu_base);
  1904	
  1905		if (!ktime_before(now, cpu_base->softirq_expires_next)) {
  1906			cpu_base->softirq_expires_next = KTIME_MAX;
  1907			cpu_base->softirq_activated = 1;
  1908			raise_softirq_irqoff(HRTIMER_SOFTIRQ);
  1909		}
  1910	
  1911		__hrtimer_run_queues(cpu_base, now, flags, HRTIMER_ACTIVE_HARD);
  1912		raw_spin_unlock_irqrestore(&cpu_base->lock, flags);
  1913	}
  1914	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] tick: shift tick_nohz_switch_to_nohz() from tick_check_oneshot_change() to hrtimer_run_queues()

Posted by Frederic Weisbecker 1 year, 6 months ago

Le Thu, May 30, 2024 at 02:42:03PM +0200, Oleg Nesterov a écrit :
> 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>
> ---
>  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();

Thanks a lot for clarifying this! I've always found that confusing
(and note how I never did anything about it!)

Just a detail below:

>  		return;
>  	}
>  
> diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h
> index 5f2105e637bd..6764fbd18afd 100644
> --- a/kernel/time/tick-internal.h
> +++ b/kernel/time/tick-internal.h
> @@ -111,7 +111,8 @@ 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 void tick_nohz_switch_to_nohz(void);

tick_nohz_switch_to_nohz() is only built with CONFIG_NO_HZ_COMMON

You will have a build issue with CONFIG_HIGH_RES_TIMER && !CONFIG_NO_HZ_COMMON

>  extern int tick_init_highres(void);
>  #else /* !CONFIG_TICK_ONESHOT: */
>  static inline
> @@ -124,7 +125,8 @@ 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; }
> +static inline void tick_nohz_switch_to_nohz(void) { }
>  #endif /* !CONFIG_TICK_ONESHOT */

Re: [PATCH] tick: shift tick_nohz_switch_to_nohz() from tick_check_oneshot_change() to hrtimer_run_queues()

Posted by Oleg Nesterov 1 year, 6 months ago

On 05/30, Frederic Weisbecker wrote:
>
> tick_nohz_switch_to_nohz() is only built with CONFIG_NO_HZ_COMMON
>
> You will have a build issue with CONFIG_HIGH_RES_TIMER && !CONFIG_NO_HZ_COMMON

Thanks again.

At first glance this patch needs a simple fixup below. I'll recheck and
send V2.

NO_HZ_COMMON selects TICK_ONESHOT. But even if it didn't,
tick_check_oneshot_change() is dummy inline "return 0" without
CONFIG_TICK_ONESHOT, hrtimer_run_queues() will never try to
call tick_nohz_switch_to_nohz().

Oleg.


--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -112,7 +112,6 @@ 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(void);
-extern void tick_nohz_switch_to_nohz(void);
 extern int tick_init_highres(void);
 #else /* !CONFIG_TICK_ONESHOT: */
 static inline
@@ -126,7 +125,6 @@ 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(void) { return 0; }
-static inline void tick_nohz_switch_to_nohz(void) { }
 #endif /* !CONFIG_TICK_ONESHOT */
 
 /* Functions related to oneshot broadcasting */
@@ -159,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);
@@ -173,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

Re: [PATCH] tick: shift tick_nohz_switch_to_nohz() from tick_check_oneshot_change() to hrtimer_run_queues()

Posted by Oleg Nesterov 1 year, 6 months ago

On 05/30, Frederic Weisbecker wrote:
>
> > +extern int tick_check_oneshot_change(void);
> > +extern void tick_nohz_switch_to_nohz(void);
>
> tick_nohz_switch_to_nohz() is only built with CONFIG_NO_HZ_COMMON
>
> You will have a build issue with CONFIG_HIGH_RES_TIMER && !CONFIG_NO_HZ_COMMON

Hmm. I naively thought that a dummy definition below should be enough...

Thanks! I'll recheck and reply on Saturday.

Oleg.

> >  #else /* !CONFIG_TICK_ONESHOT: */
> >  static inline
> > @@ -124,7 +125,8 @@ 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; }
> > +static inline void tick_nohz_switch_to_nohz(void) { }
> >  #endif /* !CONFIG_TICK_ONESHOT */
>

[PATCH v2] tick: shift tick_nohz_switch_to_nohz() from tick_check_oneshot_change() to hrtimer_run_queues()

Posted by Oleg Nesterov 1 year, 6 months ago

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

Re: [PATCH v2] tick: shift tick_nohz_switch_to_nohz() from tick_check_oneshot_change() to hrtimer_run_queues()

Posted by Thomas Gleixner 1 year, 6 months ago

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

Re: [PATCH v2] tick: shift tick_nohz_switch_to_nohz() from tick_check_oneshot_change() to hrtimer_run_queues()

Posted by Oleg Nesterov 1 year, 6 months ago

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.

Re: [PATCH v2] tick: shift tick_nohz_switch_to_nohz() from tick_check_oneshot_change() to hrtimer_run_queues()

Posted by Oleg Nesterov 1 year, 6 months ago

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.