Received-SPF: pass (google.com: domain of linux-kernel+bounces-195595-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 30 May 2024 17:24:56 +0200
From: Frederic Weisbecker <frederic@kernel.org>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Anna-Maria Behnsen <anna-maria@linutronix.de>,
	Ingo Molnar <mingo@kernel.org>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] tick: shift tick_nohz_switch_to_nohz() from
 tick_check_oneshot_change() to hrtimer_run_queues()
Message-ID: <ZliaSISeFxx_FQ6O@localhost.localdomain>
References: <20240530124203.GA26990@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240530124203.GA26990@redhat.com>

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