* [PATCH] schied/fair: Skip updating "contrib" without load @ 2019-12-06 16:14 Peng Wang 2019-12-09 16:16 ` Peter Zijlstra 2019-12-13 3:45 ` [PATCH v2] schied/fair: Skip calculating @contrib " Peng Wang 0 siblings, 2 replies; 7+ messages in thread From: Peng Wang @ 2019-12-06 16:14 UTC (permalink / raw) To: mingo, peterz, juri.lelli, vincent.guittot, dietmar.eggemann, rostedt, bsegall, mgorman Cc: linux-kernel, Peng Wang We only update load_sum/runnable_load_sum/util_sum with decayed old sum when load is clear. Signed-off-by: Peng Wang <rocking@linux.alibaba.com> --- kernel/sched/pelt.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/kernel/sched/pelt.c b/kernel/sched/pelt.c index a96db50..4392953 100644 --- a/kernel/sched/pelt.c +++ b/kernel/sched/pelt.c @@ -129,8 +129,9 @@ static u32 __accumulate_pelt_segments(u64 periods, u32 d1, u32 d3) * Step 2 */ delta %= 1024; - contrib = __accumulate_pelt_segments(periods, - 1024 - sa->period_contrib, delta); + if (load) + contrib = __accumulate_pelt_segments(periods, + 1024 - sa->period_contrib, delta); } sa->period_contrib = delta; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] schied/fair: Skip updating "contrib" without load 2019-12-06 16:14 [PATCH] schied/fair: Skip updating "contrib" without load Peng Wang @ 2019-12-09 16:16 ` Peter Zijlstra 2019-12-11 12:16 ` Peng Wang 2019-12-13 3:45 ` [PATCH v2] schied/fair: Skip calculating @contrib " Peng Wang 1 sibling, 1 reply; 7+ messages in thread From: Peter Zijlstra @ 2019-12-09 16:16 UTC (permalink / raw) To: Peng Wang Cc: mingo, juri.lelli, vincent.guittot, dietmar.eggemann, rostedt, bsegall, mgorman, linux-kernel On Sat, Dec 07, 2019 at 12:14:22AM +0800, Peng Wang wrote: > We only update load_sum/runnable_load_sum/util_sum with > decayed old sum when load is clear. What you're saying is that because of the: if (!load) runnable = running = 0; clause in ___update_load_sum(), all the actual users of @contrib in accumulate_sum(): if (load) sa->load_sum += load * contrib; if (runnable) sa->runnable_load_sum += runnable * contrib; if (running) sa->util_sum += contrib << SCHED_CAPACITY_SHIFT; don't happen, and therefore we don't care what @contrib actually is and calculating it is pointless. I suppose that is so. did you happen to have performance numbers? Also, I'm thinking this wants a comment. > Signed-off-by: Peng Wang <rocking@linux.alibaba.com> > --- > kernel/sched/pelt.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/kernel/sched/pelt.c b/kernel/sched/pelt.c > index a96db50..4392953 100644 > --- a/kernel/sched/pelt.c > +++ b/kernel/sched/pelt.c > @@ -129,8 +129,9 @@ static u32 __accumulate_pelt_segments(u64 periods, u32 d1, u32 d3) > * Step 2 > */ > delta %= 1024; > - contrib = __accumulate_pelt_segments(periods, > - 1024 - sa->period_contrib, delta); > + if (load) > + contrib = __accumulate_pelt_segments(periods, > + 1024 - sa->period_contrib, delta); > } > sa->period_contrib = delta; > > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] schied/fair: Skip updating "contrib" without load 2019-12-09 16:16 ` Peter Zijlstra @ 2019-12-11 12:16 ` Peng Wang 0 siblings, 0 replies; 7+ messages in thread From: Peng Wang @ 2019-12-11 12:16 UTC (permalink / raw) To: Peter Zijlstra Cc: mingo, juri.lelli, vincent.guittot, dietmar.eggemann, rostedt, bsegall, mgorman, linux-kernel On Mon, Dec 09, 2019 at 05:16:27PM +0100, Peter Zijlstra wrote: > On Sat, Dec 07, 2019 at 12:14:22AM +0800, Peng Wang wrote: > > We only update load_sum/runnable_load_sum/util_sum with > > decayed old sum when load is clear. > > What you're saying is that because of the: > > if (!load) > runnable = running = 0; > > clause in ___update_load_sum(), all the actual users of @contrib in > accumulate_sum(): > > if (load) > sa->load_sum += load * contrib; > if (runnable) > sa->runnable_load_sum += runnable * contrib; > if (running) > sa->util_sum += contrib << SCHED_CAPACITY_SHIFT; > > don't happen, and therefore we don't care what @contrib actually is and > calculating it is pointless. Yes. > > I suppose that is so. did you happen to have performance numbers? Also, > I'm thinking this wants a comment. Actually I don't know how to get the exact performance data. But I count the times when @load equals zero and not as below: if (load) { load_is_not_zero_count++; contrib = __accumulate_pelt_segments(periods, 1024 - sa->period_contrib, delta); } else load_is_zero_count++; As we can see, load_is_zero_count is much bigger than load_is_zero_count, and the gap is gradually widening. load_is_zero_count: 6016044 times load_is_not_zero_count: 244316 times 19:50:43 up 1 min, 1 user, load average: 0.09, 0.06, 0.02 load_is_zero_count: 7956168 times load_is_not_zero_count: 261472 times 19:51:42 up 2 min, 1 user, load average: 0.03, 0.05, 0.01 load_is_zero_count: 10199896 times load_is_not_zero_count: 278364 times 19:52:51 up 3 min, 1 user, load average: 0.06, 0.05, 0.01 load_is_zero_count: 14333700 times load_is_not_zero_count: 318424 times 19:54:53 up 5 min, 1 user, load average: 0.01, 0.03, 0.00 Perhaps we can gain some performance advantage by saving these unnecessary calculation. > > > Signed-off-by: Peng Wang <rocking@linux.alibaba.com> > > --- > > kernel/sched/pelt.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/sched/pelt.c b/kernel/sched/pelt.c > > index a96db50..4392953 100644 > > --- a/kernel/sched/pelt.c > > +++ b/kernel/sched/pelt.c > > @@ -129,8 +129,9 @@ static u32 __accumulate_pelt_segments(u64 periods, u32 d1, u32 d3) > > * Step 2 > > */ > > delta %= 1024; > > - contrib = __accumulate_pelt_segments(periods, > > - 1024 - sa->period_contrib, delta); > > + if (load) > > + contrib = __accumulate_pelt_segments(periods, > > + 1024 - sa->period_contrib, delta); > > } > > sa->period_contrib = delta; > > > > -- > > 1.8.3.1 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] schied/fair: Skip calculating @contrib without load 2019-12-06 16:14 [PATCH] schied/fair: Skip updating "contrib" without load Peng Wang 2019-12-09 16:16 ` Peter Zijlstra @ 2019-12-13 3:45 ` Peng Wang 2019-12-13 9:21 ` Vincent Guittot ` (2 more replies) 1 sibling, 3 replies; 7+ messages in thread From: Peng Wang @ 2019-12-13 3:45 UTC (permalink / raw) To: mingo, peterz, juri.lelli, vincent.guittot, dietmar.eggemann, rostedt, bsegall, mgorman Cc: linux-kernel, Peng Wang Because of the: if (!load) runnable = running = 0; clause in ___update_load_sum(), all the actual users of @contrib in accumulate_sum(): if (load) sa->load_sum += load * contrib; if (runnable) sa->runnable_load_sum += runnable * contrib; if (running) sa->util_sum += contrib << SCHED_CAPACITY_SHIFT; don't happen, and therefore we don't care what @contrib actually is and calculating it is pointless. If we count the times when @load equals zero and not as below: if (load) { load_is_not_zero_count++; contrib = __accumulate_pelt_segments(periods, 1024 - sa->period_contrib,delta); } else load_is_zero_count++; As we can see, load_is_zero_count is much bigger than load_is_zero_count, and the gap is gradually widening: load_is_zero_count: 6016044 times load_is_not_zero_count: 244316 times 19:50:43 up 1 min, 1 user, load average: 0.09, 0.06, 0.02 load_is_zero_count: 7956168 times load_is_not_zero_count: 261472 times 19:51:42 up 2 min, 1 user, load average: 0.03, 0.05, 0.01 load_is_zero_count: 10199896 times load_is_not_zero_count: 278364 times 19:52:51 up 3 min, 1 user, load average: 0.06, 0.05, 0.01 load_is_zero_count: 14333700 times load_is_not_zero_count: 318424 times 19:54:53 up 5 min, 1 user, load average: 0.01, 0.03, 0.00 Perhaps we can gain some performance advantage by saving these unnecessary calculation. Signed-off-by: Peng Wang <rocking@linux.alibaba.com> --- kernel/sched/pelt.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/kernel/sched/pelt.c b/kernel/sched/pelt.c index a96db50..4392953 100644 --- a/kernel/sched/pelt.c +++ b/kernel/sched/pelt.c @@ -129,8 +129,9 @@ static u32 __accumulate_pelt_segments(u64 periods, u32 d1, u32 d3) * Step 2 */ delta %= 1024; - contrib = __accumulate_pelt_segments(periods, - 1024 - sa->period_contrib, delta); + if (load) + contrib = __accumulate_pelt_segments(periods, + 1024 - sa->period_contrib, delta); } sa->period_contrib = delta; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] schied/fair: Skip calculating @contrib without load 2019-12-13 3:45 ` [PATCH v2] schied/fair: Skip calculating @contrib " Peng Wang @ 2019-12-13 9:21 ` Vincent Guittot 2019-12-13 12:13 ` Peter Zijlstra 2019-12-17 12:39 ` [tip: sched/core] " tip-bot2 for Peng Wang 2 siblings, 0 replies; 7+ messages in thread From: Vincent Guittot @ 2019-12-13 9:21 UTC (permalink / raw) To: Peng Wang Cc: Ingo Molnar, Peter Zijlstra, Juri Lelli, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, linux-kernel Hi Peng, minor typo on the subject s/schied/sched/ On Fri, 13 Dec 2019 at 04:46, Peng Wang <rocking@linux.alibaba.com> wrote: > > Because of the: > > if (!load) > runnable = running = 0; > > clause in ___update_load_sum(), all the actual users of @contrib in > accumulate_sum(): > > if (load) > sa->load_sum += load * contrib; > if (runnable) > sa->runnable_load_sum += runnable * contrib; > if (running) > sa->util_sum += contrib << SCHED_CAPACITY_SHIFT; > > don't happen, and therefore we don't care what @contrib actually is and > calculating it is pointless. > > If we count the times when @load equals zero and not as below: > > if (load) { > load_is_not_zero_count++; > contrib = __accumulate_pelt_segments(periods, > 1024 - sa->period_contrib,delta); > } else > load_is_zero_count++; > > As we can see, load_is_zero_count is much bigger than > load_is_zero_count, and the gap is gradually widening: > > load_is_zero_count: 6016044 times > load_is_not_zero_count: 244316 times > 19:50:43 up 1 min, 1 user, load average: 0.09, 0.06, 0.02 > > load_is_zero_count: 7956168 times > load_is_not_zero_count: 261472 times > 19:51:42 up 2 min, 1 user, load average: 0.03, 0.05, 0.01 > > load_is_zero_count: 10199896 times > load_is_not_zero_count: 278364 times > 19:52:51 up 3 min, 1 user, load average: 0.06, 0.05, 0.01 > > load_is_zero_count: 14333700 times > load_is_not_zero_count: 318424 times > 19:54:53 up 5 min, 1 user, load average: 0.01, 0.03, 0.00 your system looks pretty idle so i'm not sure to see a benefit of your patch in such situation > > Perhaps we can gain some performance advantage by saving these > unnecessary calculation. load == 0 when - system is idle and we updates blocked load but we don't really care about performance in this case and we will stop the trigger the update once the load_avg reach null value - a rt/dl/cfs rq or a sched_entity wakes up. In this case, skipping the calculation of the contribution should fasten the wake up path although i'm not sure about the amount Nevertheless, this change makes sense Reviewed-by: Vincent Guittot < vincent.guittot@linaro.org> > > Signed-off-by: Peng Wang <rocking@linux.alibaba.com> > --- > kernel/sched/pelt.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/kernel/sched/pelt.c b/kernel/sched/pelt.c > index a96db50..4392953 100644 > --- a/kernel/sched/pelt.c > +++ b/kernel/sched/pelt.c > @@ -129,8 +129,9 @@ static u32 __accumulate_pelt_segments(u64 periods, u32 d1, u32 d3) > * Step 2 > */ > delta %= 1024; > - contrib = __accumulate_pelt_segments(periods, > - 1024 - sa->period_contrib, delta); > + if (load) > + contrib = __accumulate_pelt_segments(periods, > + 1024 - sa->period_contrib, delta); > } > sa->period_contrib = delta; > > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] schied/fair: Skip calculating @contrib without load 2019-12-13 3:45 ` [PATCH v2] schied/fair: Skip calculating @contrib " Peng Wang 2019-12-13 9:21 ` Vincent Guittot @ 2019-12-13 12:13 ` Peter Zijlstra 2019-12-17 12:39 ` [tip: sched/core] " tip-bot2 for Peng Wang 2 siblings, 0 replies; 7+ messages in thread From: Peter Zijlstra @ 2019-12-13 12:13 UTC (permalink / raw) To: Peng Wang Cc: mingo, juri.lelli, vincent.guittot, dietmar.eggemann, rostedt, bsegall, mgorman, linux-kernel On Fri, Dec 13, 2019 at 11:45:40AM +0800, Peng Wang wrote: > diff --git a/kernel/sched/pelt.c b/kernel/sched/pelt.c > index a96db50..4392953 100644 > --- a/kernel/sched/pelt.c > +++ b/kernel/sched/pelt.c > @@ -129,8 +129,9 @@ static u32 __accumulate_pelt_segments(u64 periods, u32 d1, u32 d3) > * Step 2 > */ > delta %= 1024; > - contrib = __accumulate_pelt_segments(periods, > - 1024 - sa->period_contrib, delta); > + if (load) > + contrib = __accumulate_pelt_segments(periods, > + 1024 - sa->period_contrib, delta); > } > sa->period_contrib = delta; I've made that: --- a/kernel/sched/pelt.c +++ b/kernel/sched/pelt.c @@ -129,8 +129,20 @@ accumulate_sum(u64 delta, struct sched_a * Step 2 */ delta %= 1024; - contrib = __accumulate_pelt_segments(periods, - 1024 - sa->period_contrib, delta); + if (load) { + /* + * This relies on the: + * + * if (!load) + * runnable = running = 0; + * + * clause from ___update_load_sum(); this results in + * the below usage of @contrib to dissapear entirely, + * so no point in calculating it. + */ + contrib = __accumulate_pelt_segments(periods, + 1024 - sa->period_contrib, delta); + } } sa->period_contrib = delta; @@ -205,7 +217,9 @@ ___update_load_sum(u64 now, struct sched * This means that weight will be 0 but not running for a sched_entity * but also for a cfs_rq if the latter becomes idle. As an example, * this happens during idle_balance() which calls - * update_blocked_averages() + * update_blocked_averages(). + * + * Also see the comment in accumulate_sum(). */ if (!load) runnable = running = 0; ^ permalink raw reply [flat|nested] 7+ messages in thread
* [tip: sched/core] schied/fair: Skip calculating @contrib without load 2019-12-13 3:45 ` [PATCH v2] schied/fair: Skip calculating @contrib " Peng Wang 2019-12-13 9:21 ` Vincent Guittot 2019-12-13 12:13 ` Peter Zijlstra @ 2019-12-17 12:39 ` tip-bot2 for Peng Wang 2 siblings, 0 replies; 7+ messages in thread From: tip-bot2 for Peng Wang @ 2019-12-17 12:39 UTC (permalink / raw) To: linux-tip-commits Cc: Peng Wang, Peter Zijlstra (Intel), Vincent Guittot, x86, LKML The following commit has been merged into the sched/core branch of tip: Commit-ID: d040e0734fb3dedfe24c3d94f5a32b4812eca610 Gitweb: https://git.kernel.org/tip/d040e0734fb3dedfe24c3d94f5a32b4812eca610 Author: Peng Wang <rocking@linux.alibaba.com> AuthorDate: Fri, 13 Dec 2019 11:45:40 +08:00 Committer: Peter Zijlstra <peterz@infradead.org> CommitterDate: Tue, 17 Dec 2019 13:32:51 +01:00 schied/fair: Skip calculating @contrib without load Because of the: if (!load) runnable = running = 0; clause in ___update_load_sum(), all the actual users of @contrib in accumulate_sum(): if (load) sa->load_sum += load * contrib; if (runnable) sa->runnable_load_sum += runnable * contrib; if (running) sa->util_sum += contrib << SCHED_CAPACITY_SHIFT; don't happen, and therefore we don't care what @contrib actually is and calculating it is pointless. If we count the times when @load equals zero and not as below: if (load) { load_is_not_zero_count++; contrib = __accumulate_pelt_segments(periods, 1024 - sa->period_contrib,delta); } else load_is_zero_count++; As we can see, load_is_zero_count is much bigger than load_is_zero_count, and the gap is gradually widening: load_is_zero_count: 6016044 times load_is_not_zero_count: 244316 times 19:50:43 up 1 min, 1 user, load average: 0.09, 0.06, 0.02 load_is_zero_count: 7956168 times load_is_not_zero_count: 261472 times 19:51:42 up 2 min, 1 user, load average: 0.03, 0.05, 0.01 load_is_zero_count: 10199896 times load_is_not_zero_count: 278364 times 19:52:51 up 3 min, 1 user, load average: 0.06, 0.05, 0.01 load_is_zero_count: 14333700 times load_is_not_zero_count: 318424 times 19:54:53 up 5 min, 1 user, load average: 0.01, 0.03, 0.00 Perhaps we can gain some performance advantage by saving these unnecessary calculation. Signed-off-by: Peng Wang <rocking@linux.alibaba.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Reviewed-by: Vincent Guittot < vincent.guittot@linaro.org> Link: https://lkml.kernel.org/r/1576208740-35609-1-git-send-email-rocking@linux.alibaba.com --- kernel/sched/pelt.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/kernel/sched/pelt.c b/kernel/sched/pelt.c index a96db50..bd006b7 100644 --- a/kernel/sched/pelt.c +++ b/kernel/sched/pelt.c @@ -129,8 +129,20 @@ accumulate_sum(u64 delta, struct sched_avg *sa, * Step 2 */ delta %= 1024; - contrib = __accumulate_pelt_segments(periods, - 1024 - sa->period_contrib, delta); + if (load) { + /* + * This relies on the: + * + * if (!load) + * runnable = running = 0; + * + * clause from ___update_load_sum(); this results in + * the below usage of @contrib to dissapear entirely, + * so no point in calculating it. + */ + contrib = __accumulate_pelt_segments(periods, + 1024 - sa->period_contrib, delta); + } } sa->period_contrib = delta; @@ -205,7 +217,9 @@ ___update_load_sum(u64 now, struct sched_avg *sa, * This means that weight will be 0 but not running for a sched_entity * but also for a cfs_rq if the latter becomes idle. As an example, * this happens during idle_balance() which calls - * update_blocked_averages() + * update_blocked_averages(). + * + * Also see the comment in accumulate_sum(). */ if (!load) runnable = running = 0; ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-12-17 12:40 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-06 16:14 [PATCH] schied/fair: Skip updating "contrib" without load Peng Wang 2019-12-09 16:16 ` Peter Zijlstra 2019-12-11 12:16 ` Peng Wang 2019-12-13 3:45 ` [PATCH v2] schied/fair: Skip calculating @contrib " Peng Wang 2019-12-13 9:21 ` Vincent Guittot 2019-12-13 12:13 ` Peter Zijlstra 2019-12-17 12:39 ` [tip: sched/core] " tip-bot2 for Peng Wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).