Received-SPF: pass (google.com: domain of linux-kernel+bounces-92564-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <6ccd425f-7c02-4ede-9321-1e8154d5d470@linux.ibm.com>
Date: Tue, 5 Mar 2024 20:26:49 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 1/2] sched/fair: Add EAS checks before updating
 overutilized
Content-Language: en-US
To: Qais Yousef <qyousef@layalina.io>
Cc: dietmar.eggemann@arm.com, pierre.gondois@arm.com, mingo@kernel.org,
        peterz@infradead.org, vincent.guittot@linaro.org, yu.c.chen@intel.com,
        linux-kernel@vger.kernel.org, nysal@linux.ibm.com,
        aboorvad@linux.ibm.com, srikar@linux.ibm.com, vschneid@redhat.com,
        morten.rasmussen@arm.com
References: <20240301151725.874604-1-sshegde@linux.ibm.com>
 <20240301151725.874604-2-sshegde@linux.ibm.com>
 <20240303185059.wvjkrrn7liwl4wtv@airbuntu>
 <5c6259a2-fadd-41c6-aa41-91f3af01272c@linux.ibm.com>
 <20240305102318.6qfib44f2ciffomw@airbuntu>
From: Shrikanth Hegde <sshegde@linux.ibm.com>
In-Reply-To: <20240305102318.6qfib44f2ciffomw@airbuntu>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 3/5/24 3:53 PM, Qais Yousef wrote:
> On 03/04/24 13:54, Shrikanth Hegde wrote:
>>
>>
>> On 3/4/24 12:20 AM, Qais Yousef wrote:
>>> On 03/01/24 20:47, Shrikanth Hegde wrote:
>>>> Overutilized field of root domain is only used for EAS(energy aware scheduler)
>>
>> [...]
>>
>>
>> Hi Qais, Thanks for taking a look. 
>>
>>>> ---
>>>>  kernel/sched/fair.c | 49 +++++++++++++++++++++++++++------------------
>>>>  1 file changed, 30 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>>> index 6a16129f9a5c..a71f8a1506e4 100644
>>>> --- a/kernel/sched/fair.c
>>>> +++ b/kernel/sched/fair.c
>>>> @@ -6670,15 +6670,29 @@ 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 update_overutilized_status(struct rq *rq)
>>>> +static inline void set_rd_overutilized_status(struct root_domain *rd,
>>>> +					      unsigned int status)
>>>>  {
>>>> -	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);
>>>> -	}
>>>
>>> Can we add
>>>
>>> 	if (!sched_energy_enabled())
>>> 		return;
>>
>> This is very close to what i had till v2. But it was pointed out that, it 
>> would end up calling sched_energy_enabled twice in  check_update_overutilized_status. 
> 
> It's a static key. It will either patch the code to be a NOP and return, or
> work normally. I don't see a problem.


Yes. That's what i thought initially as well. It does make the code simpler. 
I will change it to use similar to what i had in v2 in next version. I will wait for a while 
to hear any issues with that. 


> 
>> In check_update_overutilized_status, it would be better to avoid access to 
>> overutilized and computing cpu_overutilized if EAS is not enabled. 
> 
> cpu_overutilized() could gain a protection with sched_energy_enabled() too.
> I think it's better to encapsulate the deps within the function.
> 

ok. let me try to incorporate that. 

>>
>> I am okay with either code. keeping sched_energy_enabled in set_rd_overutilized_status
>> would be less code and more readable. But would call sched_energy_enabled twice. 
>>
>> Dietmar, Pierre, 
>> Could you please provide your inputs here? 
> 
> I prefer not sprinkling sched_energy_enabled() for every user. But FWIW the
> code looks correct to me and these stylistic issues are not a blocker for me
> 
> Reviewed-by: Qais Yousef <qyousef@layalina.io>
> 
>>

Thank you. 

>>