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

[Suren Baghdasaryan]

On Sat, Sep 17, 2022 at 12:31 AM Hao Lee <haolee.swjtu@xxxxxxxxx> wrote:
>
> 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.

The `else` condition in your code does nothing, and that's why the
original code does not implement a handler for that case.

> 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.

The new code you posted is functionally the same as the original one
while being more verbose and IMO less readable. Unless you can explain
the problem with the original code, I don't see any reason to change
it.

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