Received-SPF: pass (google.com: domain of linux-kernel+bounces-86118-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <f022d110-9125-4d00-bc13-3e0413520cb5@linux.ibm.com>
Date: Thu, 29 Feb 2024 10:00:22 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 1/2] sched/fair: Add EAS checks before updating
 overutilized
Content-Language: en-US
To: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: yu.c.chen@intel.com, linux-kernel@vger.kernel.org, nysal@linux.ibm.com,
        aboorvad@linux.ibm.com, srikar@linux.vnet.ibm.com, vschneid@redhat.com,
        morten.rasmussen@arm.com, qyousef@layalina.io, mingo@kernel.org,
        peterz@infradead.org, vincent.guittot@linaro.org,
        Pierre Gondois <pierre.gondois@arm.com>
References: <20240228071621.602596-1-sshegde@linux.ibm.com>
 <20240228071621.602596-2-sshegde@linux.ibm.com>
 <86f000a3-3068-4c57-858d-c00e3eb6e974@arm.com>
 <72c2d279-90ae-4612-9b96-e579333b8088@linux.ibm.com>
 <1146ab2f-56c7-43f3-b26b-d91d2bd08556@arm.com>
From: Shrikanth Hegde <sshegde@linux.ibm.com>
In-Reply-To: <1146ab2f-56c7-43f3-b26b-d91d2bd08556@arm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 2/29/24 5:04 AM, Dietmar Eggemann wrote:
> On 28/02/2024 18:24, Shrikanth Hegde wrote:
> 

Thank you Dietmar, for taking a look. 

> [...]
> 
>> But we will do some extra computation currently and then not use it if it 
>> Non-EAS case in update_sg_lb_stats
>>
>> Would something like this makes sense?
>> @@ -9925,7 +9925,7 @@ static inline void update_sg_lb_stats(struct lb_env *env,
>>                 if (nr_running > 1)
>>                         *sg_status |= SG_OVERLOAD;
>>  
>> -               if (cpu_overutilized(i))
>> +               if (sched_energy_enabled() && cpu_overutilized(i))
>>                         *sg_status |= SG_OVERUTILIZED;
> 
> Yes, we could also disable the setting of OU in load_balance in the none
> !EAS case.
> 
> [...]

Ok. I will add this change. I don't see any other place where we need to do EAS 
check w.r.t to overutilized. This should cover all cases then. 

> 
>>> NIT:
>>> When called from check_update_overutilized_status(),
>>> sched_energy_enabled() will be checked twice.
>> Yes. 
>> But, I think that's okay since it is a static branch check at best. 
>> This way it keeps the code simpler. 
> 
> You could keep the ched_energy_enabled() outside of the new
> set_overutilized_status() to avoid this:
> 
> -->8--

Ok. We can do this as well.  I will incorporate this and send out v3 soon. 


> 
> ---
>  kernel/sched/fair.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 32bc98d9123d..c82164bf45f3 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6676,12 +6676,19 @@ static inline bool cpu_overutilized(int cpu)
>  	return !util_fits_cpu(cpu_util_cfs(cpu), rq_util_min, rq_util_max, cpu);
>  }
>  
> +static inline void set_overutilized_status(struct rq *rq, unsigned int val)
> +{
> +	WRITE_ONCE(rq->rd->overutilized, val);
> +	trace_sched_overutilized_tp(rq->rd, val);
> +}
> +
>  static inline void update_overutilized_status(struct rq *rq)
>  {
> -	if (!READ_ONCE(rq->rd->overutilized) && cpu_overutilized(rq->cpu)) {
> -		WRITE_ONCE(rq->rd->overutilized, SG_OVERUTILIZED);
> -		trace_sched_overutilized_tp(rq->rd, SG_OVERUTILIZED);
> -	}
> +	if (!sched_energy_enabled())
> +		return;
> +
> +	if (!READ_ONCE(rq->rd->overutilized) && cpu_overutilized(rq->cpu))
> +		set_overutilized_status(rq, SG_OVERUTILIZED);
>  }
>  #else
>  static inline void update_overutilized_status(struct rq *rq) { }
> @@ -10755,19 +10762,16 @@ static inline void update_sd_lb_stats(struct lb_env *env, struct sd_lb_stats *sd
>  		env->fbq_type = fbq_classify_group(&sds->busiest_stat);
>  
>  	if (!env->sd->parent) {
> -		struct root_domain *rd = env->dst_rq->rd;
> -
>  		/* update overload indicator if we are at root domain */
> -		WRITE_ONCE(rd->overload, sg_status & SG_OVERLOAD);
> +		WRITE_ONCE(env->dst_rq->rd->overload, sg_status & SG_OVERLOAD);
>  
>  		/* Update over-utilization (tipping point, U >= 0) indicator */
> -		WRITE_ONCE(rd->overutilized, sg_status & SG_OVERUTILIZED);
> -		trace_sched_overutilized_tp(rd, sg_status & SG_OVERUTILIZED);
> -	} else if (sg_status & SG_OVERUTILIZED) {
> -		struct root_domain *rd = env->dst_rq->rd;
> -
> -		WRITE_ONCE(rd->overutilized, SG_OVERUTILIZED);
> -		trace_sched_overutilized_tp(rd, SG_OVERUTILIZED);
> +		if (sched_energy_enabled()) {
> +			set_overutilized_status(env->dst_rq,
> +						sg_status & SG_OVERUTILIZED);
> +		}
> +	} else if (sched_energy_enabled() && sg_status & SG_OVERUTILIZED) {
> +		set_overutilized_status(env->dst_rq, SG_OVERUTILIZED);
>  	}
>  
>  	update_idle_cpu_scan(env, sum_util);