diff mbox series

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

Message ID 202310092030430136422@zte.com.cn
State New
Headers
Series sched/psi: Optimize the process of updating triggers and rtpoll_total |

Commit Message

Yang Yang Oct. 9, 2023, 12:30 p.m. UTC 
  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(-)

Comments

Ingo Molnar Oct. 9, 2023, 12:52 p.m. UTC | #1 
* 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
Suren Baghdasaryan Oct. 9, 2023, 5:42 p.m. UTC | #2 
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
Yang Yang Oct. 10, 2023, 2:12 a.m. UTC | #3 
> 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))
diff mbox series

Patch

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,