Re: [PATCH] sched/fair: Avoid overflow in place_entity()

[Hui Su]

> Indeed so; however I've rewritten things like below. Hope that works for
> you.
>
> ---
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 7455a83a6a99..6ce7233cd339 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6266,12 +6266,20 @@ place_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
> 		load = cfs_rq->sum_weight;
> 		if (curr && curr->on_rq)
> 			load += avg_vruntime_weight(cfs_rq, curr->h_load.weight);
> -
> -		weight = avg_vruntime_weight(cfs_rq, se->h_load.weight);
> -		lag *= load + weight;
> 		if (WARN_ON_ONCE(!load))
> 			load = 1;
> -		lag = div64_long(lag, load);
> +
> +		weight = avg_vruntime_weight(cfs_rq, se->h_load.weight);
> +
> +		/*
> +		 * Rewrite, because the product (W + w_i)*vl'_i might
> +		 * overflow s64:
> +		 *
> +		 *   vl_i = (W + w_i)*vl'_i / W
> +		 *        = (W*vl'_i + w_i*vl'_i) / W
> +		 *        = vl'_i + w_i*vl'_i/W
> +		 */
> +		lag += div64_long(lag * weight, load);
> 
> 		/*
> 		 * A heavy entity (relative to the tree) will pull the

Yes, this looks good to me for the normal path. One minor difference is the
`WARN_ON_ONCE(!load)` fallback: the old code produces `lag * weight`, while
this rewrite produces `lag * (weight + 1)` after setting `load` to one.
Since that should be an invariant-violation path, I don't think it matters
in practice.

Thanks for the rewrite.

Hui