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

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

[Hao Lee]

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@gmail.com>
---
 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)
 				continue;
 
 			t->pending_event = true;
-- 
2.21.0

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

[Suren Baghdasaryan]

On Wed, Sep 14, 2022 at 2:30 AM Hao Lee <haolee.swjtu@gmail.com> 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@gmail.com>
> ---
>  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. But why would
you want to set t->pending_event=true if it's already true? What am I
missing?

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

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@gmail.com> 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@gmail.com>
> > ---
> >  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
> >

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@gmail.com> 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@gmail.com> 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@gmail.com>
> > > ---
> > >  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
> > >

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

[Hao Lee]

On Sat, Sep 17, 2022 at 09:44:12PM -0700, Suren Baghdasaryan wrote:
> On Sat, Sep 17, 2022 at 12:31 AM Hao Lee <haolee.swjtu@gmail.com> 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@gmail.com> 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@gmail.com>
> > > > ---
> > > >  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.

Hi, for the original code, let's assume t->pending_event is true:
    * if new_stall is false, we will try to check event ratelimit and
      generate an event for this psi_trigger. This case is right.
    * but if new_stall is true, we will skip this psi_trigger if growth
      growth < t->threshold. I think we shouldn't skip this psi_trigger
      in this case because it has a pending event.

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

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

[Suren Baghdasaryan]

On Sun, Sep 18, 2022 at 3:55 AM Hao Lee <haolee.swjtu@gmail.com> wrote:
>
> On Sat, Sep 17, 2022 at 09:44:12PM -0700, Suren Baghdasaryan wrote:
> > On Sat, Sep 17, 2022 at 12:31 AM Hao Lee <haolee.swjtu@gmail.com> 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@gmail.com> 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@gmail.com>
> > > > > ---
> > > > >  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.
>
> Hi, for the original code, let's assume t->pending_event is true:
>     * if new_stall is false, we will try to check event ratelimit and
>       generate an event for this psi_trigger. This case is right.
>     * but if new_stall is true, we will skip this psi_trigger if growth
>       growth < t->threshold. I think we shouldn't skip this psi_trigger
>       in this case because it has a pending event.

Ok, I see the issue now. I think the following fix would be the simplest:

                       /* Calculate growth since last update */
                        growth = window_update(&t->win, now, total[t->state]);
-                       if (growth < t->threshold)
-                                continue;
+                       if (!t->pending_event) {
+                              if (growth < t->threshold)
+                                        continue;

-                        t->pending_event = true;
+                               t->pending_event = true;
+                       }


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

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

[Hao Lee]

On Sun, Sep 18, 2022 at 10:16:53PM -0700, Suren Baghdasaryan wrote:
> On Sun, Sep 18, 2022 at 3:55 AM Hao Lee <haolee.swjtu@gmail.com> wrote:
> >
> > On Sat, Sep 17, 2022 at 09:44:12PM -0700, Suren Baghdasaryan wrote:
> > > On Sat, Sep 17, 2022 at 12:31 AM Hao Lee <haolee.swjtu@gmail.com> 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@gmail.com> 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@gmail.com>
> > > > > > ---
> > > > > >  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.
> >
> > Hi, for the original code, let's assume t->pending_event is true:
> >     * if new_stall is false, we will try to check event ratelimit and
> >       generate an event for this psi_trigger. This case is right.
> >     * but if new_stall is true, we will skip this psi_trigger if growth
> >       growth < t->threshold. I think we shouldn't skip this psi_trigger
> >       in this case because it has a pending event.
> 
> Ok, I see the issue now. I think the following fix would be the simplest:
> 
>                        /* Calculate growth since last update */
>                         growth = window_update(&t->win, now, total[t->state]);
> -                       if (growth < t->threshold)
> -                                continue;
> +                       if (!t->pending_event) {
> +                              if (growth < t->threshold)
> +                                        continue;
> 
> -                        t->pending_event = true;
> +                               t->pending_event = true;
> +                       }

Great! I will update in v2.

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