linux-kernel - Re: [PATCH v9 12/32] timers: Fix nextevt calculation when no timers are pending

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:   Mon, 4 Dec 2023 17:03:50 +0100
From:   Sebastian Siewior <bigeasy@...utronix.de>
To:     Anna-Maria Behnsen <anna-maria@...utronix.de>
Cc:     linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        John Stultz <jstultz@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Eric Dumazet <edumazet@...gle.com>,
        "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
        Arjan van de Ven <arjan@...radead.org>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Frederic Weisbecker <frederic@...nel.org>,
        Rik van Riel <riel@...riel.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Giovanni Gherdovich <ggherdovich@...e.cz>,
        Lukasz Luba <lukasz.luba@....com>,
        "Gautham R . Shenoy" <gautham.shenoy@....com>,
        Srinivas Pandruvada <srinivas.pandruvada@...el.com>,
        K Prateek Nayak <kprateek.nayak@....com>
Subject: Re: [PATCH v9 12/32] timers: Fix nextevt calculation when no timers
 are pending

On 2023-12-01 10:26:34 [+0100], Anna-Maria Behnsen wrote:
> When no timer is queued into an empty timer base, the next_expiry will not
> be updated. It was originally calculated as
> 
>   base->clk + NEXT_TIMER_MAX_DELTA
> 
> When the timer base stays empty long enough (> NEXT_TIMER_MAX_DELTA), the
> next_expiry value of the empty base suggests that there is a timer pending
> soon. This might be more a kind of a theoretical problem, but the fix
> doesn't hurt.

So __run_timers() sets base::next_expiry to base->clk +
NEXT_TIMER_MAX_DELTA and then we have no more timers enqueued.

But wouldn't base->timers_pending remain false? Therefore it would use
"expires = KTIME_MAX" as return value (well cmp_next_hrtimer_event())?

Based on the code as of #11, it would only set timer_base::is_idle
wrongly false if it wraps around. Other than that, I don't see an issue.
What do I miss?

If you update it regardless here then it would make a difference to
run_local_timers() assuming we have still hrtimer which expire and this
next_expiry check might raise a softirq since it does not consider the
timers_pending value.

> Use only base->next_expiry value as nextevt when timers are
> pending. Otherwise nextevt will be jiffies + NEXT_TIMER_MAX_DELTA. As all
> information is in place, update base->next_expiry value of the empty timer
> base as well.

or consider timers_pending in run_local_timers()? An additional read vs
write?

> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -1944,10 +1943,20 @@ u64 get_next_timer_interrupt(unsigned long basej, u64 basem)
>  	__forward_timer_base(base, basej);
>  
>  	if (base->timers_pending) {
> +		nextevt = base->next_expiry;
> +
>  		/* If we missed a tick already, force 0 delta */
>  		if (time_before(nextevt, basej))
>  			nextevt = basej;
>  		expires = basem + (u64)(nextevt - basej) * TICK_NSEC;
> +	} else {
> +		/*
> +		 * Move next_expiry for the empty base into the future to
> +		 * prevent a unnecessary raise of the timer softirq when the
                           an
> +		 * next_expiry value will be reached even if there is no timer
> +		 * pending.
> +		 */
> +		base->next_expiry = nextevt;
>  	}
>  
>  	/*

Sebastian