Re: [PATCH v2 1/2] tick/nohz: Remove duplicate between tick_nohz_switch_to_nohz() and tick_setup_sched_timer()

[Frederic Weisbecker]

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!