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

[Peng Liu]

On 2023/11/29 0:56, Frederic Weisbecker wrote:
> 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!
Thanks for comments. I will update my patches, and also try removing
(CONFIG_NO_HZ_COMMON || CONFIG_HIGH_RES_TIMERS) in another patch.

Regards,
Peng