Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Date:   Tue, 28 Nov 2023 17:56:11 +0100
From:   Frederic Weisbecker <frederic@kernel.org>
To:     Peng Liu <pngliu@hotmail.com>
Cc:     tglx@linutronix.de, mingo@kernel.org, linux-kernel@vger.kernel.org,
        liupeng17@lenovo.com
Subject: Re: [PATCH v2 1/2] tick/nohz: Remove duplicate between
 tick_nohz_switch_to_nohz() and tick_setup_sched_timer()
Message-ID: <ZWYbqwZrP+WfvT/N@lothringen>
References: <20231127083049.145447-1-pngliu@hotmail.com>
 <TYCP286MB21464DF5148B8FF8B142ADF7C6BCA@TYCP286MB2146.JPNP286.PROD.OUTLOOK.COM>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <TYCP286MB21464DF5148B8FF8B142ADF7C6BCA@TYCP286MB2146.JPNP286.PROD.OUTLOOK.COM>
Precedence: bulk

On Tue, Nov 28, 2023 at 05:23:54PM +0800, Peng Liu wrote:
> @@ -1529,7 +1519,9 @@ static enum hrtimer_restart tick_nohz_highres_handler(struct hrtimer *timer)
>  
>  	return HRTIMER_RESTART;
>  }
> +#endif /* HIGH_RES_TIMERS */
>  
> +#if defined CONFIG_NO_HZ_COMMON || defined CONFIG_HIGH_RES_TIMERS

tick-sched.c is only ever built if CONFIG_TICK_ONESHOT=y and
CONFIG_TICK_ONESHOT is basically (CONFIG_NO_HZ_COMMON || CONFIG_HIGH_RES_TIMERS)

So probably the above is not needed and if you like you can even send
a subsequent patch removing such ifdefs within this file :-)

>  static int sched_skew_tick;
>  
>  static int __init skew_tick(char *str)
> @@ -1542,15 +1534,19 @@ early_param("skew_tick", skew_tick);
>  
>  /**
>   * tick_setup_sched_timer - setup the tick emulation timer
> + * @mode: tick_nohz_mode to setup for
>   */
> -void tick_setup_sched_timer(void)
> +void tick_setup_sched_timer(int mode)
>  {
>  	struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched);
>  	ktime_t now = ktime_get();
>  
>  	/* Emulate tick processing via per-CPU hrtimers: */
>  	hrtimer_init(&ts->sched_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_HARD);
> -	ts->sched_timer.function = tick_nohz_highres_handler;
> +#ifdef CONFIG_HIGH_RES_TIMERS
> +	if (mode == NOHZ_MODE_HIGHRES)
> +		ts->sched_timer.function = tick_nohz_highres_handler;
> +#endif

That ifdef could simply be removed.

>  
>  	/* Get the next period (per-CPU) */
>  	hrtimer_set_expires(&ts->sched_timer, tick_init_jiffy_update());
> @@ -1564,12 +1560,15 @@ void tick_setup_sched_timer(void)
>  	}

That invisible part above is the skew_tick thing, which can probably work
on low-res mode so why not but please tell about that in the changelog.

>  
>  	hrtimer_forward(&ts->sched_timer, now, TICK_NSEC);

Looks like this can be hrtimer_forward_now() and you can remove the now.

> -	hrtimer_start_expires(&ts->sched_timer, HRTIMER_MODE_ABS_PINNED_HARD);
> -	tick_nohz_activate(ts, NOHZ_MODE_HIGHRES);
> +#ifdef CONFIG_HIGH_RES_TIMERS
> +	if (mode == NOHZ_MODE_HIGHRES)
> +		hrtimer_start_expires(&ts->sched_timer, HRTIMER_MODE_ABS_PINNED_HARD);
> +	else
> +#endif

And probably that ifdef could simply be turned to
IS_ENABLED(CONFIG_HIGH_RES_TIMERS).

Thanks!