Re: [PATCH v4 1/2] sched/fair: Add EAS checks before updating overutilized

[Shrikanth Hegde]

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@xxxxxxxxxxx>
> 
>>

Thank you. 

>>