Re: [PATCH v10 18/20] timers: Implement the hierarchical pull model

[Anna-Maria Behnsen]

Frederic Weisbecker <frederic@xxxxxxxxxx> writes:

> Le Mon, Jan 29, 2024 at 11:50:39AM +0100, Anna-Maria Behnsen a écrit :
>> Frederic Weisbecker <frederic@xxxxxxxxxx> 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