Re: [PATCH v6 3/3] sched/fair: Combine EAS check with overutilized access

[Shrikanth Hegde]

On 3/26/24 1:56 PM, Vincent Guittot wrote:
> On Tue, 26 Mar 2024 at 08:58, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>>
>>
>> * Shrikanth Hegde <sshegde@xxxxxxxxxxxxx> wrote:
>>
>>>  /*
>>> - * Ensure that caller can do EAS. overutilized value
>>> - * make sense only if EAS is enabled
>>> + * overutilized value make sense only if EAS is enabled
>>>   */
>>> -static inline int is_rd_overutilized(struct root_domain *rd)
>>> +static inline int is_rd_not_overutilized(struct root_domain *rd)
>>>  {
>>> -     return READ_ONCE(rd->overutilized);
>>> +     return sched_energy_enabled() && !READ_ONCE(rd->overutilized);
>>>  }
>>
>> While adding the sched_energy_enabled() condition looks OK, the _not prefix
>> This is silly: putting logical operators into functions names is far less
>> readable than a !fn()...
>>
>>> -     if (!is_rd_overutilized(rq->rd) && cpu_overutilized(rq->cpu))
>>> +     if (is_rd_not_overutilized(rq->rd) && cpu_overutilized(rq->cpu))
>>
>> Especially since we already have cpu_overutilized(). It's far more coherent
>> to have the same basic attribute functions and put any negation into
>> *actual* logical operators.
> 
> I was concerned by the || in this case that could defeat the purpose
> of sched_energy_enabled() but it will return early anyway
> 

> return !sched_energy_enabled() || READ_ONCE(rd->overutilized);

I think this would work. 

> 
>>
>> Thanks,
>>
>>         Ingo


If EAS - false, then is_rd_overutilized -> would be true always and all users of it do !is_rd_overutilized(). so No operation.
If EAS - true, it reads rd->overutilized value. 

Does this look correct?  
-------------------------------------------------------------------------------------