On Thu, Mar 30, 2017 at 03:46:58PM +0200, Peter Zijlstra wrote:
> On Thu, Mar 30, 2017 at 02:16:58PM +0200, Peter Zijlstra wrote:
> > On Thu, Mar 30, 2017 at 04:21:08AM -0700, Paul Turner wrote:
>
> > > - The naming here is really ambiguous:
> > > "__accumulate_sum" -> "__accumulate_pelt_segments"?
> >
> > OK, I did struggle with that a bit too but failed to improve, I'll change
> > it.
> >
> > > - Passing in "remainder" seems irrelevant to the sum accumulation. It
> > > would be
> > > more clear to handle it from the caller.
> >
> > Well, this way we have all 3 delta parts in one function. I'll try it
> > and see what it looks like though.
>
> > > This is super confusing. It only works because remainder already had
> > > period_contrib aggregated _into_ it. We're literally computing:
> > > remainder + period_contrib - period_contrib
> >
> > Correct; although I didn't find it too confusing. Could be because I'd
> > been staring at this code for a few hours though.
> >
> > > We should just not call this in the !periods case and handle the remainder
> > > below.
> >
> > I'll change it see what it looks like.
>
> How's this?
That is good. Only:
> ---
> kernel/sched/fair.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 76f67b3e34d6..10d34498b5fe 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -2795,12 +2795,9 @@ static u64 decay_load(u64 val, u64 n)
> return val;
> }
>
> -static u32 __accumulate_sum(u64 periods, u32 period_contrib, u32 remainder)
> +static u32 __accumulate_pelt_segments(u64 periods, u32 d1, u32 d3)
> {
> - u32 c1, c2, c3 = remainder; /* y^0 == 1 */
> -
> - if (!periods)
> - return remainder - period_contrib;
> + u32 c1, c2, c3 = d3; /* y^0 == 1 */
>
> if (unlikely(periods >= LOAD_AVG_MAX_N))
> return LOAD_AVG_MAX;
> @@ -2861,8 +2858,8 @@ accumulate_sum(u64 delta, int cpu, struct sched_avg *sa,
> unsigned long weight, int running, struct cfs_rq *cfs_rq)
> {
> unsigned long scale_freq, scale_cpu;
> + u32 contrib = delta;
u64 -> u32 may raise some question, so better cast it to show confidence
at the first.
