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

[Peng Liu <pngliu@hotmail.com>]

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