[PATCH linux-next] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[PATCH linux-next] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[yang.yang29]

From: Yang Yang <yang.yang29@zte.com.cn>

When psimon wakes up and there are no state changes for rtpoll_states,
it's unnecessary to update triggers and rtpoll_total because the pressures
being monitored by user have not changed.

Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
---
 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 81fca77397f6..e4463bb267c3 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -701,7 +701,7 @@ static void psi_rtpoll_work(struct psi_group *group)
 		goto out;
 	}

-	if (now >= group->rtpoll_next_update) {
+	if ((changed_states & group->rtpoll_states) && (now >= group->rtpoll_next_update)) {
 		group->rtpoll_next_update = update_triggers(group, now, &update_total, PSI_POLL);
 		if (update_total)
 			memcpy(group->rtpoll_total, group->total[PSI_POLL],
-- 
2.25.1

Re: [PATCH linux-next] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[Suren Baghdasaryan]

On Wed, Sep 13, 2023 at 12:44 PM <yang.yang29@zte.com.cn> wrote:
>
> From: Yang Yang <yang.yang29@zte.com.cn>
>
> When psimon wakes up and there are no state changes for rtpoll_states,
> it's unnecessary to update triggers and rtpoll_total because the pressures
> being monitored by user have not changed.
>
> Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
> ---
>  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 81fca77397f6..e4463bb267c3 100644
> --- a/kernel/sched/psi.c
> +++ b/kernel/sched/psi.c
> @@ -701,7 +701,7 @@ static void psi_rtpoll_work(struct psi_group *group)
>                 goto out;
>         }
>
> -       if (now >= group->rtpoll_next_update) {
> +       if ((changed_states & group->rtpoll_states) && (now >= group->rtpoll_next_update)) {

With this additional condition group->rtpoll_next_update will not be
updated if states do not change and later on we will have a call to
psi_schedule_rtpoll_work() with the delay calculated as
"nsecs_to_jiffies(group->rtpoll_next_update - now) + 1". With
group->rtpoll_next_update not updated, we can end up with a negative
delay. I don't think that's your intention here, it is?

>                 group->rtpoll_next_update = update_triggers(group, now, &update_total, PSI_POLL);
>                 if (update_total)
>                         memcpy(group->rtpoll_total, group->total[PSI_POLL],
> --
> 2.25.1

Re: [PATCH linux-next] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[yang.yang29]

>  I don't think that's your intention here, it is?
It's an omission, rtpoll_next_update should be update, thanks
for your reviewing. I did a simple test and did not see delay,
maybe it is other reason.

And I read update_triggers() again, it always return 
now + group->rtpoll_min_period, and this return
value is only used by psi_rtpoll_work(). So I think
there is no need for update_triggers() to return
value. So what about this version see below:

---
 kernel/sched/psi.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 81fca77397f6..b6b8af2cf311 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -434,7 +434,7 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
 	return growth;
 }

-static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
+static void update_triggers(struct psi_group *group, u64 now, bool *update_total,
 						   enum psi_aggregators aggregator)
 {
 	struct psi_trigger *t;
@@ -499,8 +499,6 @@ static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
 		/* Reset threshold breach flag once event got generated */
 		t->pending_event = false;
 	}
-
-	return now + group->rtpoll_min_period;
 }

 static u64 update_averages(struct psi_group *group, u64 now)
@@ -699,10 +697,11 @@ static void psi_rtpoll_work(struct psi_group *group)
 	if (now > group->rtpoll_until) {
 		group->rtpoll_next_update = ULLONG_MAX;
 		goto out;
-	}
+	} else
+		group->rtpoll_next_update = now + group->rtpoll_min_period;

-	if (now >= group->rtpoll_next_update) {
-		group->rtpoll_next_update = update_triggers(group, now, &update_total, PSI_POLL);
+	if ((changed_states & group->rtpoll_states) && (now >= group->rtpoll_next_update)) {
+		update_triggers(group, now, &update_total, PSI_POLL);
 		if (update_total)
 			memcpy(group->rtpoll_total, group->total[PSI_POLL],
 				   sizeof(group->rtpoll_total));
-- 
2.25.1

Re: [PATCH linux-next] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[yang.yang29]

Please see this version, thanks.

---
 kernel/sched/psi.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 81fca77397f6..3fe96d77f275 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -434,7 +434,7 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
 	return growth;
 }

-static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
+static void update_triggers(struct psi_group *group, u64 now, bool *update_total,
 						   enum psi_aggregators aggregator)
 {
 	struct psi_trigger *t;
@@ -499,8 +499,6 @@ static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
 		/* Reset threshold breach flag once event got generated */
 		t->pending_event = false;
 	}
-
-	return now + group->rtpoll_min_period;
 }

 static u64 update_averages(struct psi_group *group, u64 now)
@@ -702,10 +700,14 @@ static void psi_rtpoll_work(struct psi_group *group)
 	}

 	if (now >= group->rtpoll_next_update) {
-		group->rtpoll_next_update = update_triggers(group, now, &update_total, PSI_POLL);
-		if (update_total)
-			memcpy(group->rtpoll_total, group->total[PSI_POLL],
-				   sizeof(group->rtpoll_total));
+		group->rtpoll_next_update = now + group->rtpoll_min_period;
+
+		if (changed_states & group->rtpoll_states) {
+			update_triggers(group, now, &update_total, PSI_POLL);
+			if (update_total)
+				memcpy(group->rtpoll_total, group->total[PSI_POLL],
+					   sizeof(group->rtpoll_total));
+		}
 	}

 	psi_schedule_rtpoll_work(group,
-- 
2.25.1

Re: [PATCH linux-next] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[yang.yang29]

Update_total in update_triggers() is also only used by psi_rtpoll_work().
And when changed_states & group->rtpoll_states is true, update_total
should also be true, so try to delete update_total, please see below:

---
 kernel/sched/psi.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 81fca77397f6..744ba8b4e029 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -434,14 +434,13 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
 	return growth;
 }

-static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
+static void update_triggers(struct psi_group *group, u64 now,
 						   enum psi_aggregators aggregator)
 {
 	struct psi_trigger *t;
 	u64 *total = group->total[aggregator];
 	struct list_head *triggers;
 	u64 *aggregator_total;
-	*update_total = false;

 	if (aggregator == PSI_AVGS) {
 		triggers = &group->avg_triggers;
@@ -471,14 +470,6 @@ static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
 		 * events without dropping any).
 		 */
 		if (new_stall) {
-			/*
-			 * Multiple triggers might be looking at the same state,
-			 * remember to update group->polling_total[] once we've
-			 * been through all of them. Also remember to extend the
-			 * polling time if we see new stall activity.
-			 */
-			*update_total = true;
-
 			/* Calculate growth since last update */
 			growth = window_update(&t->win, now, total[t->state]);
 			if (!t->pending_event) {
@@ -499,8 +490,6 @@ static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
 		/* Reset threshold breach flag once event got generated */
 		t->pending_event = false;
 	}
-
-	return now + group->rtpoll_min_period;
 }

 static u64 update_averages(struct psi_group *group, u64 now)
@@ -561,7 +550,6 @@ static void psi_avgs_work(struct work_struct *work)
 	struct delayed_work *dwork;
 	struct psi_group *group;
 	u32 changed_states;
-	bool update_total;
 	u64 now;

 	dwork = to_delayed_work(work);
@@ -580,7 +568,7 @@ static void psi_avgs_work(struct work_struct *work)
 	 * go - see calc_avgs() and missed_periods.
 	 */
 	if (now >= group->avg_next_update) {
-		update_triggers(group, now, &update_total, PSI_AVGS);
+		update_triggers(group, now, PSI_AVGS);
 		group->avg_next_update = update_averages(group, now);
 	}

@@ -636,7 +624,6 @@ static void psi_rtpoll_work(struct psi_group *group)
 {
 	bool force_reschedule = false;
 	u32 changed_states;
-	bool update_total;
 	u64 now;

 	mutex_lock(&group->rtpoll_trigger_lock);
@@ -702,10 +689,13 @@ static void psi_rtpoll_work(struct psi_group *group)
 	}

 	if (now >= group->rtpoll_next_update) {
-		group->rtpoll_next_update = update_triggers(group, now, &update_total, PSI_POLL);
-		if (update_total)
+		group->rtpoll_next_update = now + group->rtpoll_min_period;
+
+		if (changed_states & group->rtpoll_states) {
+			update_triggers(group, now, PSI_POLL);
 			memcpy(group->rtpoll_total, group->total[PSI_POLL],
 				   sizeof(group->rtpoll_total));
+		}
 	}

 	psi_schedule_rtpoll_work(group,
-- 
2.25.1

Re: [PATCH linux-next] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[Suren Baghdasaryan]

On Thu, Sep 14, 2023 at 2:10 AM <yang.yang29@zte.com.cn> wrote:
>
> Update_total in update_triggers() is also only used by psi_rtpoll_work().
> And when changed_states & group->rtpoll_states is true, update_total
> should also be true, so try to delete update_total, please see below:

Sorry, I know I owe a review here. I'll try to review it by the end of
this week.
Suren.

>
> ---
>  kernel/sched/psi.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
>
> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> index 81fca77397f6..744ba8b4e029 100644
> --- a/kernel/sched/psi.c
> +++ b/kernel/sched/psi.c
> @@ -434,14 +434,13 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
>         return growth;
>  }
>
> -static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
> +static void update_triggers(struct psi_group *group, u64 now,
>                                                    enum psi_aggregators aggregator)
>  {
>         struct psi_trigger *t;
>         u64 *total = group->total[aggregator];
>         struct list_head *triggers;
>         u64 *aggregator_total;
> -       *update_total = false;
>
>         if (aggregator == PSI_AVGS) {
>                 triggers = &group->avg_triggers;
> @@ -471,14 +470,6 @@ static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
>                  * events without dropping any).
>                  */
>                 if (new_stall) {
> -                       /*
> -                        * Multiple triggers might be looking at the same state,
> -                        * remember to update group->polling_total[] once we've
> -                        * been through all of them. Also remember to extend the
> -                        * polling time if we see new stall activity.
> -                        */
> -                       *update_total = true;
> -
>                         /* Calculate growth since last update */
>                         growth = window_update(&t->win, now, total[t->state]);
>                         if (!t->pending_event) {
> @@ -499,8 +490,6 @@ static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
>                 /* Reset threshold breach flag once event got generated */
>                 t->pending_event = false;
>         }
> -
> -       return now + group->rtpoll_min_period;
>  }
>
>  static u64 update_averages(struct psi_group *group, u64 now)
> @@ -561,7 +550,6 @@ static void psi_avgs_work(struct work_struct *work)
>         struct delayed_work *dwork;
>         struct psi_group *group;
>         u32 changed_states;
> -       bool update_total;
>         u64 now;
>
>         dwork = to_delayed_work(work);
> @@ -580,7 +568,7 @@ static void psi_avgs_work(struct work_struct *work)
>          * go - see calc_avgs() and missed_periods.
>          */
>         if (now >= group->avg_next_update) {
> -               update_triggers(group, now, &update_total, PSI_AVGS);
> +               update_triggers(group, now, PSI_AVGS);
>                 group->avg_next_update = update_averages(group, now);
>         }
>
> @@ -636,7 +624,6 @@ static void psi_rtpoll_work(struct psi_group *group)
>  {
>         bool force_reschedule = false;
>         u32 changed_states;
> -       bool update_total;
>         u64 now;
>
>         mutex_lock(&group->rtpoll_trigger_lock);
> @@ -702,10 +689,13 @@ static void psi_rtpoll_work(struct psi_group *group)
>         }
>
>         if (now >= group->rtpoll_next_update) {
> -               group->rtpoll_next_update = update_triggers(group, now, &update_total, PSI_POLL);
> -               if (update_total)
> +               group->rtpoll_next_update = now + group->rtpoll_min_period;
> +
> +               if (changed_states & group->rtpoll_states) {
> +                       update_triggers(group, now, PSI_POLL);
>                         memcpy(group->rtpoll_total, group->total[PSI_POLL],
>                                    sizeof(group->rtpoll_total));
> +               }
>         }
>
>         psi_schedule_rtpoll_work(group,
> --
> 2.25.1

Re: [PATCH linux-next] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[yang.yang29]

> Sorry, I know I owe a review here. I'll try to review it by the end of
> this week.

Never mind, always appreciate your reviewing. Hope don't disturb your
weekend rest.

Re: [PATCH linux-next] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[Suren Baghdasaryan]

On Thu, Sep 14, 2023 at 2:10 AM <yang.yang29@zte.com.cn> wrote:
>
> Update_total in update_triggers() is also only used by psi_rtpoll_work().
> And when changed_states & group->rtpoll_states is true, update_total
> should also be true, so try to delete update_total, please see below:


Ok, sorry for the delay. I wanted to check for any side-effects and
recheck the whole logic since the code has changed quite a bit since
it was introduced...
I think this optimization is safe and won't have side-effects, however
I haven't tested it yet. One small comment above and when you post the
V2 please include peterz@infradead.org. Peter is hosting PSI in his
tree, so he is the maintainer you absolutely need :)

>
> ---
>  kernel/sched/psi.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
>
> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> index 81fca77397f6..744ba8b4e029 100644
> --- a/kernel/sched/psi.c
> +++ b/kernel/sched/psi.c
> @@ -434,14 +434,13 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
>         return growth;
>  }
>
> -static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
> +static void update_triggers(struct psi_group *group, u64 now,
>                                                    enum psi_aggregators aggregator)
>  {
>         struct psi_trigger *t;
>         u64 *total = group->total[aggregator];
>         struct list_head *triggers;
>         u64 *aggregator_total;
> -       *update_total = false;
>
>         if (aggregator == PSI_AVGS) {
>                 triggers = &group->avg_triggers;
> @@ -471,14 +470,6 @@ static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
>                  * events without dropping any).
>                  */
>                 if (new_stall) {
> -                       /*
> -                        * Multiple triggers might be looking at the same state,
> -                        * remember to update group->polling_total[] once we've
> -                        * been through all of them. Also remember to extend the
> -                        * polling time if we see new stall activity.
> -                        */
> -                       *update_total = true;
> -
>                         /* Calculate growth since last update */
>                         growth = window_update(&t->win, now, total[t->state]);
>                         if (!t->pending_event) {
> @@ -499,8 +490,6 @@ static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
>                 /* Reset threshold breach flag once event got generated */
>                 t->pending_event = false;
>         }
> -
> -       return now + group->rtpoll_min_period;
>  }
>
>  static u64 update_averages(struct psi_group *group, u64 now)
> @@ -561,7 +550,6 @@ static void psi_avgs_work(struct work_struct *work)
>         struct delayed_work *dwork;
>         struct psi_group *group;
>         u32 changed_states;
> -       bool update_total;
>         u64 now;
>
>         dwork = to_delayed_work(work);
> @@ -580,7 +568,7 @@ static void psi_avgs_work(struct work_struct *work)
>          * go - see calc_avgs() and missed_periods.
>          */
>         if (now >= group->avg_next_update) {
> -               update_triggers(group, now, &update_total, PSI_AVGS);
> +               update_triggers(group, now, PSI_AVGS);
>                 group->avg_next_update = update_averages(group, now);
>         }
>
> @@ -636,7 +624,6 @@ static void psi_rtpoll_work(struct psi_group *group)
>  {
>         bool force_reschedule = false;
>         u32 changed_states;
> -       bool update_total;
>         u64 now;
>
>         mutex_lock(&group->rtpoll_trigger_lock);
> @@ -702,10 +689,13 @@ static void psi_rtpoll_work(struct psi_group *group)
>         }
>
>         if (now >= group->rtpoll_next_update) {
> -               group->rtpoll_next_update = update_triggers(group, now, &update_total, PSI_POLL);
> -               if (update_total)
> +               group->rtpoll_next_update = now + group->rtpoll_min_period;

It's not technically wrong and does not have side-effects today but to
be safe, please update group->rtpoll_next_update *after* you call
update_triggers() and update group->rtpoll_total. This will prevent
bugs if update_triggers() uses group->rtpoll_next_update in the future
and I think it makes more sense to set the next update time after we
finished the current update. So, the code should look something like:

        if (now >= group->rtpoll_next_update) {
                if (changed_states & group->rtpoll_states) {
                        update_triggers(group, now, PSI_POLL);
                        memcpy(group->rtpoll_total, group->total[PSI_POLL],
                                sizeof(group->rtpoll_total));
                }
                group->rtpoll_next_update = now + group->rtpoll_min_period;
        }

Thanks,
Suren.

> +
> +               if (changed_states & group->rtpoll_states) {
> +                       update_triggers(group, now, PSI_POLL);
>                         memcpy(group->rtpoll_total, group->total[PSI_POLL],
>                                    sizeof(group->rtpoll_total));
> +               }
>         }
>
>         psi_schedule_rtpoll_work(group,
> --
> 2.25.1

[PATCH linux-next v2] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[yang.yang29]

From: Yang Yang <yang.yang29@zte.com.cn>

When psimon wakes up and there are no state changes for rtpoll_states,
it's unnecessary to update triggers and rtpoll_total because the pressures
being monitored by user have not changed. This will help to slightly reduce
unnecessary computations of psi.

There are also some minor related optimizations, please see below.

The parameter update_total in update_triggers() is useless now. Since if
changed_states & group->rtpoll_states is true, new_stall in update_triggers()
will be true, then update_total should also be true. We have no need for
update_total to help judgment whether to update rtpoll_total, so delete
update_total.

Update_triggers() always return now + group->rtpoll_min_period, and the
return value is only used by psi_rtpoll_work(), so change update_triggers()
to a void function, let group->rtpoll_next_update = now +
group->rtpoll_min_period directly.

Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
Suggested-by: Suren Baghdasaryan <surenb@google.com>
---
 kernel/sched/psi.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 1d0f634725a6..44a78774ae87 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -434,14 +434,13 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
 	return growth;
 }

-static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
+static void update_triggers(struct psi_group *group, u64 now,
 						   enum psi_aggregators aggregator)
 {
 	struct psi_trigger *t;
 	u64 *total = group->total[aggregator];
 	struct list_head *triggers;
 	u64 *aggregator_total;
-	*update_total = false;

 	if (aggregator == PSI_AVGS) {
 		triggers = &group->avg_triggers;
@@ -471,14 +470,6 @@ static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
 		 * events without dropping any).
 		 */
 		if (new_stall) {
-			/*
-			 * Multiple triggers might be looking at the same state,
-			 * remember to update group->polling_total[] once we've
-			 * been through all of them. Also remember to extend the
-			 * polling time if we see new stall activity.
-			 */
-			*update_total = true;
-
 			/* Calculate growth since last update */
 			growth = window_update(&t->win, now, total[t->state]);
 			if (!t->pending_event) {
@@ -503,8 +494,6 @@ static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
 		/* Reset threshold breach flag once event got generated */
 		t->pending_event = false;
 	}
-
-	return now + group->rtpoll_min_period;
 }

 static u64 update_averages(struct psi_group *group, u64 now)
@@ -565,7 +554,6 @@ static void psi_avgs_work(struct work_struct *work)
 	struct delayed_work *dwork;
 	struct psi_group *group;
 	u32 changed_states;
-	bool update_total;
 	u64 now;

 	dwork = to_delayed_work(work);
@@ -584,7 +572,7 @@ static void psi_avgs_work(struct work_struct *work)
 	 * go - see calc_avgs() and missed_periods.
 	 */
 	if (now >= group->avg_next_update) {
-		update_triggers(group, now, &update_total, PSI_AVGS);
+		update_triggers(group, now, PSI_AVGS);
 		group->avg_next_update = update_averages(group, now);
 	}

@@ -640,7 +628,6 @@ static void psi_rtpoll_work(struct psi_group *group)
 {
 	bool force_reschedule = false;
 	u32 changed_states;
-	bool update_total;
 	u64 now;

 	mutex_lock(&group->rtpoll_trigger_lock);
@@ -706,10 +693,12 @@ static void psi_rtpoll_work(struct psi_group *group)
 	}

 	if (now >= group->rtpoll_next_update) {
-		group->rtpoll_next_update = update_triggers(group, now, &update_total, PSI_POLL);
-		if (update_total)
+		if (changed_states & group->rtpoll_states) {
+			update_triggers(group, now, PSI_POLL);
 			memcpy(group->rtpoll_total, group->total[PSI_POLL],
 				   sizeof(group->rtpoll_total));
+		}
+		group->rtpoll_next_update = now + group->rtpoll_min_period;
 	}

 	psi_schedule_rtpoll_work(group,
-- 
2.25.1

Re: [PATCH linux-next v2] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[Ingo Molnar]

* yang.yang29@zte.com.cn <yang.yang29@zte.com.cn> wrote:

> From: Yang Yang <yang.yang29@zte.com.cn>
> 
> When psimon wakes up and there are no state changes for rtpoll_states,
> it's unnecessary to update triggers and rtpoll_total because the pressures
> being monitored by user have not changed. This will help to slightly reduce
> unnecessary computations of psi.
> 
> There are also some minor related optimizations, please see below.
> 
> The parameter update_total in update_triggers() is useless now. Since if
> changed_states & group->rtpoll_states is true, new_stall in update_triggers()
> will be true, then update_total should also be true. We have no need for
> update_total to help judgment whether to update rtpoll_total, so delete
> update_total.
> 
> Update_triggers() always return now + group->rtpoll_min_period, and the
> return value is only used by psi_rtpoll_work(), so change update_triggers()
> to a void function, let group->rtpoll_next_update = now +
> group->rtpoll_min_period directly.

Yeah, so please split this up into 3 patches: one change per patch, even
if each patch is relatively small.

Thanks,

	Ingo

[PATCH linux-next 0/3] sched/psi: Optimize the process of updating triggers and rtpoll_total

[yang.yang29]

From: Yang Yang <yang.yang29@zte.com.cn>

When psimon wakes up and there are no state changes for rtpoll_states,
it's unnecessary to update triggers and rtpoll_total because the pressures
being monitored by user have not changed. This will help to slightly reduce
unnecessary computations of psi.

There are also some minor related optimizations, please see below.

The parameter update_total in update_triggers() is useless now. Since if
changed_states & group->rtpoll_states is true, new_stall in update_triggers()
will be true, then update_total should also be true. We have no need for
update_total to help judgment whether to update rtpoll_total, so delete
update_total.

Update_triggers() always return now + group->rtpoll_min_period, and the
return value is only used by psi_rtpoll_work(), so change update_triggers()
to a void function, let group->rtpoll_next_update = now +
group->rtpoll_min_period directly.

Yang Yang(3):
  sched/psi: Change update_triggers() to a void function
  sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary
  sched/psi: Delete the function parameter update_total of update_triggers()

 kernel/sched/psi.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

-- 
2.25.1

[PATCH linux-next 1/3] sched/psi: Change update_triggers() to a void function

[yang.yang29]

From: Yang Yang <yang.yang29@zte.com.cn>

Update_triggers() always return now + group->rtpoll_min_period, and the
return value is only used by psi_rtpoll_work(), so change update_triggers()
to a void function, let group->rtpoll_next_update = now +
group->rtpoll_min_period directly.
This will avoid unnecessary function return value passing.

Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
Suggested-by: Suren Baghdasaryan <surenb@google.com>
Suggested-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/psi.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 1d0f634725a6..be853f227e40 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -434,7 +434,7 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
 	return growth;
 }

-static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
+static void update_triggers(struct psi_group *group, u64 now, bool *update_total,
 						   enum psi_aggregators aggregator)
 {
 	struct psi_trigger *t;
@@ -503,8 +503,6 @@ static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
 		/* Reset threshold breach flag once event got generated */
 		t->pending_event = false;
 	}
-
-	return now + group->rtpoll_min_period;
 }

 static u64 update_averages(struct psi_group *group, u64 now)
@@ -706,7 +704,8 @@ static void psi_rtpoll_work(struct psi_group *group)
 	}

 	if (now >= group->rtpoll_next_update) {
-		group->rtpoll_next_update = update_triggers(group, now, &update_total, PSI_POLL);
+		update_triggers(group, now, &update_total, PSI_POLL);
+		group->rtpoll_next_update = now + group->rtpoll_min_period;
 		if (update_total)
 			memcpy(group->rtpoll_total, group->total[PSI_POLL],
 				   sizeof(group->rtpoll_total));
-- 
2.25.1

[PATCH linux-next 2/3] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[yang.yang29]

From: Yang Yang <yang.yang29@zte.com.cn>

When psimon wakes up and there are no state changes for rtpoll_states,
it's unnecessary to update triggers and rtpoll_total because the pressures
being monitored by user had not changed. This will help to slightly reduce
unnecessary computations of psi.

And update group->rtpoll_next_update after called update_triggers() and
update rtpoll_total. This will prevent bugs if update_triggers() uses
group->rtpoll_next_update in the future, and it makes more sense
to set the next update time after we finished the current update.

Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
Suggested-by: Suren Baghdasaryan <surenb@google.com>
Suggested-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/psi.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index be853f227e40..79f8db0c6150 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -704,11 +704,12 @@ static void psi_rtpoll_work(struct psi_group *group)
 	}

 	if (now >= group->rtpoll_next_update) {
-		update_triggers(group, now, &update_total, PSI_POLL);
-		group->rtpoll_next_update = now + group->rtpoll_min_period;
-		if (update_total)
+		if (changed_states & group->rtpoll_states) {
+			update_triggers(group, now, &update_total, PSI_POLL);
 			memcpy(group->rtpoll_total, group->total[PSI_POLL],
 				   sizeof(group->rtpoll_total));
+		}
+		group->rtpoll_next_update = now + group->rtpoll_min_period;
 	}

 	psi_schedule_rtpoll_work(group,
-- 
2.25.1

[PATCH linux-next 3/3] sched/psi: Delete the function parameter update_total of update_triggers()

[yang.yang29]

From: Yang Yang <yang.yang29@zte.com.cn>

The parameter update_total in update_triggers() is useless after patch
"sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary".
If changed_states & group->rtpoll_states is true, new_stall in
update_triggers() will be true, then update_total should also
be true. We have no need for update_total to help judgment
whether to update rtpoll_total, so delete it.
This makes the code cleaner.

Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
Suggested-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/psi.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 79f8db0c6150..44a78774ae87 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -434,14 +434,13 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
 	return growth;
 }

-static void update_triggers(struct psi_group *group, u64 now, bool *update_total,
+static void update_triggers(struct psi_group *group, u64 now,
 						   enum psi_aggregators aggregator)
 {
 	struct psi_trigger *t;
 	u64 *total = group->total[aggregator];
 	struct list_head *triggers;
 	u64 *aggregator_total;
-	*update_total = false;

 	if (aggregator == PSI_AVGS) {
 		triggers = &group->avg_triggers;
@@ -471,14 +470,6 @@ static void update_triggers(struct psi_group *group, u64 now, bool *update_total
 		 * events without dropping any).
 		 */
 		if (new_stall) {
-			/*
-			 * Multiple triggers might be looking at the same state,
-			 * remember to update group->polling_total[] once we've
-			 * been through all of them. Also remember to extend the
-			 * polling time if we see new stall activity.
-			 */
-			*update_total = true;
-
 			/* Calculate growth since last update */
 			growth = window_update(&t->win, now, total[t->state]);
 			if (!t->pending_event) {
@@ -563,7 +554,6 @@ static void psi_avgs_work(struct work_struct *work)
 	struct delayed_work *dwork;
 	struct psi_group *group;
 	u32 changed_states;
-	bool update_total;
 	u64 now;

 	dwork = to_delayed_work(work);
@@ -582,7 +572,7 @@ static void psi_avgs_work(struct work_struct *work)
 	 * go - see calc_avgs() and missed_periods.
 	 */
 	if (now >= group->avg_next_update) {
-		update_triggers(group, now, &update_total, PSI_AVGS);
+		update_triggers(group, now, PSI_AVGS);
 		group->avg_next_update = update_averages(group, now);
 	}

@@ -638,7 +628,6 @@ static void psi_rtpoll_work(struct psi_group *group)
 {
 	bool force_reschedule = false;
 	u32 changed_states;
-	bool update_total;
 	u64 now;

 	mutex_lock(&group->rtpoll_trigger_lock);
@@ -705,7 +694,7 @@ static void psi_rtpoll_work(struct psi_group *group)

 	if (now >= group->rtpoll_next_update) {
 		if (changed_states & group->rtpoll_states) {
-			update_triggers(group, now, &update_total, PSI_POLL);
+			update_triggers(group, now, PSI_POLL);
 			memcpy(group->rtpoll_total, group->total[PSI_POLL],
 				   sizeof(group->rtpoll_total));
 		}
-- 
2.25.1

Re: [PATCH linux-next 1/3] sched/psi: Change update_triggers() to a void function

[Ingo Molnar]

* yang.yang29@zte.com.cn <yang.yang29@zte.com.cn> wrote:

> From: Yang Yang <yang.yang29@zte.com.cn>
> 
> Update_triggers() always return now + group->rtpoll_min_period, and the
> return value is only used by psi_rtpoll_work(), so change update_triggers()
> to a void function, let group->rtpoll_next_update = now +
> group->rtpoll_min_period directly.
> This will avoid unnecessary function return value passing.
> 
> Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
> Suggested-by: Suren Baghdasaryan <surenb@google.com>
> Suggested-by: Ingo Molnar <mingo@kernel.org>

Thanks for the split-up!

I'll remove the Suggested-by tags you added for me: I didn't suggest the
changes themselves, only the split-up, which isn't credit-worthy :-)

Thanks,

	Ingo

Re: [PATCH linux-next 2/3] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[Ingo Molnar]

* yang.yang29@zte.com.cn <yang.yang29@zte.com.cn> wrote:

> From: Yang Yang <yang.yang29@zte.com.cn>
> 
> When psimon wakes up and there are no state changes for rtpoll_states,
> it's unnecessary to update triggers and rtpoll_total because the pressures
> being monitored by user had not changed. This will help to slightly reduce
> unnecessary computations of psi.
> 
> And update group->rtpoll_next_update after called update_triggers() and
> update rtpoll_total. This will prevent bugs if update_triggers() uses
> group->rtpoll_next_update in the future, and it makes more sense
> to set the next update time after we finished the current update.

>  	if (now >= group->rtpoll_next_update) {
> -		update_triggers(group, now, &update_total, PSI_POLL);
> -		group->rtpoll_next_update = now + group->rtpoll_min_period;
> -		if (update_total)
> +		if (changed_states & group->rtpoll_states) {
> +			update_triggers(group, now, &update_total, PSI_POLL);
>  			memcpy(group->rtpoll_total, group->total[PSI_POLL],
>  				   sizeof(group->rtpoll_total));
> +		}
> +		group->rtpoll_next_update = now + group->rtpoll_min_period;

So please also split out the second change into a separate patch as well, 
as it's an unrelated patch to the state-change optimization.

We have a "one conceptual change per patch" rule for most things.

Thanks,

	Ingo

[tip: sched/core] sched/psi: Change update_triggers() to a 'void' function

[tip-bot2 for Yang Yang]

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     e03dc9fa0663bc303383170e961561462ff00c93
Gitweb:        https://git.kernel.org/tip/e03dc9fa0663bc303383170e961561462ff00c93
Author:        Yang Yang <yang.yang29@zte.com.cn>
AuthorDate:    Mon, 09 Oct 2023 20:24:28 +08:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Mon, 09 Oct 2023 14:54:50 +02:00

sched/psi: Change update_triggers() to a 'void' function

Update_triggers() always returns now + group->rtpoll_min_period, and the
return value is only used by psi_rtpoll_work(), so change update_triggers()
to a void function, let group->rtpoll_next_update = now +
group->rtpoll_min_period directly.

This will avoid unnecessary function return value passing & simplifies
the function.

[ mingo: Updated changelog ]

Suggested-by: Suren Baghdasaryan <surenb@google.com>
Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lore.kernel.org/r/202310092024289721617@zte.com.cn
---
 kernel/sched/psi.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 1d0f634..be853f2 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -434,7 +434,7 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
 	return growth;
 }
 
-static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
+static void update_triggers(struct psi_group *group, u64 now, bool *update_total,
 						   enum psi_aggregators aggregator)
 {
 	struct psi_trigger *t;
@@ -503,8 +503,6 @@ static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
 		/* Reset threshold breach flag once event got generated */
 		t->pending_event = false;
 	}
-
-	return now + group->rtpoll_min_period;
 }
 
 static u64 update_averages(struct psi_group *group, u64 now)
@@ -706,7 +704,8 @@ static void psi_rtpoll_work(struct psi_group *group)
 	}
 
 	if (now >= group->rtpoll_next_update) {
-		group->rtpoll_next_update = update_triggers(group, now, &update_total, PSI_POLL);
+		update_triggers(group, now, &update_total, PSI_POLL);
+		group->rtpoll_next_update = now + group->rtpoll_min_period;
 		if (update_total)
 			memcpy(group->rtpoll_total, group->total[PSI_POLL],
 				   sizeof(group->rtpoll_total));

Re: [PATCH linux-next 2/3] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[Suren Baghdasaryan]

On Mon, Oct 9, 2023 at 5:52 AM Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * yang.yang29@zte.com.cn <yang.yang29@zte.com.cn> wrote:
>
> > From: Yang Yang <yang.yang29@zte.com.cn>
> >
> > When psimon wakes up and there are no state changes for rtpoll_states,
> > it's unnecessary to update triggers and rtpoll_total because the pressures
> > being monitored by user had not changed. This will help to slightly reduce
> > unnecessary computations of psi.
> >
> > And update group->rtpoll_next_update after called update_triggers() and
> > update rtpoll_total. This will prevent bugs if update_triggers() uses
> > group->rtpoll_next_update in the future, and it makes more sense
> > to set the next update time after we finished the current update.
>
> >       if (now >= group->rtpoll_next_update) {
> > -             update_triggers(group, now, &update_total, PSI_POLL);
> > -             group->rtpoll_next_update = now + group->rtpoll_min_period;
> > -             if (update_total)
> > +             if (changed_states & group->rtpoll_states) {
> > +                     update_triggers(group, now, &update_total, PSI_POLL);
> >                       memcpy(group->rtpoll_total, group->total[PSI_POLL],
> >                                  sizeof(group->rtpoll_total));
> > +             }
> > +             group->rtpoll_next_update = now + group->rtpoll_min_period;
>
> So please also split out the second change into a separate patch as well,
> as it's an unrelated patch to the state-change optimization.

I think that the second part could have been done in the first patch
to place the "group->rtpoll_next_update = now +
group->rtpoll_min_period" line at the right place from the beginning.
Also when posting the next version please add the version number to
all the patch titles in the patchset, not only to the cover letter.
That helps with finding the latest version.
Thanks!

>
> We have a "one conceptual change per patch" rule for most things.
>
> Thanks,
>
>         Ingo

Re: [PATCH linux-next 2/3] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[yang.yang29]

> I think that the second part could have been done in the first patch
> to place the "group->rtpoll_next_update = now +
> group->rtpoll_min_period" line at the right place from the beginning.

Thanks for your advice, if we strict follow "one conceptual change per patch"
rule, I think "group->rtpoll_next_update = ..." should be in another patch.

> Also when posting the next version please add the version number to
> all the patch titles in the patchset, not only to the cover letter.
> That helps with finding the latest version.
> Thanks!

Get it, thanks to your reminder. I treat the split-up patches  as new patches
previously, so didn't add the version number. I will add version number in
follow-up patches.

> One small comment above and when you post the V2 please include
> peterz@infradead.org. Peter is hosting PSI in his tree, so he is the
> maintainer you absolutely need :)

I get the maintainer information from get_maintainer.pl, it said Peter is
a reviewer, maybe get_maintainer.pl should update ?
Johannes Weiner <hannes@cmpxchg.org> (maintainer:PRESSURE STALL INFORMATION (PSI))
Suren Baghdasaryan <surenb@google.com> (maintainer:PRESSURE STALL INFORMATION (PSI))
Peter Ziljstra <peterz@infradead.org> (reviewer:PRESSURE STALL INFORMATION (PSI))

[PATCH linux-next v2 0/4] sched/psi: Optimize the process of updating triggers and rtpoll_total

[yang.yang29]

From: Yang Yang <yang.yang29@zte.com.cn>

When psimon wakes up and there are no state changes for rtpoll_states,
it's unnecessary to update triggers and rtpoll_total because the pressures
being monitored by user have not changed. This will help to slightly reduce
unnecessary computations of psi.

There are also some minor related optimizations, please see below.

Update_triggers() always return now + group->rtpoll_min_period, and the
return value is only used by psi_rtpoll_work(), so change update_triggers()
to a void function, let group->rtpoll_next_update = now +
group->rtpoll_min_period directly.

Update group->rtpoll_next_update after called update_triggers() and
update rtpoll_total. This will prevent bugs if update_triggers() uses
group->rtpoll_next_update in the future, and it makes more sense
to set the next update time after we finished the current update.

The parameter update_total in update_triggers() is useless now. Since if
changed_states & group->rtpoll_states is true, new_stall in update_triggers()
will be true, then update_total should also be true. We have no need for
update_total to help judgment whether to update rtpoll_total, so delete
update_total.

Change log
----------
v1->v2:
----------
(1) the line number of group->rtpoll_next_update in patch 1 remain unchanged
(2) split patch 3 from patch 2

Yang Yang (4):
  sched/psi: Change update_triggers() to a void function
  sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary
  sched/psi: Update rtpoll_next_update after update triggers and rtpoll_total
  sched/psi: Delete the function parameter update_total of update_triggers()

 kernel/sched/psi.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

-- 
2.25.1

[PATCH linux-next v2 1/4] sched/psi: Change update_triggers() to a 'void' function

[yang.yang29]

From: Yang Yang <yang.yang29@zte.com.cn>

Update_triggers() always returns now + group->rtpoll_min_period, and the
return value is only used by psi_rtpoll_work(), so change update_triggers()
to a void function, let group->rtpoll_next_update = now +
group->rtpoll_min_period directly.

This will avoid unnecessary function return value passing & simplifies
the function.

Suggested-by: Suren Baghdasaryan <surenb@google.com>
Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
---
 kernel/sched/psi.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 1d0f634725a6..fec8aab096a8 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -434,7 +434,7 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
 	return growth;
 }

-static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
+static void update_triggers(struct psi_group *group, u64 now, bool *update_total,
 						   enum psi_aggregators aggregator)
 {
 	struct psi_trigger *t;
@@ -503,8 +503,6 @@ static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
 		/* Reset threshold breach flag once event got generated */
 		t->pending_event = false;
 	}
-
-	return now + group->rtpoll_min_period;
 }

 static u64 update_averages(struct psi_group *group, u64 now)
@@ -706,7 +704,8 @@ static void psi_rtpoll_work(struct psi_group *group)
 	}

 	if (now >= group->rtpoll_next_update) {
-		group->rtpoll_next_update = update_triggers(group, now, &update_total, PSI_POLL);
+		group->rtpoll_next_update = now + group->rtpoll_min_period;
+		update_triggers(group, now, &update_total, PSI_POLL);
 		if (update_total)
 			memcpy(group->rtpoll_total, group->total[PSI_POLL],
 				   sizeof(group->rtpoll_total));
-- 
2.25.1

[PATCH linux-next v2 2/4] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[yang.yang29]

From: Yang Yang <yang.yang29@zte.com.cn>

When psimon wakes up and there are no state changes for rtpoll_states,
it's unnecessary to update triggers and rtpoll_total because the pressures
being monitored by user had not changed.
This will help to slightly reduce unnecessary computations of psi.

Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
Cc: Zhang Yunkai <zhang.yunkai@zte.com.cn>
Cc: Ran Xiaokai <ran.xiaokai@zte.com.cn>
---
 kernel/sched/psi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index fec8aab096a8..143f8eb34f9d 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -705,10 +705,11 @@ static void psi_rtpoll_work(struct psi_group *group)

 	if (now >= group->rtpoll_next_update) {
 		group->rtpoll_next_update = now + group->rtpoll_min_period;
-		update_triggers(group, now, &update_total, PSI_POLL);
-		if (update_total)
+		if (changed_states & group->rtpoll_states) {
+			update_triggers(group, now, &update_total, PSI_POLL);
 			memcpy(group->rtpoll_total, group->total[PSI_POLL],
 				   sizeof(group->rtpoll_total));
+		}
 	}

 	psi_schedule_rtpoll_work(group,
-- 
2.25.1

[PATCH linux-next v2 3/4] sched/psi: update rtpoll_next_update after update triggers and rtpoll_total

[yang.yang29]

From: Yang Yang <yang.yang29@zte.com.cn>

Update group->rtpoll_next_update after called update_triggers() and
update rtpoll_total. This will prevent bugs if update_triggers() uses
group->rtpoll_next_update in the future, and it makes more sense
to set the next update time after we finished the current update.

Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
Suggested-by: Suren Baghdasaryan <surenb@google.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 143f8eb34f9d..79f8db0c6150 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -704,12 +704,12 @@ static void psi_rtpoll_work(struct psi_group *group)
 	}

 	if (now >= group->rtpoll_next_update) {
-		group->rtpoll_next_update = now + group->rtpoll_min_period;
 		if (changed_states & group->rtpoll_states) {
 			update_triggers(group, now, &update_total, PSI_POLL);
 			memcpy(group->rtpoll_total, group->total[PSI_POLL],
 				   sizeof(group->rtpoll_total));
 		}
+		group->rtpoll_next_update = now + group->rtpoll_min_period;
 	}

 	psi_schedule_rtpoll_work(group,
-- 
2.25.1

[PATCH linux-next v2 4/4] sched/psi: Delete the function parameter update_total of update_triggers()

[yang.yang29]

From: Yang Yang <yang.yang29@zte.com.cn>

The parameter update_total in update_triggers() is useless after patch
"sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary".
If changed_states & group->rtpoll_states is true, new_stall in
update_triggers() will be true, then update_total should also
be true. We have no need for update_total to help judgment
whether to update rtpoll_total, so delete it.
This makes the code cleaner & simpler.

Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
---
 kernel/sched/psi.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 79f8db0c6150..44a78774ae87 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -434,14 +434,13 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
 	return growth;
 }

-static void update_triggers(struct psi_group *group, u64 now, bool *update_total,
+static void update_triggers(struct psi_group *group, u64 now,
 						   enum psi_aggregators aggregator)
 {
 	struct psi_trigger *t;
 	u64 *total = group->total[aggregator];
 	struct list_head *triggers;
 	u64 *aggregator_total;
-	*update_total = false;

 	if (aggregator == PSI_AVGS) {
 		triggers = &group->avg_triggers;
@@ -471,14 +470,6 @@ static void update_triggers(struct psi_group *group, u64 now, bool *update_total
 		 * events without dropping any).
 		 */
 		if (new_stall) {
-			/*
-			 * Multiple triggers might be looking at the same state,
-			 * remember to update group->polling_total[] once we've
-			 * been through all of them. Also remember to extend the
-			 * polling time if we see new stall activity.
-			 */
-			*update_total = true;
-
 			/* Calculate growth since last update */
 			growth = window_update(&t->win, now, total[t->state]);
 			if (!t->pending_event) {
@@ -563,7 +554,6 @@ static void psi_avgs_work(struct work_struct *work)
 	struct delayed_work *dwork;
 	struct psi_group *group;
 	u32 changed_states;
-	bool update_total;
 	u64 now;

 	dwork = to_delayed_work(work);
@@ -582,7 +572,7 @@ static void psi_avgs_work(struct work_struct *work)
 	 * go - see calc_avgs() and missed_periods.
 	 */
 	if (now >= group->avg_next_update) {
-		update_triggers(group, now, &update_total, PSI_AVGS);
+		update_triggers(group, now, PSI_AVGS);
 		group->avg_next_update = update_averages(group, now);
 	}

@@ -638,7 +628,6 @@ static void psi_rtpoll_work(struct psi_group *group)
 {
 	bool force_reschedule = false;
 	u32 changed_states;
-	bool update_total;
 	u64 now;

 	mutex_lock(&group->rtpoll_trigger_lock);
@@ -705,7 +694,7 @@ static void psi_rtpoll_work(struct psi_group *group)

 	if (now >= group->rtpoll_next_update) {
 		if (changed_states & group->rtpoll_states) {
-			update_triggers(group, now, &update_total, PSI_POLL);
+			update_triggers(group, now, PSI_POLL);
 			memcpy(group->rtpoll_total, group->total[PSI_POLL],
 				   sizeof(group->rtpoll_total));
 		}
-- 
2.25.1

Re: [PATCH linux-next v2 1/4] sched/psi: Change update_triggers() to a 'void' function

[Ingo Molnar]

* yang.yang29@zte.com.cn <yang.yang29@zte.com.cn> wrote:

> From: Yang Yang <yang.yang29@zte.com.cn>
> 
> Update_triggers() always returns now + group->rtpoll_min_period, and the
> return value is only used by psi_rtpoll_work(), so change update_triggers()
> to a void function, let group->rtpoll_next_update = now +
> group->rtpoll_min_period directly.
> 
> This will avoid unnecessary function return value passing & simplifies
> the function.
> 
> Suggested-by: Suren Baghdasaryan <surenb@google.com>
> Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
> ---
>  kernel/sched/psi.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> index 1d0f634725a6..fec8aab096a8 100644
> --- a/kernel/sched/psi.c
> +++ b/kernel/sched/psi.c
> @@ -434,7 +434,7 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
>  	return growth;
>  }
> 
> -static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
> +static void update_triggers(struct psi_group *group, u64 now, bool *update_total,
>  						   enum psi_aggregators aggregator)
>  {
>  	struct psi_trigger *t;
> @@ -503,8 +503,6 @@ static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
>  		/* Reset threshold breach flag once event got generated */
>  		t->pending_event = false;
>  	}
> -
> -	return now + group->rtpoll_min_period;
>  }
> 
>  static u64 update_averages(struct psi_group *group, u64 now)
> @@ -706,7 +704,8 @@ static void psi_rtpoll_work(struct psi_group *group)
>  	}
> 
>  	if (now >= group->rtpoll_next_update) {
> -		group->rtpoll_next_update = update_triggers(group, now, &update_total, PSI_POLL);
> +		group->rtpoll_next_update = now + group->rtpoll_min_period;
> +		update_triggers(group, now, &update_total, PSI_POLL);

This step is wrong. The equivalent transformation when removing a return value is:

	x = fn(y); // fn(y) returns 'z'

to:

	fn(y);
	x = z;

not:

	x = z;
	fn(y);

...

Furthermore, I already applied the correct #1 patch to the scheduler tree, see:

  e03dc9fa0663 ("sched/psi: Change update_triggers() to a 'void' function")

... which tree is at:

  git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git sched/core

So please send the remaining patch on top of that.

Thanks,

	Ingo

[PATCH linux-next v3 0/4] sched/psi: Optimize the process of updating triggers and rtpoll_total

[yang.yang29]

From: Yang Yang <yang.yang29@zte.com.cn>

When psimon wakes up and there are no state changes for rtpoll_states,
it's unnecessary to update triggers and rtpoll_total because the pressures
being monitored by user have not changed. This will help to slightly reduce
unnecessary computations of psi.

There are also some minor related optimizations, please see below.

Update_triggers() always return now + group->rtpoll_min_period, and the
return value is only used by psi_rtpoll_work(), so change update_triggers()
to a void function, let group->rtpoll_next_update = now +
group->rtpoll_min_period directly.

Update group->rtpoll_next_update after called update_triggers() and
update rtpoll_total. This will prevent bugs if update_triggers() uses
group->rtpoll_next_update in the future, and it makes more sense
to set the next update time after we finished the current update.

The parameter update_total in update_triggers() is useless now. Since if
changed_states & group->rtpoll_states is true, new_stall in update_triggers()
will be true, then update_total should also be true. We have no need for
update_total to help judgment whether to update rtpoll_total, so delete
update_total.

Change log
----------
v2->v3:
----------
(1) revert #1 patch to the same as:
  e03dc9fa0663 ("sched/psi: Change update_triggers() to a 'void' function")
(2) remake the remaining patches on top of that #1 patch
----------
(1) the line number of group->rtpoll_next_update in patch 1 remain unchanged
(2) split patch 3 from patch 2

Yang Yang (4):
sched/psi: Change update_triggers() to a 'void' function
sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary
sched/psi: Update rtpoll_next_update after update triggers and rtpoll_total
sched/psi: Delete the function parameter update_total of update_triggers()

kernel/sched/psi.c | 23 ++++++-----------------
1 file changed, 6 insertions(+), 17 deletions(-)

--
2.25.1

[PATCH linux-next v3 1/4] sched/psi: Change update_triggers() to a 'void' function

[yang.yang29]

From: Yang Yang <yang.yang29@zte.com.cn>

Update_triggers() always returns now + group->rtpoll_min_period, and the
return value is only used by psi_rtpoll_work(), so change update_triggers()
to a void function, let group->rtpoll_next_update = now +
group->rtpoll_min_period directly.

This will avoid unnecessary function return value passing & simplifies
the function.

Suggested-by: Suren Baghdasaryan <surenb@google.com>
Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
---
 kernel/sched/psi.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 1d0f634725a6..be853f227e40 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -434,7 +434,7 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
 	return growth;
 }

-static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
+static void update_triggers(struct psi_group *group, u64 now, bool *update_total,
 						   enum psi_aggregators aggregator)
 {
 	struct psi_trigger *t;
@@ -503,8 +503,6 @@ static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
 		/* Reset threshold breach flag once event got generated */
 		t->pending_event = false;
 	}
-
-	return now + group->rtpoll_min_period;
 }

 static u64 update_averages(struct psi_group *group, u64 now)
@@ -706,7 +704,8 @@ static void psi_rtpoll_work(struct psi_group *group)
 	}

 	if (now >= group->rtpoll_next_update) {
-		group->rtpoll_next_update = update_triggers(group, now, &update_total, PSI_POLL);
+		update_triggers(group, now, &update_total, PSI_POLL);
+		group->rtpoll_next_update = now + group->rtpoll_min_period;
 		if (update_total)
 			memcpy(group->rtpoll_total, group->total[PSI_POLL],
 				   sizeof(group->rtpoll_total));
-- 
2.25.1

[PATCH linux-next v3 2/4] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[yang.yang29]

From: Yang Yang <yang.yang29@zte.com.cn>

When psimon wakes up and there are no state changes for rtpoll_states,
it's unnecessary to update triggers and rtpoll_total because the pressures
being monitored by user had not changed.
This will help to slightly reduce unnecessary computations of psi.

Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
Cc: Zhang Yunkai <zhang.yunkai@zte.com.cn>
Cc: Ran Xiaokai <ran.xiaokai@zte.com.cn>
---
 kernel/sched/psi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index be853f227e40..143f8eb34f9d 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -704,11 +704,12 @@ static void psi_rtpoll_work(struct psi_group *group)
 	}

 	if (now >= group->rtpoll_next_update) {
-		update_triggers(group, now, &update_total, PSI_POLL);
 		group->rtpoll_next_update = now + group->rtpoll_min_period;
-		if (update_total)
+		if (changed_states & group->rtpoll_states) {
+			update_triggers(group, now, &update_total, PSI_POLL);
 			memcpy(group->rtpoll_total, group->total[PSI_POLL],
 				   sizeof(group->rtpoll_total));
+		}
 	}

 	psi_schedule_rtpoll_work(group,
-- 
2.25.1

[PATCH linux-next v3 3/4] sched/psi: update rtpoll_next_update after update triggers and rtpoll_total

[yang.yang29]

From: Yang Yang <yang.yang29@zte.com.cn>

Update group->rtpoll_next_update after called update_triggers() and
update rtpoll_total. This will prevent bugs if update_triggers() uses
group->rtpoll_next_update in the future, and it makes more sense
to set the next update time after we finished the current update.

Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
Suggested-by: Suren Baghdasaryan <surenb@google.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 143f8eb34f9d..79f8db0c6150 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -704,12 +704,12 @@ static void psi_rtpoll_work(struct psi_group *group)
 	}

 	if (now >= group->rtpoll_next_update) {
-		group->rtpoll_next_update = now + group->rtpoll_min_period;
 		if (changed_states & group->rtpoll_states) {
 			update_triggers(group, now, &update_total, PSI_POLL);
 			memcpy(group->rtpoll_total, group->total[PSI_POLL],
 				   sizeof(group->rtpoll_total));
 		}
+		group->rtpoll_next_update = now + group->rtpoll_min_period;
 	}

 	psi_schedule_rtpoll_work(group,
-- 
2.25.1

[PATCH linux-next v3 4/4] sched/psi: Delete the function parameter update_total of update_triggers()

[yang.yang29]

From: Yang Yang <yang.yang29@zte.com.cn>

The parameter update_total in update_triggers() is useless after patch
"sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary".
If changed_states & group->rtpoll_states is true, new_stall in
update_triggers() will be true, then update_total should also
be true. We have no need for update_total to help judgment
whether to update rtpoll_total, so delete it.
This makes the code cleaner & simpler.

Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
---
 kernel/sched/psi.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 79f8db0c6150..44a78774ae87 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -434,14 +434,13 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
 	return growth;
 }

-static void update_triggers(struct psi_group *group, u64 now, bool *update_total,
+static void update_triggers(struct psi_group *group, u64 now,
 						   enum psi_aggregators aggregator)
 {
 	struct psi_trigger *t;
 	u64 *total = group->total[aggregator];
 	struct list_head *triggers;
 	u64 *aggregator_total;
-	*update_total = false;

 	if (aggregator == PSI_AVGS) {
 		triggers = &group->avg_triggers;
@@ -471,14 +470,6 @@ static void update_triggers(struct psi_group *group, u64 now, bool *update_total
 		 * events without dropping any).
 		 */
 		if (new_stall) {
-			/*
-			 * Multiple triggers might be looking at the same state,
-			 * remember to update group->polling_total[] once we've
-			 * been through all of them. Also remember to extend the
-			 * polling time if we see new stall activity.
-			 */
-			*update_total = true;
-
 			/* Calculate growth since last update */
 			growth = window_update(&t->win, now, total[t->state]);
 			if (!t->pending_event) {
@@ -563,7 +554,6 @@ static void psi_avgs_work(struct work_struct *work)
 	struct delayed_work *dwork;
 	struct psi_group *group;
 	u32 changed_states;
-	bool update_total;
 	u64 now;

 	dwork = to_delayed_work(work);
@@ -582,7 +572,7 @@ static void psi_avgs_work(struct work_struct *work)
 	 * go - see calc_avgs() and missed_periods.
 	 */
 	if (now >= group->avg_next_update) {
-		update_triggers(group, now, &update_total, PSI_AVGS);
+		update_triggers(group, now, PSI_AVGS);
 		group->avg_next_update = update_averages(group, now);
 	}

@@ -638,7 +628,6 @@ static void psi_rtpoll_work(struct psi_group *group)
 {
 	bool force_reschedule = false;
 	u32 changed_states;
-	bool update_total;
 	u64 now;

 	mutex_lock(&group->rtpoll_trigger_lock);
@@ -705,7 +694,7 @@ static void psi_rtpoll_work(struct psi_group *group)

 	if (now >= group->rtpoll_next_update) {
 		if (changed_states & group->rtpoll_states) {
-			update_triggers(group, now, &update_total, PSI_POLL);
+			update_triggers(group, now, PSI_POLL);
 			memcpy(group->rtpoll_total, group->total[PSI_POLL],
 				   sizeof(group->rtpoll_total));
 		}
-- 
2.25.1

Re: [PATCH linux-next v3 2/4] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[Ingo Molnar]

* yang.yang29@zte.com.cn <yang.yang29@zte.com.cn> wrote:

> From: Yang Yang <yang.yang29@zte.com.cn>
> 
> When psimon wakes up and there are no state changes for rtpoll_states,
> it's unnecessary to update triggers and rtpoll_total because the pressures
> being monitored by user had not changed.
> This will help to slightly reduce unnecessary computations of psi.
> 
> Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
> Cc: Zhang Yunkai <zhang.yunkai@zte.com.cn>
> Cc: Ran Xiaokai <ran.xiaokai@zte.com.cn>
> ---
>  kernel/sched/psi.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> index be853f227e40..143f8eb34f9d 100644
> --- a/kernel/sched/psi.c
> +++ b/kernel/sched/psi.c
> @@ -704,11 +704,12 @@ static void psi_rtpoll_work(struct psi_group *group)
>  	}
> 
>  	if (now >= group->rtpoll_next_update) {
> -		update_triggers(group, now, &update_total, PSI_POLL);
>  		group->rtpoll_next_update = now + group->rtpoll_min_period;
> -		if (update_total)
> +		if (changed_states & group->rtpoll_states) {
> +			update_triggers(group, now, &update_total, PSI_POLL);
>  			memcpy(group->rtpoll_total, group->total[PSI_POLL],
>  				   sizeof(group->rtpoll_total));
> +		}

Yeah, so I believe we may have been talking past each other for past 
versions of this patch: why is this patch modifying the order of the 
modification to group->rtpoll_next_update?

It should do the below sequence, nothing more - see the patch attached 
below. This is basically a combination of patches #2 and #3.

And then the final patch removes the now superfluous 'update_total' 
parameter, which is always true.

Here are the commits I applied to tip:sched/core:

  e03dc9fa0663 sched/psi: Change update_triggers() to a 'void' function
  ...
  80cc1d1d5ee3 sched/psi: Avoid updating PSI triggers and ->rtpoll_total when there are no state changes
  3657680f38cd sched/psi: Delete the 'update_total' function parameter from update_triggers()

I rewrote the changelogs for readability.

Thanks,

	Ingo

===================>
From: Yang Yang <yang.yang29@zte.com.cn>
Date: Tue, 10 Oct 2023 16:41:07 +0800
Subject: [PATCH] sched/psi: Avoid updating PSI triggers and ->rtpoll_total when there are no state changes

When psimon wakes up and there are no state changes for ->rtpoll_states,
it's unnecessary to update triggers and ->rtpoll_total because the pressures
being monitored by the user have not changed.

This will help to slightly reduce unnecessary computations of PSI.

[ mingo: Changelog updates ]

Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Peter Ziljstra <peterz@infradead.org>
Link: https://lore.kernel.org/r/202310101641075436843@zte.com.cn
---
 kernel/sched/psi.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index be853f227e40..79f8db0c6150 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -704,11 +704,12 @@ static void psi_rtpoll_work(struct psi_group *group)
 	}
 
 	if (now >= group->rtpoll_next_update) {
-		update_triggers(group, now, &update_total, PSI_POLL);
-		group->rtpoll_next_update = now + group->rtpoll_min_period;
-		if (update_total)
+		if (changed_states & group->rtpoll_states) {
+			update_triggers(group, now, &update_total, PSI_POLL);
 			memcpy(group->rtpoll_total, group->total[PSI_POLL],
 				   sizeof(group->rtpoll_total));
+		}
+		group->rtpoll_next_update = now + group->rtpoll_min_period;
 	}
 
 	psi_schedule_rtpoll_work(group,

[tip: sched/core] sched/psi: Delete the 'update_total' function parameter from update_triggers()

[tip-bot2 for Yang Yang]

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     3657680f38cd7df413d665f2b2f38e9a78130d8b
Gitweb:        https://git.kernel.org/tip/3657680f38cd7df413d665f2b2f38e9a78130d8b
Author:        Yang Yang <yang.yang29@zte.com.cn>
AuthorDate:    Tue, 10 Oct 2023 16:45:43 +08:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Wed, 11 Oct 2023 23:08:09 +02:00

sched/psi: Delete the 'update_total' function parameter from update_triggers()

The 'update_total' parameter of update_triggers() is always true after the
previous commit:

  80cc1d1d5ee3 ("sched/psi: Avoid updating PSI triggers and ->rtpoll_total when there are no state changes")

If the 'changed_states & group->rtpoll_states' condition is true,
'new_stall' in update_triggers() will be true, and then 'update_total'
should also be true.

So update_total is redundant - remove it.

[ mingo: Changelog updates ]

Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Peter Ziljstra <peterz@infradead.org>
Link: https://lore.kernel.org/r/202310101645437859599@zte.com.cn
---
 kernel/sched/psi.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 79f8db0..44a7877 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -434,14 +434,13 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
 	return growth;
 }
 
-static void update_triggers(struct psi_group *group, u64 now, bool *update_total,
+static void update_triggers(struct psi_group *group, u64 now,
 						   enum psi_aggregators aggregator)
 {
 	struct psi_trigger *t;
 	u64 *total = group->total[aggregator];
 	struct list_head *triggers;
 	u64 *aggregator_total;
-	*update_total = false;
 
 	if (aggregator == PSI_AVGS) {
 		triggers = &group->avg_triggers;
@@ -471,14 +470,6 @@ static void update_triggers(struct psi_group *group, u64 now, bool *update_total
 		 * events without dropping any).
 		 */
 		if (new_stall) {
-			/*
-			 * Multiple triggers might be looking at the same state,
-			 * remember to update group->polling_total[] once we've
-			 * been through all of them. Also remember to extend the
-			 * polling time if we see new stall activity.
-			 */
-			*update_total = true;
-
 			/* Calculate growth since last update */
 			growth = window_update(&t->win, now, total[t->state]);
 			if (!t->pending_event) {
@@ -563,7 +554,6 @@ static void psi_avgs_work(struct work_struct *work)
 	struct delayed_work *dwork;
 	struct psi_group *group;
 	u32 changed_states;
-	bool update_total;
 	u64 now;
 
 	dwork = to_delayed_work(work);
@@ -582,7 +572,7 @@ static void psi_avgs_work(struct work_struct *work)
 	 * go - see calc_avgs() and missed_periods.
 	 */
 	if (now >= group->avg_next_update) {
-		update_triggers(group, now, &update_total, PSI_AVGS);
+		update_triggers(group, now, PSI_AVGS);
 		group->avg_next_update = update_averages(group, now);
 	}
 
@@ -638,7 +628,6 @@ static void psi_rtpoll_work(struct psi_group *group)
 {
 	bool force_reschedule = false;
 	u32 changed_states;
-	bool update_total;
 	u64 now;
 
 	mutex_lock(&group->rtpoll_trigger_lock);
@@ -705,7 +694,7 @@ static void psi_rtpoll_work(struct psi_group *group)
 
 	if (now >= group->rtpoll_next_update) {
 		if (changed_states & group->rtpoll_states) {
-			update_triggers(group, now, &update_total, PSI_POLL);
+			update_triggers(group, now, PSI_POLL);
 			memcpy(group->rtpoll_total, group->total[PSI_POLL],
 				   sizeof(group->rtpoll_total));
 		}

[tip: sched/core] sched/psi: Avoid updating PSI triggers and ->rtpoll_total when there are no state changes

[tip-bot2 for Yang Yang]

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     80cc1d1d5ee35701daf11725ce06d8a240588973
Gitweb:        https://git.kernel.org/tip/80cc1d1d5ee35701daf11725ce06d8a240588973
Author:        Yang Yang <yang.yang29@zte.com.cn>
AuthorDate:    Tue, 10 Oct 2023 16:41:07 +08:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Wed, 11 Oct 2023 23:07:50 +02:00

sched/psi: Avoid updating PSI triggers and ->rtpoll_total when there are no state changes

When psimon wakes up and there are no state changes for ->rtpoll_states,
it's unnecessary to update triggers and ->rtpoll_total because the pressures
being monitored by the user have not changed.

This will help to slightly reduce unnecessary computations of PSI.

[ mingo: Changelog updates ]

Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Peter Ziljstra <peterz@infradead.org>
Link: https://lore.kernel.org/r/202310101641075436843@zte.com.cn
---
 kernel/sched/psi.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index be853f2..79f8db0 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -704,11 +704,12 @@ static void psi_rtpoll_work(struct psi_group *group)
 	}
 
 	if (now >= group->rtpoll_next_update) {
-		update_triggers(group, now, &update_total, PSI_POLL);
-		group->rtpoll_next_update = now + group->rtpoll_min_period;
-		if (update_total)
+		if (changed_states & group->rtpoll_states) {
+			update_triggers(group, now, &update_total, PSI_POLL);
 			memcpy(group->rtpoll_total, group->total[PSI_POLL],
 				   sizeof(group->rtpoll_total));
+		}
+		group->rtpoll_next_update = now + group->rtpoll_min_period;
 	}
 
 	psi_schedule_rtpoll_work(group,

Re: [PATCH linux-next v3 2/4] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[yang.yang29]

> Yeah, so I believe we may have been talking past each other for past 
> versions of this patch: why is this patch modifying the order of the 
> modification to group->rtpoll_next_update?

Yes it is, I may not have fully expressed the reasons clearly before.

> I rewrote the changelogs for readability.

Much grateful for your guidance and work. Learned a lot.

Re: [PATCH linux-next v3 1/4] sched/psi: Change update_triggers() to a 'void' function

[Suren Baghdasaryan]

On Tue, Oct 10, 2023 at 1:36 AM <yang.yang29@zte.com.cn> wrote:
>
> From: Yang Yang <yang.yang29@zte.com.cn>
>
> Update_triggers() always returns now + group->rtpoll_min_period, and the
> return value is only used by psi_rtpoll_work(), so change update_triggers()
> to a void function, let group->rtpoll_next_update = now +
> group->rtpoll_min_period directly.
>
> This will avoid unnecessary function return value passing & simplifies
> the function.
>
> Suggested-by: Suren Baghdasaryan <surenb@google.com>
> Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>

Sorry for the delay, I was traveling and just got back. I think Ingo
already applied this patch but FWIW

Acked-by: Suren Baghdasaryan <surenb@google.com>

> ---
>  kernel/sched/psi.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> index 1d0f634725a6..be853f227e40 100644
> --- a/kernel/sched/psi.c
> +++ b/kernel/sched/psi.c
> @@ -434,7 +434,7 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
>         return growth;
>  }
>
> -static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
> +static void update_triggers(struct psi_group *group, u64 now, bool *update_total,
>                                                    enum psi_aggregators aggregator)
>  {
>         struct psi_trigger *t;
> @@ -503,8 +503,6 @@ static u64 update_triggers(struct psi_group *group, u64 now, bool *update_total,
>                 /* Reset threshold breach flag once event got generated */
>                 t->pending_event = false;
>         }
> -
> -       return now + group->rtpoll_min_period;
>  }
>
>  static u64 update_averages(struct psi_group *group, u64 now)
> @@ -706,7 +704,8 @@ static void psi_rtpoll_work(struct psi_group *group)
>         }
>
>         if (now >= group->rtpoll_next_update) {
> -               group->rtpoll_next_update = update_triggers(group, now, &update_total, PSI_POLL);
> +               update_triggers(group, now, &update_total, PSI_POLL);
> +               group->rtpoll_next_update = now + group->rtpoll_min_period;
>                 if (update_total)
>                         memcpy(group->rtpoll_total, group->total[PSI_POLL],
>                                    sizeof(group->rtpoll_total));
> --
> 2.25.1

Re: [PATCH linux-next v3 2/4] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary

[Suren Baghdasaryan]

On Wed, Oct 11, 2023 at 2:15 PM Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * yang.yang29@zte.com.cn <yang.yang29@zte.com.cn> wrote:
>
> > From: Yang Yang <yang.yang29@zte.com.cn>
> >
> > When psimon wakes up and there are no state changes for rtpoll_states,
> > it's unnecessary to update triggers and rtpoll_total because the pressures
> > being monitored by user had not changed.
> > This will help to slightly reduce unnecessary computations of psi.
> >
> > Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
> > Cc: Zhang Yunkai <zhang.yunkai@zte.com.cn>
> > Cc: Ran Xiaokai <ran.xiaokai@zte.com.cn>
> > ---
> >  kernel/sched/psi.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> > index be853f227e40..143f8eb34f9d 100644
> > --- a/kernel/sched/psi.c
> > +++ b/kernel/sched/psi.c
> > @@ -704,11 +704,12 @@ static void psi_rtpoll_work(struct psi_group *group)
> >       }
> >
> >       if (now >= group->rtpoll_next_update) {
> > -             update_triggers(group, now, &update_total, PSI_POLL);
> >               group->rtpoll_next_update = now + group->rtpoll_min_period;
> > -             if (update_total)
> > +             if (changed_states & group->rtpoll_states) {
> > +                     update_triggers(group, now, &update_total, PSI_POLL);
> >                       memcpy(group->rtpoll_total, group->total[PSI_POLL],
> >                                  sizeof(group->rtpoll_total));
> > +             }
>
> Yeah, so I believe we may have been talking past each other for past
> versions of this patch: why is this patch modifying the order of the
> modification to group->rtpoll_next_update?
>
> It should do the below sequence, nothing more - see the patch attached
> below. This is basically a combination of patches #2 and #3.
>
> And then the final patch removes the now superfluous 'update_total'
> parameter, which is always true.
>
> Here are the commits I applied to tip:sched/core:
>
>   e03dc9fa0663 sched/psi: Change update_triggers() to a 'void' function
>   ...
>   80cc1d1d5ee3 sched/psi: Avoid updating PSI triggers and ->rtpoll_total when there are no state changes
>   3657680f38cd sched/psi: Delete the 'update_total' function parameter from update_triggers()
>
> I rewrote the changelogs for readability.
>
> Thanks,
>
>         Ingo
>
> ===================>
> From: Yang Yang <yang.yang29@zte.com.cn>
> Date: Tue, 10 Oct 2023 16:41:07 +0800
> Subject: [PATCH] sched/psi: Avoid updating PSI triggers and ->rtpoll_total when there are no state changes
>
> When psimon wakes up and there are no state changes for ->rtpoll_states,
> it's unnecessary to update triggers and ->rtpoll_total because the pressures
> being monitored by the user have not changed.
>
> This will help to slightly reduce unnecessary computations of PSI.
>
> [ mingo: Changelog updates ]
>
> Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Peter Ziljstra <peterz@infradead.org>
> Link: https://lore.kernel.org/r/202310101641075436843@zte.com.cn

This version looks correct to me.

Acked-by: Suren Baghdasaryan <surenb@google.com>

> ---
>  kernel/sched/psi.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> index be853f227e40..79f8db0c6150 100644
> --- a/kernel/sched/psi.c
> +++ b/kernel/sched/psi.c
> @@ -704,11 +704,12 @@ static void psi_rtpoll_work(struct psi_group *group)
>         }
>
>         if (now >= group->rtpoll_next_update) {
> -               update_triggers(group, now, &update_total, PSI_POLL);
> -               group->rtpoll_next_update = now + group->rtpoll_min_period;
> -               if (update_total)
> +               if (changed_states & group->rtpoll_states) {
> +                       update_triggers(group, now, &update_total, PSI_POLL);
>                         memcpy(group->rtpoll_total, group->total[PSI_POLL],
>                                    sizeof(group->rtpoll_total));
> +               }
> +               group->rtpoll_next_update = now + group->rtpoll_min_period;
>         }
>
>         psi_schedule_rtpoll_work(group,

Re: [PATCH linux-next v3 3/4] sched/psi: update rtpoll_next_update after update triggers and rtpoll_total

[Suren Baghdasaryan]

On Tue, Oct 10, 2023 at 1:43 AM <yang.yang29@zte.com.cn> wrote:
>
> From: Yang Yang <yang.yang29@zte.com.cn>
>
> Update group->rtpoll_next_update after called update_triggers() and
> update rtpoll_total. This will prevent bugs if update_triggers() uses
> group->rtpoll_next_update in the future, and it makes more sense
> to set the next update time after we finished the current update.
>
> Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
> Suggested-by: Suren Baghdasaryan <surenb@google.com>

I believe Ingo's version at
https://lore.kernel.org/all/ZScQZLTssSfq19Jm@gmail.com/ already
includes this change.

> ---
>  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 143f8eb34f9d..79f8db0c6150 100644
> --- a/kernel/sched/psi.c
> +++ b/kernel/sched/psi.c
> @@ -704,12 +704,12 @@ static void psi_rtpoll_work(struct psi_group *group)
>         }
>
>         if (now >= group->rtpoll_next_update) {
> -               group->rtpoll_next_update = now + group->rtpoll_min_period;
>                 if (changed_states & group->rtpoll_states) {
>                         update_triggers(group, now, &update_total, PSI_POLL);
>                         memcpy(group->rtpoll_total, group->total[PSI_POLL],
>                                    sizeof(group->rtpoll_total));
>                 }
> +               group->rtpoll_next_update = now + group->rtpoll_min_period;
>         }
>
>         psi_schedule_rtpoll_work(group,
> --
> 2.25.1

Re: [PATCH linux-next v3 4/4] sched/psi: Delete the function parameter update_total of update_triggers()

[Suren Baghdasaryan]

On Tue, Oct 10, 2023 at 1:45 AM <yang.yang29@zte.com.cn> wrote:
>
> From: Yang Yang <yang.yang29@zte.com.cn>
>
> The parameter update_total in update_triggers() is useless after patch
> "sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary".
> If changed_states & group->rtpoll_states is true, new_stall in
> update_triggers() will be true, then update_total should also
> be true. We have no need for update_total to help judgment
> whether to update rtpoll_total, so delete it.
> This makes the code cleaner & simpler.
>
> Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>

Acked-by: Suren Baghdasaryan <surenb@google.com>

> ---
>  kernel/sched/psi.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> index 79f8db0c6150..44a78774ae87 100644
> --- a/kernel/sched/psi.c
> +++ b/kernel/sched/psi.c
> @@ -434,14 +434,13 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
>         return growth;
>  }
>
> -static void update_triggers(struct psi_group *group, u64 now, bool *update_total,
> +static void update_triggers(struct psi_group *group, u64 now,
>                                                    enum psi_aggregators aggregator)
>  {
>         struct psi_trigger *t;
>         u64 *total = group->total[aggregator];
>         struct list_head *triggers;
>         u64 *aggregator_total;
> -       *update_total = false;
>
>         if (aggregator == PSI_AVGS) {
>                 triggers = &group->avg_triggers;
> @@ -471,14 +470,6 @@ static void update_triggers(struct psi_group *group, u64 now, bool *update_total
>                  * events without dropping any).
>                  */
>                 if (new_stall) {
> -                       /*
> -                        * Multiple triggers might be looking at the same state,
> -                        * remember to update group->polling_total[] once we've
> -                        * been through all of them. Also remember to extend the
> -                        * polling time if we see new stall activity.
> -                        */
> -                       *update_total = true;
> -
>                         /* Calculate growth since last update */
>                         growth = window_update(&t->win, now, total[t->state]);
>                         if (!t->pending_event) {
> @@ -563,7 +554,6 @@ static void psi_avgs_work(struct work_struct *work)
>         struct delayed_work *dwork;
>         struct psi_group *group;
>         u32 changed_states;
> -       bool update_total;
>         u64 now;
>
>         dwork = to_delayed_work(work);
> @@ -582,7 +572,7 @@ static void psi_avgs_work(struct work_struct *work)
>          * go - see calc_avgs() and missed_periods.
>          */
>         if (now >= group->avg_next_update) {
> -               update_triggers(group, now, &update_total, PSI_AVGS);
> +               update_triggers(group, now, PSI_AVGS);
>                 group->avg_next_update = update_averages(group, now);
>         }
>
> @@ -638,7 +628,6 @@ static void psi_rtpoll_work(struct psi_group *group)
>  {
>         bool force_reschedule = false;
>         u32 changed_states;
> -       bool update_total;
>         u64 now;
>
>         mutex_lock(&group->rtpoll_trigger_lock);
> @@ -705,7 +694,7 @@ static void psi_rtpoll_work(struct psi_group *group)
>
>         if (now >= group->rtpoll_next_update) {
>                 if (changed_states & group->rtpoll_states) {
> -                       update_triggers(group, now, &update_total, PSI_POLL);
> +                       update_triggers(group, now, PSI_POLL);
>                         memcpy(group->rtpoll_total, group->total[PSI_POLL],
>                                    sizeof(group->rtpoll_total));
>                 }
> --
> 2.25.1