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

Anna-Maria Behnsen
2024-01-30T13:32:06Z

Frederic Weisbecker <frederic@kernel.org> writes:

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