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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 26 Oct 2022 17:26:21 +0200
From:   Frederic Weisbecker <frederic@...nel.org>
To:     Anna-Maria Behnsen <anna-maria@...utronix.de>
Cc:     linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        John Stultz <john.stultz@...aro.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        linux-pm@...r.kernel.org, Arjan van de Ven <arjan@...radead.org>,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        Frederic Weisbecker <fweisbec@...il.com>,
        Rik van Riel <riel@...hat.com>,
        Richard Cochran <richardcochran@...il.com>
Subject: Re: [PATCH v3 06/17] timer: Keep the pinned timers separate from the
 others

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@...nel.org>

Thanks!


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