Re: [PATCH v3 1/1] psi: stop relying on timer_pending for poll_work rescheduling

From: Suren Baghdasaryan
Date: Thu Jul 08 2021 - 11:55:13 EST


t

On Thu, Jul 8, 2021 at 7:44 AM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
>
> On Wed, Jul 07, 2021 at 03:43:48PM -0700, Suren Baghdasaryan wrote:
> > On Wed, Jul 7, 2021 at 6:39 AM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
> > >
> > > This looks good to me now code wise. Just a comment on the comments:
> > >
> > > On Tue, Jul 06, 2021 at 07:39:33PM -0700, Suren Baghdasaryan wrote:
> > > > @@ -559,18 +560,14 @@ static u64 update_triggers(struct psi_group *group, u64 now)
> > > > return now + group->poll_min_period;
> > > > }
> > > >
> > > > -/* Schedule polling if it's not already scheduled. */
> > > > -static void psi_schedule_poll_work(struct psi_group *group, unsigned long delay)
> > > > +/* Schedule polling if it's not already scheduled or forced. */
> > > > +static void psi_schedule_poll_work(struct psi_group *group, unsigned long delay,
> > > > + bool force)
> > > > {
> > > > struct task_struct *task;
> > > >
> > > > - /*
> > > > - * Do not reschedule if already scheduled.
> > > > - * Possible race with a timer scheduled after this check but before
> > > > - * mod_timer below can be tolerated because group->polling_next_update
> > > > - * will keep updates on schedule.
> > > > - */
> > > > - if (timer_pending(&group->poll_timer))
> > > > + /* xchg should be called even when !force to set poll_scheduled */
> > > > + if (atomic_xchg(&group->poll_scheduled, 1) && !force)
> > > > return;
> > >
> > > This explains what the code does, but not why. It would be good to
> > > explain the ordering with poll_work, here or there. But both sides
> > > should mention each other.
> >
> > How about this:
> >
> > /*
> > * atomic_xchg should be called even when !force to always set poll_scheduled
> > * and to provide a memory barrier (see the comment inside psi_poll_work).
> > */
>
> The memory barrier part makes sense, but the first part says what the
> code does and the message is unclear to me. Are you worried somebody
> might turn this around in the future and only conditionalize on
> poll_scheduled when !force? Essentially, I don't see the downside of
> dropping that. But maybe I'm missing something.

Actually you are right. Originally I was worried that there might be a
case when poll_scheduled==0 and force==true and if someone flips the
conditions we will reschedule the timer but will not set
poll_scheduled back to 1. However I don't think this condition is
possible. We set force=true only when we skipped resetting
poll_schedule to 0 and on initial wakeup we always reset
poll_schedule. How about changing the comment to this:

/*
* atomic_xchg should be called even when !force to provide a
* full memory barrier (see the comment inside psi_poll_work).
*/

> /*
> * The xchg implies a full barrier that matches the one
> * in psi_poll_work() (see corresponding comment there).
> */
>
> > > > @@ -595,6 +595,28 @@ static void psi_poll_work(struct psi_group *group)
> > > >
> > > > now = sched_clock();
> > > >
> > > > + if (now > group->polling_until) {
> > > > + /*
> > > > + * We are either about to start or might stop polling if no
> > > > + * state change was recorded. Resetting poll_scheduled leaves
> > > > + * a small window for psi_group_change to sneak in and schedule
> > > > + * an immegiate poll_work before we get to rescheduling. One
> > > > + * potential extra wakeup at the end of the polling window
> > > > + * should be negligible and polling_next_update still keeps
> > > > + * updates correctly on schedule.
> > > > + */
> > > > + atomic_set(&group->poll_scheduled, 0);
> > > > + /*
> > > > + * Ensure that operations of clearing group->poll_scheduled and
> > > > + * obtaining changed_states are not reordered.
> > > > + */
> > > > + smp_mb();
> > >
> > > Same here, it would be good to explain that this is ordering the
> > > scheduler with the timer such that no events are missed. Feel free to
> > > reuse my race diagram from the other thread - those are better at
> > > conveying the situation than freeform text.
> >
> > I tried to make your diagram a bit less abstract by using the actual
> > names. How about this?
> >
> > /*
> > * We need to enforce ordering between poll_scheduled and psi_group_cpu.times
> > * reads and writes in psi_poll_work and psi_group_change functions.
> > Otherwise we
> > * might fail to reschedule the timer when monitored states change:
> > *
> > * psi_poll_work:
> > * poll_scheduled = 0
> > * smp_mb()
> > * changed_states = collect_percpu_times()
> > * if changed_states && xchg(poll_scheduled, 1) == 0
> > * mod_timer()
>
> Those last two lines aren't relevant for the race, right? I'd leave
> those out to not distract from it.

They did help me illustrate the two failure cases but yeah, someone
who can read the code can derive the rest :)

>
> > * psi_group_change:
> > * record_times()
> > * smp_mb()
> > * if xchg(poll_scheduled, 1) == 0
> > * mod_timer()
>
> The reason I tend to keep these more abstract is because 1) the names
> of the functions change (I had already sent out patches to rename half
> the variable and function names in this diagram), while the
> architecture (task change vs poll worker) likely won't, and 2) because
> it's easy to drown out what the reads, writes, and thus the race
> condition is with code details and function call indirections.

Got it.

>
> How about a compromise?
>
> /*
> * A task change can race with the poll worker that is supposed to
> * report on it. To avoid missing events, ensure ordering between
> * poll_scheduled and the task state accesses, such that if the poll
> * worker misses the state update, the task change is guaranteed to
> * reschedule the poll worker:
> *
> * poll worker:
> * atomic_set(poll_scheduled, 0)
> * smp_mb()
> * LOAD states
> *
> * task change:
> * STORE states
> * if atomic_xchg(poll_scheduled, 1) == 0:
> * schedule poll worker
> *
> * The atomic_xchg() implies a full barrier.
> */
> smp_mb();
>
> This gives a high-level view of what's happening but it can still be
> mapped to the code by following the poll_scheduled variable.

This looks really good to me.
If you agree on the first comment modification, should I respin the
next version?

>
> > If we remove smp_mb barriers then there are the following possible
> > reordering cases:
> >
> > Case1: reordering in psi_poll_work
> > psi_poll_work psi_group_change
> > changed_states = collect_percpu_times()
> > record_times()
> > if xchg(poll_scheduled,
> > 1) == 0 <-- false
> > mod_timer()
> > poll_scheduled = 0
> > if changed_states && xchg(poll_scheduled, 1) == 0 <-- changed_states is false
> > mod_timer()
> >
> > Case2: reordering in psi_group_change
> > psi_poll_work psi_group_change
> > if xchg(poll_scheduled,
> > 1) == 0 <-- false
> > mod_timer()
> > poll_scheduled = 0
> > changed_states = collect_percpu_times()
> > record_times()
> > if changed_states && xchg(poll_scheduled, 1) == 0 <-- changed_states is false
> > mod_timer()
> >
> > In both cases mod_timer() is not called, poll update is missed. But
> > describing this all in the comments would be an overkill IMHO.
> > WDYT?
>
> Yeah, I also think that's overkill. The failure cases can be derived
> from the concurrency diagram and explanation.
>
> Thanks