Messages in this thread | | | From | Suren Baghdasaryan <> | Date | Sat, 30 Sep 2023 15:32:19 -0700 | Subject | Re: [PATCH linux-next] sched/psi: Avoid update triggers and rtpoll_total when it is unnecessary |
| |
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
| |