Re: [PATCH v3 06/17] timer: Keep the pinned timers separate from the others

[Frederic Weisbecker]

On Tue, Oct 25, 2022 at 03:58:39PM +0200, Anna-Maria Behnsen wrote:
> @@ -1711,38 +1724,69 @@ u64 get_next_timer_interrupt(unsigned long basej, u64 basem)
>  	if (cpu_is_offline(smp_processor_id()))
>  		return expires;
>  
> -	raw_spin_lock(&base->lock);
> +	base_local = this_cpu_ptr(&timer_bases[BASE_LOCAL]);
> +	base_global = this_cpu_ptr(&timer_bases[BASE_GLOBAL]);
>  
> -	nextevt = next_timer_interrupt(base);
> +	raw_spin_lock(&base_local->lock);
> +	raw_spin_lock_nested(&base_global->lock, SINGLE_DEPTH_NESTING);
> +
> +	nextevt_local = next_timer_interrupt(base_local);
> +	nextevt_global = next_timer_interrupt(base_global);
>  
>  	/*
>  	 * We have a fresh next event. Check whether we can forward the
>  	 * base. We can only do that when @basej is past base->clk
>  	 * otherwise we might rewind base->clk.
>  	 */
> -	if (time_after(basej, base->clk)) {
> -		if (time_after(nextevt, basej))
> -			base->clk = basej;
> -		else if (time_after(nextevt, base->clk))
> -			base->clk = nextevt;
> +	if (time_after(basej, base_local->clk)) {
> +		if (time_after(nextevt_local, basej))
> +			base_local->clk = basej;
> +		else if (time_after(nextevt_local, base_local->clk))
> +			base_local->clk = nextevt_local;
> +	}
> +
> +	if (time_after(basej, base_global->clk)) {
> +		if (time_after(nextevt_global, basej))
> +			base_global->clk = basej;
> +		else if (time_after(nextevt_global, base_global->clk))
> +			base_global->clk = nextevt_global;

Perhaps make a helper for the two above?

>  	}
>  
> @@ -1763,6 +1807,9 @@ void timer_clear_idle(void)
>  	 * the lock in the exit from idle path.
>  	 */
>  	base->is_idle = false;
> +
> +	base = this_cpu_ptr(&timer_bases[BASE_GLOBAL]);
> +	base->is_idle = false;

May be just:

    __this_cpu_write(timer_bases[BASE_LOCAL].is_idle, false)
    __this_cpu_write(timer_bases[BASE_GLOBAL].is_idle, false)

>  }
>  #endif
>  
> @@ -1820,17 +1869,21 @@ static __latent_entropy void run_timer_softirq(struct softirq_action *h)
>   */
>  static void run_local_timers(void)
>  {
> -	struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]);
> +	struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_LOCAL]);
>  
>  	hrtimer_run_queues();
>  	/* Raise the softirq only if required. */
>  	if (time_before(jiffies, base->next_expiry)) {
>  		if (!IS_ENABLED(CONFIG_NO_HZ_COMMON))
>  			return;
> -		/* CPU is awake, so check the deferrable base. */
> +		/* CPU is awake, so check for the global base. */
>  		base++;
> -		if (time_before(jiffies, base->next_expiry))
> -			return;
> +		if (time_before(jiffies, base->next_expiry)) {
> +			/* CPU is awake, so check the deferrable base. */
> +			base++;
> +			if (time_before(jiffies, base->next_expiry))
> +				return;
> +		}

Could be:

for (i = 0; i < NR_BASES; i++) {
    struct timer_base *base = this_cpu_ptr(&timer_bases[i]);
    if (time_after_eq(jiffies, base->next_expiry)) {
       raise_softirq(TIMER_SOFTIRQ);
       return;
    }
}
       

Reviewed-by: Frederic Weisbecker <frederic@xxxxxxxxxx>

Thanks!


>  	}
>  	raise_softirq(TIMER_SOFTIRQ);
>  }
> -- 
> 2.30.2
>