Re: [PATCH] psi: fix possible missing or delayed pending event

[Hao Lee]

On Fri, Sep 16, 2022 at 11:08:34PM -0700, Suren Baghdasaryan wrote:
> On Wed, Sep 14, 2022 at 2:30 AM Hao Lee <haolee.swjtu@xxxxxxxxx> wrote:
> >
> > When a pending event exists and growth is less than the threshold, the
> > current logic is to skip this trigger without generating event. However,
> > from e6df4ead85d9 ("psi: fix possible trigger missing in the window"),
> > our purpose is to generate event as long as pending event exists and the
> > rate meets the limit. This patch fixes the possible pending-event
> > missing or delay.
> >
> > Fixes: e6df4ead85d9 ("psi: fix possible trigger missing in the window")
> > Signed-off-by: Hao Lee <haolee.swjtu@xxxxxxxxx>
> > ---
> >  kernel/sched/psi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> > index 9711827e3..0bae4ee2b 100644
> > --- a/kernel/sched/psi.c
> > +++ b/kernel/sched/psi.c
> > @@ -539,7 +539,7 @@ static u64 update_triggers(struct psi_group *group, u64 now)
> >
> >                         /* Calculate growth since last update */
> >                         growth = window_update(&t->win, now, total[t->state]);
> > -                       if (growth < t->threshold)
> > +                       if (growth < t->threshold && !t->pending_event)
> 
> I'm not sure how this additional condition changes things. Current
> logic is to set t->pending_event=true whenever growth exceeds the
> t->threshold. This patch will change this logic into setting
> t->pending_event=true also when t->pending_event=true.

This is right.

> But why would
> you want to set t->pending_event=true if it's already true? What am I
> missing?

If I expand this if-else branch and the pending_event statement
to a more detailed snippet, it will be like this:

if (growth < t->threshold && !t->pending_event) // under threshold && no pending event. Skip.
	continue;
else if (growth >= t->threshold) // above threshold. Try to generate event.
	t->pending_event = true;
else // under threshold && have pending events. Try to generate event.
	; // pending_event is already true. do nothing


The original code didn't handle the `else` condition properly. It will
skip the trigger when its growth is under the threshold, even though it
has a pending event. This patch handles this condition correctly.

But I think assigning true to pending_event when it's already true doesn't
have other side effects, so I eliminate the `else if` branch. Maybe we'd
better make it explicit, like the above snippet? Thanks.

> 
> >                                 continue;
> >
> >                         t->pending_event = true;
> > --
> > 2.21.0
> >