* Re: [PATCH linux-next 2/3] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary
2023-10-09 17:42 ` Suren Baghdasaryan
@ 2023-10-10 2:12 ` yang.yang29
2023-10-10 3:05 ` [PATCH linux-next v2 0/4] sched/psi: Optimize the process of updating triggers and rtpoll_total yang.yang29
` (4 subsequent siblings)
5 siblings, 0 replies; 38+ messages in thread
From: yang.yang29 @ 2023-10-10 2:12 UTC (permalink / raw)
To: surenb; +Cc: mingo, peterz, hannes, mingo, linux-kernel, juri.lelli
> 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))
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH linux-next v2 0/4] sched/psi: Optimize the process of updating triggers and rtpoll_total
2023-10-09 17:42 ` Suren Baghdasaryan
2023-10-10 2:12 ` yang.yang29
@ 2023-10-10 3:05 ` yang.yang29
2023-10-10 3:09 ` [PATCH linux-next v2 1/4] sched/psi: Change update_triggers() to a 'void' function yang.yang29
` (3 subsequent siblings)
5 siblings, 0 replies; 38+ messages in thread
From: yang.yang29 @ 2023-10-10 3:05 UTC (permalink / raw)
To: surenb, mingo, peterz, hannes; +Cc: mingo, linux-kernel, juri.lelli
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
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH linux-next v2 1/4] sched/psi: Change update_triggers() to a 'void' function
2023-10-09 17:42 ` Suren Baghdasaryan
2023-10-10 2:12 ` yang.yang29
2023-10-10 3:05 ` [PATCH linux-next v2 0/4] sched/psi: Optimize the process of updating triggers and rtpoll_total yang.yang29
@ 2023-10-10 3:09 ` yang.yang29
2023-10-10 7:38 ` Ingo Molnar
2023-10-10 3:20 ` [PATCH linux-next v2 2/4] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary yang.yang29
` (2 subsequent siblings)
5 siblings, 1 reply; 38+ messages in thread
From: yang.yang29 @ 2023-10-10 3:09 UTC (permalink / raw)
To: surenb, mingo, peterz, hannes; +Cc: mingo, linux-kernel, juri.lelli
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH linux-next v2 1/4] sched/psi: Change update_triggers() to a 'void' function
2023-10-10 3:09 ` [PATCH linux-next v2 1/4] sched/psi: Change update_triggers() to a 'void' function yang.yang29
@ 2023-10-10 7:38 ` Ingo Molnar
2023-10-10 8:31 ` [PATCH linux-next v3 0/4] sched/psi: Optimize the process of updating triggers and rtpoll_total yang.yang29
` (4 more replies)
0 siblings, 5 replies; 38+ messages in thread
From: Ingo Molnar @ 2023-10-10 7:38 UTC (permalink / raw)
To: yang.yang29; +Cc: surenb, peterz, hannes, mingo, linux-kernel, juri.lelli
* 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
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH linux-next v3 0/4] sched/psi: Optimize the process of updating triggers and rtpoll_total
2023-10-10 7:38 ` Ingo Molnar
@ 2023-10-10 8:31 ` yang.yang29
2023-10-10 8:36 ` [PATCH linux-next v3 1/4] sched/psi: Change update_triggers() to a 'void' function yang.yang29
` (3 subsequent siblings)
4 siblings, 0 replies; 38+ messages in thread
From: yang.yang29 @ 2023-10-10 8:31 UTC (permalink / raw)
To: mingo, surenb, peterz, hannes; +Cc: mingo, linux-kernel, juri.lelli
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
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH linux-next v3 1/4] sched/psi: Change update_triggers() to a 'void' function
2023-10-10 7:38 ` Ingo Molnar
2023-10-10 8:31 ` [PATCH linux-next v3 0/4] sched/psi: Optimize the process of updating triggers and rtpoll_total yang.yang29
@ 2023-10-10 8:36 ` yang.yang29
2023-10-19 16:01 ` Suren Baghdasaryan
2023-10-10 8:41 ` [PATCH linux-next v3 2/4] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary yang.yang29
` (2 subsequent siblings)
4 siblings, 1 reply; 38+ messages in thread
From: yang.yang29 @ 2023-10-10 8:36 UTC (permalink / raw)
To: mingo, surenb, peterz, hannes; +Cc: mingo, linux-kernel, juri.lelli
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH linux-next v3 1/4] sched/psi: Change update_triggers() to a 'void' function
2023-10-10 8:36 ` [PATCH linux-next v3 1/4] sched/psi: Change update_triggers() to a 'void' function yang.yang29
@ 2023-10-19 16:01 ` Suren Baghdasaryan
0 siblings, 0 replies; 38+ messages in thread
From: Suren Baghdasaryan @ 2023-10-19 16:01 UTC (permalink / raw)
To: yang.yang29; +Cc: mingo, peterz, hannes, mingo, linux-kernel, juri.lelli
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH linux-next v3 2/4] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary
2023-10-10 7:38 ` Ingo Molnar
2023-10-10 8:31 ` [PATCH linux-next v3 0/4] sched/psi: Optimize the process of updating triggers and rtpoll_total yang.yang29
2023-10-10 8:36 ` [PATCH linux-next v3 1/4] sched/psi: Change update_triggers() to a 'void' function yang.yang29
@ 2023-10-10 8:41 ` yang.yang29
2023-10-11 21:15 ` Ingo Molnar
2023-10-11 21:20 ` [tip: sched/core] sched/psi: Avoid updating PSI triggers and ->rtpoll_total when there are no state changes tip-bot2 for Yang Yang
2023-10-10 8:42 ` [PATCH linux-next v3 3/4] sched/psi: update rtpoll_next_update after update triggers and rtpoll_total yang.yang29
2023-10-10 8:45 ` [PATCH linux-next v3 4/4] sched/psi: Delete the function parameter update_total of update_triggers() yang.yang29
4 siblings, 2 replies; 38+ messages in thread
From: yang.yang29 @ 2023-10-10 8:41 UTC (permalink / raw)
To: mingo, surenb, peterz, hannes; +Cc: linux-kernel, juri.lelli, mingo
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH linux-next v3 2/4] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary
2023-10-10 8:41 ` [PATCH linux-next v3 2/4] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary yang.yang29
@ 2023-10-11 21:15 ` Ingo Molnar
2023-10-12 2:13 ` yang.yang29
2023-10-19 16:04 ` Suren Baghdasaryan
2023-10-11 21:20 ` [tip: sched/core] sched/psi: Avoid updating PSI triggers and ->rtpoll_total when there are no state changes tip-bot2 for Yang Yang
1 sibling, 2 replies; 38+ messages in thread
From: Ingo Molnar @ 2023-10-11 21:15 UTC (permalink / raw)
To: yang.yang29; +Cc: surenb, peterz, hannes, linux-kernel, juri.lelli, mingo
* 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,
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH linux-next v3 2/4] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary
2023-10-11 21:15 ` Ingo Molnar
@ 2023-10-12 2:13 ` yang.yang29
2023-10-19 16:04 ` Suren Baghdasaryan
1 sibling, 0 replies; 38+ messages in thread
From: yang.yang29 @ 2023-10-12 2:13 UTC (permalink / raw)
To: mingo; +Cc: surenb, peterz, hannes, linux-kernel, juri.lelli, mingo
> 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.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH linux-next v3 2/4] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary
2023-10-11 21:15 ` Ingo Molnar
2023-10-12 2:13 ` yang.yang29
@ 2023-10-19 16:04 ` Suren Baghdasaryan
1 sibling, 0 replies; 38+ messages in thread
From: Suren Baghdasaryan @ 2023-10-19 16:04 UTC (permalink / raw)
To: Ingo Molnar; +Cc: yang.yang29, peterz, hannes, linux-kernel, juri.lelli, mingo
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,
^ permalink raw reply [flat|nested] 38+ messages in thread
* [tip: sched/core] sched/psi: Avoid updating PSI triggers and ->rtpoll_total when there are no state changes
2023-10-10 8:41 ` [PATCH linux-next v3 2/4] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary yang.yang29
2023-10-11 21:15 ` Ingo Molnar
@ 2023-10-11 21:20 ` tip-bot2 for Yang Yang
1 sibling, 0 replies; 38+ messages in thread
From: tip-bot2 for Yang Yang @ 2023-10-11 21:20 UTC (permalink / raw)
To: linux-tip-commits
Cc: Yang Yang, Ingo Molnar, Johannes Weiner, Suren Baghdasaryan,
Peter Ziljstra, x86, linux-kernel
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,
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH linux-next v3 3/4] sched/psi: update rtpoll_next_update after update triggers and rtpoll_total
2023-10-10 7:38 ` Ingo Molnar
` (2 preceding siblings ...)
2023-10-10 8:41 ` [PATCH linux-next v3 2/4] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary yang.yang29
@ 2023-10-10 8:42 ` yang.yang29
2023-10-19 16:07 ` Suren Baghdasaryan
2023-10-10 8:45 ` [PATCH linux-next v3 4/4] sched/psi: Delete the function parameter update_total of update_triggers() yang.yang29
4 siblings, 1 reply; 38+ messages in thread
From: yang.yang29 @ 2023-10-10 8:42 UTC (permalink / raw)
To: mingo, surenb, peterz, hannes; +Cc: mingo, linux-kernel, juri.lelli
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH linux-next v3 3/4] sched/psi: update rtpoll_next_update after update triggers and rtpoll_total
2023-10-10 8:42 ` [PATCH linux-next v3 3/4] sched/psi: update rtpoll_next_update after update triggers and rtpoll_total yang.yang29
@ 2023-10-19 16:07 ` Suren Baghdasaryan
0 siblings, 0 replies; 38+ messages in thread
From: Suren Baghdasaryan @ 2023-10-19 16:07 UTC (permalink / raw)
To: yang.yang29; +Cc: mingo, peterz, hannes, mingo, linux-kernel, juri.lelli
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH linux-next v3 4/4] sched/psi: Delete the function parameter update_total of update_triggers()
2023-10-10 7:38 ` Ingo Molnar
` (3 preceding siblings ...)
2023-10-10 8:42 ` [PATCH linux-next v3 3/4] sched/psi: update rtpoll_next_update after update triggers and rtpoll_total yang.yang29
@ 2023-10-10 8:45 ` yang.yang29
2023-10-11 21:20 ` [tip: sched/core] sched/psi: Delete the 'update_total' function parameter from update_triggers() tip-bot2 for Yang Yang
2023-10-19 16:09 ` [PATCH linux-next v3 4/4] sched/psi: Delete the function parameter update_total of update_triggers() Suren Baghdasaryan
4 siblings, 2 replies; 38+ messages in thread
From: yang.yang29 @ 2023-10-10 8:45 UTC (permalink / raw)
To: mingo, surenb, peterz, hannes; +Cc: mingo, linux-kernel, juri.lelli
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* [tip: sched/core] sched/psi: Delete the 'update_total' function parameter from update_triggers()
2023-10-10 8:45 ` [PATCH linux-next v3 4/4] sched/psi: Delete the function parameter update_total of update_triggers() yang.yang29
@ 2023-10-11 21:20 ` tip-bot2 for Yang Yang
2023-10-19 16:09 ` [PATCH linux-next v3 4/4] sched/psi: Delete the function parameter update_total of update_triggers() Suren Baghdasaryan
1 sibling, 0 replies; 38+ messages in thread
From: tip-bot2 for Yang Yang @ 2023-10-11 21:20 UTC (permalink / raw)
To: linux-tip-commits
Cc: Yang Yang, Ingo Molnar, Johannes Weiner, Suren Baghdasaryan,
Peter Ziljstra, x86, linux-kernel
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));
}
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH linux-next v3 4/4] sched/psi: Delete the function parameter update_total of update_triggers()
2023-10-10 8:45 ` [PATCH linux-next v3 4/4] sched/psi: Delete the function parameter update_total of update_triggers() yang.yang29
2023-10-11 21:20 ` [tip: sched/core] sched/psi: Delete the 'update_total' function parameter from update_triggers() tip-bot2 for Yang Yang
@ 2023-10-19 16:09 ` Suren Baghdasaryan
1 sibling, 0 replies; 38+ messages in thread
From: Suren Baghdasaryan @ 2023-10-19 16:09 UTC (permalink / raw)
To: yang.yang29; +Cc: mingo, peterz, hannes, mingo, linux-kernel, juri.lelli
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH linux-next v2 2/4] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary
2023-10-09 17:42 ` Suren Baghdasaryan
` (2 preceding siblings ...)
2023-10-10 3:09 ` [PATCH linux-next v2 1/4] sched/psi: Change update_triggers() to a 'void' function yang.yang29
@ 2023-10-10 3:20 ` yang.yang29
2023-10-10 3:24 ` [PATCH linux-next v2 3/4] sched/psi: update rtpoll_next_update after update triggers and rtpoll_total yang.yang29
2023-10-10 3:28 ` [PATCH linux-next v2 4/4] sched/psi: Delete the function parameter update_total of update_triggers() yang.yang29
5 siblings, 0 replies; 38+ messages in thread
From: yang.yang29 @ 2023-10-10 3:20 UTC (permalink / raw)
To: surenb, mingo, peterz, hannes
Cc: mingo, linux-kernel, juri.lelli, zhang.yunkai, ran.xiaokai
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH linux-next v2 3/4] sched/psi: update rtpoll_next_update after update triggers and rtpoll_total
2023-10-09 17:42 ` Suren Baghdasaryan
` (3 preceding siblings ...)
2023-10-10 3:20 ` [PATCH linux-next v2 2/4] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary yang.yang29
@ 2023-10-10 3:24 ` yang.yang29
2023-10-10 3:28 ` [PATCH linux-next v2 4/4] sched/psi: Delete the function parameter update_total of update_triggers() yang.yang29
5 siblings, 0 replies; 38+ messages in thread
From: yang.yang29 @ 2023-10-10 3:24 UTC (permalink / raw)
To: surenb, mingo, peterz, hannes; +Cc: mingo, linux-kernel, juri.lelli
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH linux-next v2 4/4] sched/psi: Delete the function parameter update_total of update_triggers()
2023-10-09 17:42 ` Suren Baghdasaryan
` (4 preceding siblings ...)
2023-10-10 3:24 ` [PATCH linux-next v2 3/4] sched/psi: update rtpoll_next_update after update triggers and rtpoll_total yang.yang29
@ 2023-10-10 3:28 ` yang.yang29
5 siblings, 0 replies; 38+ messages in thread
From: yang.yang29 @ 2023-10-10 3:28 UTC (permalink / raw)
To: surenb, mingo, peterz, hannes; +Cc: mingo, linux-kernel, juri.lelli
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
^ permalink raw reply related [flat|nested] 38+ messages in thread