Received-SPF: pass (google.com: domain of linux-kernel+bounces-92570-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <251f9b0d-3e00-4ca7-829b-e622fc0b4c49@linux.ibm.com>
Date: Tue, 5 Mar 2024 20:33:14 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 2/2] sched/fair: Use helper function to access
 rd->overutilized
Content-Language: en-US
To: Vincent Guittot <vincent.guittot@linaro.org>
Cc: mingo@kernel.org, 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, morten.rasmussen@arm.com,
        qyousef@layalina.io
References: <20240301151725.874604-1-sshegde@linux.ibm.com>
 <20240301151725.874604-3-sshegde@linux.ibm.com>
 <CAKfTPtCzHf_R4SwR29FsnxXTv2J4Xrmh3gfoHcVPu3KU5OLDcQ@mail.gmail.com>
From: Shrikanth Hegde <sshegde@linux.ibm.com>
In-Reply-To: <CAKfTPtCzHf_R4SwR29FsnxXTv2J4Xrmh3gfoHcVPu3KU5OLDcQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 3/5/24 7:35 PM, Vincent Guittot wrote:
> On Fri, 1 Mar 2024 at 16:18, Shrikanth Hegde <sshegde@linux.ibm.com> wrote:
>
> 
> It seems that is_rd_overutilized() is always used with
> sched_energy_enabled() in the pattern:
> 
> If (sched_energy_enabled() && !is_rd_overutilized(rd))
>        do something
> 
> This pattern includes feec() case where we have in select_task_rq_fair():
> 
> If (sched_energy_enabled())
>        feec():
>        |->  if (is_rd_overutilized())
>        |->       goto unlock
> 
> which could be changed into
> If (sched_energy_enabled() && !is_rd_overutilized(rd))
>        feec()
> 
> Then you can create the function is_rd_not_overutilized() instead of
> is_rd_overutilized()
> 
> -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);
>  }
> 
> and use is_rd_not_overutilized() instead
> 

Ok. Makes sense. I will keep this patch as is. and use the above 
approach in a new patch. 

>> +
>>  static inline void set_rd_overutilized_status(struct root_domain *rd,
>>                                               unsigned int status)
>>  {
>> @@ -6686,13 +6695,14 @@ static inline void check_update_overutilized_status(struct rq *rq)
>>         if (!sched_energy_enabled())
>>                 return;
>>
>> -       if (!READ_ONCE(rq->rd->overutilized) && cpu_overutilized(rq->cpu))
>> +       if (!is_rd_overutilized(rq->rd) && cpu_overutilized(rq->cpu))
>>                 set_rd_overutilized_status(rq->rd, SG_OVERUTILIZED);
>>  }
>>  #else
>>  static inline void check_update_overutilized_status(struct rq *rq) { }
>>  static inline void set_rd_overutilized_status(struct root_domain *rd,
>>                                               unsigned int status) { }
>> +static inline int is_rd_overutilized(struct root_domain *rd) { }
> 
> It should be
> static inline int is_rd_overutilized(struct root_domain *rd) { return 0; }

ok. 

> 
>>  #endif
>>
>>  /* Runqueue only has SCHED_IDLE tasks enqueued */
>> @@ -7974,7 +7984,7 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
>>
>>         rcu_read_lock();

>>