linux-kernel - Re: [PATCH v9 30/32] timers: Implement the hierarchical pull model

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]

Message-ID: <878r652hko.fsf@somnus>
Date:   Fri, 08 Dec 2023 10:01:59 +0100
From:   Anna-Maria Behnsen <anna-maria@...utronix.de>
To:     Sebastian Siewior <bigeasy@...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 30/32] timers: Implement the hierarchical pull model

Sebastian Siewior <bigeasy@...utronix.de> writes:

> On 2023-12-01 10:26:52 [+0100], Anna-Maria Behnsen wrote:

[...]

>> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
>> index b6c9ac0c3712..ac3e888d053f 100644
>> --- a/kernel/time/timer.c
>> +++ b/kernel/time/timer.c
>> @@ -2103,6 +2104,64 @@ void timer_lock_remote_bases(unsigned int cpu)
> …
>> +static void timer_use_tmigr(unsigned long basej, u64 basem,
>> +			    unsigned long *nextevt, bool *tick_stop_path,
>> +			    bool timer_base_idle, struct timer_events *tevt)
>> +{
>> +	u64 next_tmigr;
>> +
>> +	if (timer_base_idle)
>> +		next_tmigr = tmigr_cpu_new_timer(tevt->global);
>> +	else if (tick_stop_path)
>> +		next_tmigr = tmigr_cpu_deactivate(tevt->global);
>> +	else
>> +		next_tmigr = tmigr_quick_check();
>> +
>> +	/*
>> +	 * If the CPU is the last going idle in timer migration hierarchy, make
>> +	 * sure the CPU will wake up in time to handle remote timers.
>> +	 * next_tmigr == KTIME_MAX if other CPUs are still active.
>> +	 */
>> +	if (next_tmigr < tevt->local) {
>> +		u64 tmp;
>> +
>> +		/* If we missed a tick already, force 0 delta */
>> +		if (next_tmigr < basem)
>> +			next_tmigr = basem;
>> +
>> +		tmp = div_u64(next_tmigr - basem, TICK_NSEC);
>
> Is this considered a hot path? Asking because u64 divs are nice if can
> be avoided ;)

It's the 'try to go idle path' - so no hot path. Please correct me if
I'm wrong.

> I guess the original value is from fetch_next_timer_interrupt(). But
> then you only need it if the caller (__get_next_timer_interrupt()) has
> the `idle' value set. Otherwise the operation is pointless.
> Would it somehow work to replace
> 	base_local->is_idle = time_after(nextevt, basej + 1);
>
> with maybe something like
> 	base_local->is_idle = tevt.local > basem + TICK_NSEC
>
> If so you could avoid the `nextevt' maneuver.
>

This change could be done indepentant as an improvement on top of the
queue as well. I will not improve it right now, if it's ok.

>> +		*nextevt = basej + (unsigned long)tmp;
>> +		tevt->local = next_tmigr;
>> +	}
>> +}
>> +# else

Thanks for the other input - I already changed it for v10 of the queue.

Thanks,

	Anna-Maria