Received-SPF: pass (google.com: domain of linux-kernel+bounces-119215-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <b1f784ca-0583-4f85-8f2d-5ceb2d035ae7@linux.ibm.com>
Date: Tue, 26 Mar 2024 20:19:12 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v6 3/3] sched/fair: Combine EAS check with overutilized
 access
To: Vincent Guittot <vincent.guittot@linaro.org>,
        Ingo Molnar <mingo@kernel.org>
Cc: peterz@infradead.org, yu.c.chen@intel.com, dietmar.eggemann@arm.com,
        linux-kernel@vger.kernel.org, nysal@linux.ibm.com,
        aboorvad@linux.ibm.com, srikar@linux.ibm.com, vschneid@redhat.com,
        pierre.gondois@arm.com, qyousef@layalina.io
References: <20240307085725.444486-1-sshegde@linux.ibm.com>
 <20240307085725.444486-4-sshegde@linux.ibm.com> <ZgKAJAiAM61MLPBN@gmail.com>
 <CAKfTPtBTUHv3xBe0U75_4GGYPLYj-NC1uSGd6QjdQyS5k1N0QA@mail.gmail.com>
 <3495922c-91cd-448f-8831-5ce8bab0eda4@linux.ibm.com>
 <CAKfTPtDPMShXQYv7ZtfZ6RAgve3Ogsn7aDAYFj7vJuTYe9BOfg@mail.gmail.com>
Content-Language: en-US
From: Shrikanth Hegde <sshegde@linux.ibm.com>
In-Reply-To: <CAKfTPtDPMShXQYv7ZtfZ6RAgve3Ogsn7aDAYFj7vJuTYe9BOfg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 3/26/24 7:42 PM, Vincent Guittot wrote:
> On Tue, 26 Mar 2024 at 13:26, Shrikanth Hegde <sshegde@linux.ibm.com> wrote:
>>
>>
>>
>>>>
>>>> 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?
> 
> yes looks good to me
> 

Thank you. Let me send it as a patch

>> -------------------------------------------------------------------------------------
>> From 3adc0d58f87d8a2e96196a0f47bcd0d2afd057ae Mon Sep 17 00:00:00 2001
>> From: Shrikanth Hegde <sshegde@linux.ibm.com>
>> Date: Wed, 6 Mar 2024 03:58:58 -0500
>> Subject: [PATCH v7 3/3] sched/fair: Combine EAS check with overutilized access
>>
>> Access to overutilized is always used with sched_energy_enabled in
>> the pattern:
>>
>> if (sched_energy_enabled && !overutilized)
>>        do something
>>
>> So modify the helper function to return this pattern. This is more
>> readable code as it would say, do something when root domain is not
>> overutilized. This function always return true when EAS is disabled.
>>
>> No change in functionality intended.
>>
>> Suggested-by: Vincent Guittot <vincent.guittot@linaro.org>
>> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
>> ---
>>  kernel/sched/fair.c | 20 +++++++-------------
>>  1 file changed, 7 insertions(+), 13 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 24a7530a7d3f..e222e3ad4cfe 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -6686,12 +6686,11 @@ static inline bool cpu_overutilized(int cpu)
>>  }
>>
>>  /*
>> - * 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)
>>  {
>> -       return READ_ONCE(rd->overutilized);
>> +       return !sched_energy_enabled() || READ_ONCE(rd->overutilized);
>>  }
>>
>>  static inline void set_rd_overutilized_status(struct root_domain *rd,
>> @@ -6710,8 +6709,6 @@ static inline void check_update_overutilized_status(struct rq *rq)
>>          * overutilized field is used for load balancing decisions only
>>          * if energy aware scheduler is being used
>>          */
>> -       if (!sched_energy_enabled())
>> -               return;
>>
>>         if (!is_rd_overutilized(rq->rd) && cpu_overutilized(rq->cpu))
>>                 set_rd_overutilized_status(rq->rd, SG_OVERUTILIZED);
>> @@ -7999,7 +7996,7 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
>>
>>         rcu_read_lock();
>>         pd = rcu_dereference(rd->pd);
>> -       if (!pd || is_rd_overutilized(rd))
>> +       if (!pd)
>>                 goto unlock;
>>
>>         /*
>> @@ -8202,7 +8199,7 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags)
>>                     cpumask_test_cpu(cpu, p->cpus_ptr))
>>                         return cpu;
>>
>> -               if (sched_energy_enabled()) {
>> +               if (!is_rd_overutilized(this_rq()->rd)) {
>>                         new_cpu = find_energy_efficient_cpu(p, prev_cpu);
>>                         if (new_cpu >= 0)
>>                                 return new_cpu;
>> @@ -10903,12 +10900,9 @@ static struct sched_group *sched_balance_find_src_group(struct lb_env *env)
>>         if (busiest->group_type == group_misfit_task)
>>                 goto force_balance;
>>
>> -       if (sched_energy_enabled()) {
>> -               struct root_domain *rd = env->dst_rq->rd;
>> -
>> -               if (rcu_dereference(rd->pd) && !is_rd_overutilized(rd))
>> -                       goto out_balanced;
>> -       }
>> +       if (!is_rd_overutilized(env->dst_rq->rd) &&
>> +           rcu_dereference(env->dst_rq->rd->pd))
>> +               goto out_balanced;
>>
>>         /* ASYM feature bypasses nice load balance check */
>>         if (busiest->group_type == group_asym_packing)
>> --
>> 2.39.3