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

[Johannes Weiner]

On Thu, Jul 08, 2021 at 08:54:56AM -0700, Suren Baghdasaryan wrote:
> 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.

Oh I see.

Right, flipping the condition doesn't make sense because we need
poll_scheduled to be set when we go ahead - whether we're forcing or
not. I.e. if we were in a locked section, we'd write it like this:

	if (poll_scheduled)
		if (!force)
			return;
	else
		poll_scheduled = 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).
>   */

Personally, I still find this more confusing than no comment on
!force, because when you read it it sort of raises the question what
the alternatives would be. And the alternatives appear to be
nonsensical code rather than legitimate options.

But I won't insist if you prefer to leave it in. Your call.

> > /*
> >  * 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?

Yeah, sounds good to me!