Received-SPF: pass (google.com: domain of linux-kernel+bounces-151159-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 19 Apr 2024 10:24:40 +0200
From: Peter Zijlstra <peterz@infradead.org>
To: Chen Yu <yu.c.chen@intel.com>
Cc: Xuewen Yan <xuewen.yan94@gmail.com>, Abel Wu <wuyun.abel@bytedance.com>,
	Ingo Molnar <mingo@redhat.com>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Juri Lelli <juri.lelli@redhat.com>, Tim Chen <tim.c.chen@intel.com>,
	Tiwei Bie <tiwei.btw@antgroup.com>,
	Honglei Wang <wanghonglei@didichuxing.com>,
	Aaron Lu <aaron.lu@intel.com>, Chen Yu <yu.chen.surf@gmail.com>,
	Yujie Liu <yujie.liu@intel.com>, linux-kernel@vger.kernel.org,
	kernel test robot <oliver.sang@intel.com>,
	Tianchen Ding <dtcccc@linux.alibaba.com>
Subject: Re: [RFC PATCH] sched/eevdf: Return leftmost entity in pick_eevdf()
 if no eligible entity is found
Message-ID: <20240419082440.GB6345@noisy.programming.kicks-ass.net>
References: <20240226082349.302363-1-yu.c.chen@intel.com>
 <758ebf4e-ee84-414b-99ec-182537bcc168@bytedance.com>
 <20240408115833.GF21904@noisy.programming.kicks-ass.net>
 <ZhPtCyRmPxa0DpMe@chenyu5-mobl2>
 <20240409092104.GA2665@noisy.programming.kicks-ass.net>
 <ZiAWTU5xb/JMn/Hs@chenyu5-mobl2>
 <CAB8ipk-fejQ41Jgk6z52+T6CP+impwbaOAfhA9vG_-FB9BeRyw@mail.gmail.com>
 <ZiEaKOQwiNEglYtS@chenyu5-mobl2>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZiEaKOQwiNEglYtS@chenyu5-mobl2>

On Thu, Apr 18, 2024 at 09:03:36PM +0800, Chen Yu wrote:

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 31bca05c3612..9f203012e8f5 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -696,15 +696,23 @@ u64 avg_vruntime(struct cfs_rq *cfs_rq)
>   *
>   * XXX could add max_slice to the augmented data to track this.
>   */
> +
> +static s64 limit_entity_lag(struct sched_entity *se, s64 lag)
> +{
> +	s64 limit;
> +
> +	limit = calc_delta_fair(max_t(u64, 2*se->slice, TICK_NSEC), se);
> +	return clamp(lag, -limit, limit);
> +}

Right, helper makes sense.

> @@ -3721,6 +3729,7 @@ static void reweight_eevdf(struct cfs_rq *cfs_rq, struct sched_entity *se,
>  	if (avruntime != se->vruntime) {
>  		vlag = (s64)(avruntime - se->vruntime);
>  		vlag = div_s64(vlag * old_weight, weight);
> +		vlag = limit_entity_lag(se, vlag);
>  		se->vruntime = avruntime - vlag;

So the !on_rq case has clamping in update_entity_lag() which is before
scaling. And that makes more sense to me, because putting a limit on
vlag before the multiplication *should* ensure the multiplication itself
doesn't overflow.

But now you allow it to compute garbage and then clip the garbage.

>  	}
>  
> @@ -3768,6 +3777,9 @@ static void reweight_entity(struct cfs_rq *cfs_rq, struct sched_entity *se,
>  
>  	update_load_set(&se->load, weight);
>  
> +	if (!se->on_rq)
> +		se->vlag = limit_entity_lag(se, se->vlag);
> +

Except you now add clamping after scaling too, but in a really weird
place. Should this not go right after the div_s64() that scales?

Unlike the reweight_eevdf() case, there might be an argument for doing
it after scaling in this case. Namely, you can have multiple reweights
stacking their scale ops.


Also, could you put a few words in on how often these clips are hit? I
suspect it's fairly rare (but crucial when it does).