2024-05-30 12:43:54

by Oleg Nesterov

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

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 <[email protected]>
---
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




2024-05-30 15:41:32

by Frederic Weisbecker

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

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 <[email protected]>
> ---
> 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 */

2024-05-30 17:13:34

by Oleg Nesterov

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

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 */
>


2024-05-31 00:28:34

by kernel test robot

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

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/[email protected]/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/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

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

2024-05-31 01:22:44

by kernel test robot

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

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/[email protected]/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/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

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

2024-06-01 14:58:42

by Oleg Nesterov

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

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


2024-06-02 10:21:51

by Oleg Nesterov

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

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 <[email protected]>
---
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



2024-06-03 08:14:59

by Thomas Gleixner

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

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

2024-06-03 13:42:46

by Oleg Nesterov

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

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.


2024-06-03 15:05:00

by Oleg Nesterov

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

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.