Re: [PATCH] timers: Clear must_forward_clk inside base lock

[Kohli, Gaurav]

Hi John, Thomas,

Can you please review below patch and update your comments:

Regards
Gaurav

On 7/26/2018 2:12 PM, Gaurav Kohli wrote:
> While migrating timer to new base, there is a need
> to update base clk by calling forward_timer_base to
> avoid stale clock , but at the same time if run_timer
> is exectuing in new core it may set must_forward_clk
> to false and due to this forward base logic may fail as
> per below check:
>
> if (likely(!base->must_forward_clk))
>                  return;
>
> So preventing the same by putting must_forward_clk inside
> base lock.
>
> Signed-off-by: Gaurav Kohli <gkohli@xxxxxxxxxxxxxx>
>
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index cc2d23e..675241d 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -1657,6 +1657,19 @@ static inline void __run_timers(struct timer_base *base)
>   
>   	raw_spin_lock_irq(&base->lock);
>   
> +	/*
> +	 * must_forward_clk must be cleared before running timers so that any
> +	 * timer functions that call mod_timer will not try to forward the
> +	 * base. idle trcking / clock forwarding logic is only used with
> +	 * BASE_STD timers.
> +	 *
> +	 * The deferrable base does not do idle tracking at all, so we do
> +	 * not forward it. This can result in very large variations in
> +	 * granularity for deferrable timers, but they can be deferred for
> +	 * long periods due to idle.
> +	 */
> +	base->must_forward_clk = false;
> +
>   	while (time_after_eq(jiffies, base->clk)) {
>   
>   		levels = collect_expired_timers(base, heads);
> @@ -1676,19 +1689,6 @@ static __latent_entropy void run_timer_softirq(struct softirq_action *h)
>   {
>   	struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]);
>   
> -	/*
> -	 * must_forward_clk must be cleared before running timers so that any
> -	 * timer functions that call mod_timer will not try to forward the
> -	 * base. idle trcking / clock forwarding logic is only used with
> -	 * BASE_STD timers.
> -	 *
> -	 * The deferrable base does not do idle tracking at all, so we do
> -	 * not forward it. This can result in very large variations in
> -	 * granularity for deferrable timers, but they can be deferred for
> -	 * long periods due to idle.
> -	 */
> -	base->must_forward_clk = false;
> -
>   	__run_timers(base);
>   	if (IS_ENABLED(CONFIG_NO_HZ_COMMON))
>   		__run_timers(this_cpu_ptr(&timer_bases[BASE_DEF]));

--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, 
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.