Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1631593 > unrolled thread

Re: [PATCH 1/2] sched/fair: Fix how load gets propagated from cfs_rq to its sched_entity

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] sched/fair: Fix how load gets propagated from cfs_rq  to its sched_entity Vincent Guittot <vincent.guittot@linaro.org> - 2017-04-26 18:20 +0200
    Re: [PATCH 1/2] sched/fair: Fix how load gets propagated from cfs_rq  to its sched_entity Tejun Heo <tj@kernel.org> - 2017-04-27 00:30 +0200
      Re: [PATCH 1/2] sched/fair: Fix how load gets propagated from cfs_rq  to its sched_entity Vincent Guittot <vincent.guittot@linaro.org> - 2017-04-27 11:00 +0200
        Re: [PATCH 1/2] sched/fair: Fix how load gets propagated from cfs_rq  to its sched_entity Tejun Heo <tj@kernel.org> - 2017-04-28 19:50 +0200
          Re: [PATCH 1/2] sched/fair: Fix how load gets propagated from cfs_rq  to its sched_entity Vincent Guittot <vincent.guittot@linaro.org> - 2017-05-02 09:30 +0200

#1631593 — Re: [PATCH 1/2] sched/fair: Fix how load gets propagated from cfs_rq to its sched_entity

On 24 April 2017 at 22:14, Tejun Heo <tj@kernel.org> wrote:
> 09a43ace1f98 ("sched/fair: Propagate load during synchronous
> attach/detach") added immediate load propagation from cfs_rq to its
> sched_entity then to the parent cfs_rq; however, what gets propagated
> doesn't seem to make sense.
>
> It repeats the tg_weight calculation done in calc_cfs_shares() but
> only uses it to compensate for shares being out of date.  After that,
> it sets the sched_entity's load_avg to the load_avg of the
> corresponding cfs_rq.
>
> This doesn't make sense as the cfs_rq's load_avg is some fraction of
> its total weight, which the sched_entity's weight has nothing to with.
> For example, if the cfs_rq has a single constant load 1 task the
> cfs_rq's load_avg would be around 1.  If that cfs_rq is the only
> active sched_entity in the parent cfs_rq which has the maximum weight,
> the sched_entity's load should be around the maximum weight but
> update_tg_cfs_load() ends up overriding it to 1.
>
> At the parent's level, the absolute value of load_avg inside a child
> cfs_rq doesn't mean anything.  Only the ratio against its weight is
> meaningful.
>
> This patch changes update_tg_cfs_load() to normalize the
> runnable_load_avg of the cfs_rq and then scale it to the matching
> sched_entity's freshly calculated shares for propagation.  Use of
> runnable_load_avg instead of load_avg is intentional and keeps the
> parent's runnable_load_avg true to the sum of scaled loads of all
> tasks queued under it which is critical for the correction operation
> of load balancer.  The next patch will depend on it.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Vincent Guittot <vincent.guittot@linaro.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Paul Turner <pjt@google.com>
> ---
>  kernel/sched/fair.c |   46 +++++++++++++++++++---------------------------
>  1 file changed, 19 insertions(+), 27 deletions(-)
>
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -3078,37 +3078,29 @@ static inline void
>  update_tg_cfs_load(struct cfs_rq *cfs_rq, struct sched_entity *se)
>  {
>         struct cfs_rq *gcfs_rq = group_cfs_rq(se);
> -       long delta, load = gcfs_rq->avg.load_avg;
> +       long load = 0, delta;
>
>         /*
> -        * If the load of group cfs_rq is null, the load of the
> -        * sched_entity will also be null so we can skip the formula
> +        * A cfs_rq's load avg contribution to the parent should be scaled
> +        * to the sched_entity's weight.  Use freshly calculated shares
> +        * instead of @se->load.weight as the latter may not reflect
> +        * changes from the current scheduling operation.
> +        *
> +        * Note that the propagation source is runnable_load_avg instead of
> +        * load_avg.  This keeps every cfs_rq's runnable_load_avg true to
> +        * the sum of the scaled loads of all tasks queued under it, which
> +        * is important for the correct operation of the load balancer.
> +        *
> +        * This can make the sched_entity's load_avg jumpier but that
> +        * correctly reflects what would happen without cgroups if each
> +        * task's load is scaled across nesting - the load is being
> +        * averaged at the task and each cfs_rq.
>          */
> -       if (load) {
> -               long tg_load;
> +       if (gcfs_rq->load.weight) {
> +               long shares = calc_cfs_shares(gcfs_rq, gcfs_rq->tg);
>
> -               /* Get tg's load and ensure tg_load > 0 */
> -               tg_load = atomic_long_read(&gcfs_rq->tg->load_avg) + 1;
> -
> -               /* Ensure tg_load >= load and updated with current load*/
> -               tg_load -= gcfs_rq->tg_load_avg_contrib;
> -               tg_load += load;
> -
> -               /*
> -                * We need to compute a correction term in the case that the
> -                * task group is consuming more CPU than a task of equal
> -                * weight. A task with a weight equals to tg->shares will have
> -                * a load less or equal to scale_load_down(tg->shares).
> -                * Similarly, the sched_entities that represent the task group
> -                * at parent level, can't have a load higher than
> -                * scale_load_down(tg->shares). And the Sum of sched_entities'
> -                * load must be <= scale_load_down(tg->shares).
> -                */
> -               if (tg_load > scale_load_down(gcfs_rq->tg->shares)) {
> -                       /* scale gcfs_rq's load into tg's shares*/
> -                       load *= scale_load_down(gcfs_rq->tg->shares);
> -                       load /= tg_load;
> -               }
> +               load = min(gcfs_rq->runnable_load_avg *
> +                          shares / gcfs_rq->load.weight, shares);

There is a unit problem above:
runnable_load_avg and shares are not in the same range but
runnable_load_avg and  scale_load_down(gcfs_rq->load.weight) are so
you should use
gcfs_rq->runnable_load_avg * scale_load_down(shares) /
scale_load_down(gcfs_rq->load.weight).
Hopefully both  scale_load_down cancel between them
But the min should be then tested with scale_load_down(shares) and not
only shares


>         }
>
>         delta = load - se->avg.load_avg;

[toc] | [next] | [standalone]

#1631767

Hello, Vincent.

On Wed, Apr 26, 2017 at 06:14:17PM +0200, Vincent Guittot wrote:
> > +       if (gcfs_rq->load.weight) {
> > +               long shares = calc_cfs_shares(gcfs_rq, gcfs_rq->tg);
> >
> > +               load = min(gcfs_rq->runnable_load_avg *
> > +                          shares / gcfs_rq->load.weight, shares);
> 
> There is a unit problem above:
> runnable_load_avg and shares are not in the same range but
> runnable_load_avg and  scale_load_down(gcfs_rq->load.weight) are so
> you should use
> gcfs_rq->runnable_load_avg * scale_load_down(shares) /
> scale_load_down(gcfs_rq->load.weight).

But the only difference there is that we lose accuracy in calculation;
otherwise, the end results are the same, no?

> Hopefully both  scale_load_down cancel between them
> But the min should be then tested with scale_load_down(shares) and not
> only shares

Ah, that's right.  The min should be against scaled down shares.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1631977

On 27 April 2017 at 00:27, Tejun Heo <tj@kernel.org> wrote:
> Hello, Vincent.
>
> On Wed, Apr 26, 2017 at 06:14:17PM +0200, Vincent Guittot wrote:
>> > +       if (gcfs_rq->load.weight) {
>> > +               long shares = calc_cfs_shares(gcfs_rq, gcfs_rq->tg);
>> >
>> > +               load = min(gcfs_rq->runnable_load_avg *
>> > +                          shares / gcfs_rq->load.weight, shares);
>>
>> There is a unit problem above:
>> runnable_load_avg and shares are not in the same range but
>> runnable_load_avg and  scale_load_down(gcfs_rq->load.weight) are so
>> you should use
>> gcfs_rq->runnable_load_avg * scale_load_down(shares) /
>> scale_load_down(gcfs_rq->load.weight).
>
> But the only difference there is that we lose accuracy in calculation;
> otherwise, the end results are the same, no?

Yes the end result is the same, it was mainly to point out the range
difference and explain why we need scale_load_down(shares) for the 2nd
argument of min.
This should also explain the warning issue you mentioned earlier

>
>> Hopefully both  scale_load_down cancel between them
>> But the min should be then tested with scale_load_down(shares) and not
>> only shares
>
> Ah, that's right.  The min should be against scaled down shares.
>
> Thanks.
>
> --
> tejun

[toc] | [prev] | [next] | [standalone]

#1633054

Hello, Vincent.

On Thu, Apr 27, 2017 at 10:59:12AM +0200, Vincent Guittot wrote:
> > But the only difference there is that we lose accuracy in calculation;
> > otherwise, the end results are the same, no?
> 
> Yes the end result is the same, it was mainly to point out the range
> difference and explain why we need scale_load_down(shares) for the 2nd
> argument of min.
> This should also explain the warning issue you mentioned earlier

I'm not sure this makes sense.  Practically, we're doing more shifts
just to lose calculation accuracy.  Even conceptually, what we're
doing is

             C
	A * ---
             B

Where A is in a different scale while B and C are in the same.  What
you're suggesting is

             scale_down(C)
	A * ---------------
	     scale_down(B)

I can't see why this is better in any way.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1634187

On 28 April 2017 at 19:46, Tejun Heo <tj@kernel.org> wrote:
> Hello, Vincent.
>
> On Thu, Apr 27, 2017 at 10:59:12AM +0200, Vincent Guittot wrote:
>> > But the only difference there is that we lose accuracy in calculation;
>> > otherwise, the end results are the same, no?
>>
>> Yes the end result is the same, it was mainly to point out the range
>> difference and explain why we need scale_load_down(shares) for the 2nd
>> argument of min.
>> This should also explain the warning issue you mentioned earlier
>
> I'm not sure this makes sense.  Practically, we're doing more shifts
> just to lose calculation accuracy.  Even conceptually, what we're
> doing is
>
>              C
>         A * ---
>              B
>
> Where A is in a different scale while B and C are in the same.  What
> you're suggesting is
>
>              scale_down(C)
>         A * ---------------
>              scale_down(B)
>
> I can't see why this is better in any way.

i'm not saying it's netter, i'm saying that it might be the cause of
the build warning you mentioned because you are mixing  u64 and
unsigned long

>
> Thanks.
>
> --
> tejun

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web