Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9347e3d4-e774-f75f-22c4-6c2dba294423@joelfernandes.org>
Date:   Fri, 14 Jul 2023 14:44:49 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH 3/3] tick/nohz: Don't shutdown the lowres tick from itself
Content-Language: en-US
To:     Frederic Weisbecker <frederic@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>
Cc:     LKML <linux-kernel@vger.kernel.org>
References: <20230714120852.23573-1-frederic@kernel.org>
 <20230714120852.23573-4-frederic@kernel.org>
From:   Joel Fernandes <joel@joelfernandes.org>
In-Reply-To: <20230714120852.23573-4-frederic@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/14/23 08:08, Frederic Weisbecker wrote:
> In lowres dynticks mode, just like in highres dynticks mode, when there
> is no tick to program in the future, the tick eventually gets
> deactivated either:
> 
> * From the idle loop if in idle mode.
> * From the IRQ exit if in full dynticks mode.
> 
> Therefore there is no need to deactivate it from the tick itself. This
> just just brings more overhead in the idle tick path for no reason.
> 
> Fixes: 62c1256d5447 ("timers/nohz: Switch to ONESHOT_STOPPED in the low-res handler when the tick is stopped")
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> ---
>   kernel/time/tick-sched.c | 24 +++++++++++++-----------
>   1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index dd25da8531f4..101251e103be 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -1392,18 +1392,16 @@ static void tick_lowres_handler(struct clock_event_device *dev)
>   	tick_sched_do_timer(ts, now);
>   	tick_sched_handle(ts, regs);
>   
> -	if (unlikely(ts->tick_stopped)) {
> -		/*
> -		 * The clockevent device is not reprogrammed, so change the
> -		 * clock event device to ONESHOT_STOPPED to avoid spurious
> -		 * interrupts on devices which might not be truly one shot.
> -		 */
> -		tick_program_event(KTIME_MAX, 1);
> -		return;
> +	/*
> +	 * In dynticks mode, tick reprogram is deferred:
> +	 * - to the idle task if in dynticks-idle
> +	 * - to IRQ exit if in full-dynticks.
> +	 */
> +	if (likely(!ts->tick_stopped)) {
> +		hrtimer_forward(&ts->sched_timer, now, TICK_NSEC);
> +		tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1);
>   	}
>   
> -	hrtimer_forward(&ts->sched_timer, now, TICK_NSEC);
> -	tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1);

One slight concern here though, where in the idle loop is the removed statement 
"tick_program_event(KTIME_MAX, 1);" happening if the tick was already stopped 
before? If it is happening in tick_nohz_stop_tick(), don't we early return from 
there and avoid doing that "tick_program_event(KTIME_MAX, 1);" altogether, if 
the tick was already stopped and the next event has not changed?

         /* Skip reprogram of event if its not changed */
         if (ts->tick_stopped && (expires == ts->next_tick)) {
                 /* Sanity check: make sure clockevent is actually programmed */
                 if (tick == KTIME_MAX || ts->next_tick ==  [...]
                         return;
		[...]
	}

Also just a nit, here you can remove indent by doing:

if (unlikely(ts->tick_stopped))
     return;
hrtimer_forward(&ts->sched_timer, now, TICK_NSEC);
tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1);

Which is pretty much the original code except for the tick_program_event().

thanks,

  - Joel


>   }
>   
>   static inline void tick_nohz_activate(struct tick_sched *ts, int mode)
> @@ -1508,7 +1506,11 @@ static enum hrtimer_restart tick_highres_handler(struct hrtimer *timer)
>   	else
>   		ts->next_tick = 0;
>   
> -	/* No need to reprogram if we are in idle or full dynticks mode */
> +	/*
> +	 * In dynticks mode, tick reprogram is deferred:
> +	 * - to the idle task if in dynticks-idle
> +	 * - to IRQ exit if in full-dynticks.
> +	 */
>   	if (unlikely(ts->tick_stopped))
>   		return HRTIMER_NORESTART;
>