linux-kernel - Re: [PATCH v10 18/20] 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: <87cytj3q24.fsf@somnus>
Date: Tue, 30 Jan 2024 14:32:03 +0100
From: Anna-Maria Behnsen <anna-maria@...utronix.de>
To: Frederic Weisbecker <frederic@...nel.org>
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>, Rik van Riel <riel@...riel.com>,
 Steven Rostedt <rostedt@...dmis.org>, Sebastian Siewior
 <bigeasy@...utronix.de>, 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>, Boqun Feng <boqun.feng@...il.com>, Mark Rutland
 <mark.rutland@....com>
Subject: Re: [PATCH v10 18/20] timers: Implement the hierarchical pull model

Frederic Weisbecker <frederic@...nel.org> writes:

> Le Mon, Jan 29, 2024 at 11:50:39AM +0100, Anna-Maria Behnsen a écrit :
>> Frederic Weisbecker <frederic@...nel.org> writes:
>> 
>> > Le Mon, Jan 15, 2024 at 03:37:41PM +0100, Anna-Maria Behnsen a écrit :
>> >> +static bool tmigr_inactive_up(struct tmigr_group *group,
>> >> +			      struct tmigr_group *child,
>> >> +			      void *ptr)
>> >> +{
>> >> +	union tmigr_state curstate, newstate, childstate;
>> >> +	struct tmigr_walk *data = ptr;
>> >> +	bool walk_done;
>> >> +	u8 childmask;
>> >> +
>> >> +	childmask = data->childmask;
>> >> +	curstate.state = atomic_read(&group->migr_state);
>> >> +	childstate.state = 0;
>> >> +
>> >> +	do {
>> >
>> > So I got the confirmation from Boqun (+Cc) and Paul that a failing cmpxchg
>> > may not order the load of the old value against subsequent loads. And
>> > that may apply to atomic_try_cmpxchg() as well.
>> >
>> > Therefore you not only need to turn group->migr_state read into
>> > an atomic_read_acquire() but you also need to do this on each iteration
>> > of this loop. For example you can move the read_acquire right here.
>> 
>> I tried to read and understand more about the memory barriers especially
>> the acquire/release stuff. So please correct me whenever I'm wrong.
>> 
>> We have to make sure that the child/group state values contain the last
>> updates and prevent reordering to be able to rely on those values.
>> 
>> So I understand, that we need the atomic_read_acquire() here for the
>> child state, because we change the group state accordingly and need to
>> make sure, that it contains the last update of it. The cmpxchg which
>> writes the child state is (on success) a full memory barrier. And the
>> atomic_read_acquire() makes sure all preceding "critical sections"
>> (which ends with the full memory barrier) are visible. Is this right?
>
> Right. And BTW I'm being suggested by Paul to actually avoid
> atomic_read_acquire() after cmpxchg() failure because that implies an
> error prone re-read. So pick up your favourite between smp_rmb() or
> smp_mb__after_atomic().
>
> With the latter this could look like:
>
> curstate.state = atomic_read_acquire(&group->migr_state);
> for (;;) {
>     childstate.state = atomic_read(&child->migr_state);
>     ...
>     if (atomic_try_cmpxchg(&group->migr_state, &curstate.state, newstate.state))
>        break;
>     smp_mb__after_atomic();
> }

I'll take this.

>> 
>> To make sure the proper states are used, atomic_read_acquire() is then
>> also required in:
>>   - tmigr_check_migrator()
>>   - tmigr_check_migrator_and_lonely()
>>   - tmigr_check_lonely()
>
> Not sure about those. I'll check them.

Please ignore - I was on the wrong track and John helped me out (at
least another step) of my "memory barrier understanding mess". So I have
no more questions regarding the memory barriers here - at least at the
moment.

>>   - tmigr_new_timer_up() (for childstate and groupstate)
>
> Actually you need to fix some ordering there that I suggested a while ago :)
> See https://lore.kernel.org/all/ZIhKT3h7Dc0G3xoU@lothringen/

I quickly opened it and it seems that I completely missed it
somehow. I'm sorry!

Thanks for your patience!

	Anna-Maria