Received-SPF: pass (google.com: domain of linux-kernel+bounces-110493-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <7080aa6d-34e4-4d25-9f1b-95ba07f0e6bb@linux.ibm.com>
Date: Thu, 21 Mar 2024 22:51:14 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/2] sched/fair: Check rd->overload value before update
Content-Language: en-US
To: Vincent Guittot <vincent.guittot@linaro.org>
Cc: mingo@kernel.org, peterz@infradead.org, dietmar.eggemann@arm.com,
        qyousef@layalina.io, linux-kernel@vger.kernel.org, vschneid@redhat.com,
        qperret@google.com, yu.c.chen@intel.com, srikar@linux.ibm.com,
        pierre.gondois@arm.com
References: <20240313150133.1182298-1-sshegde@linux.ibm.com>
 <20240313150133.1182298-2-sshegde@linux.ibm.com>
 <CAKfTPtBYAibCdsPumTAPDKBwNfLP9nr5577MSNKffV_VDmHDgA@mail.gmail.com>
From: Shrikanth Hegde <sshegde@linux.ibm.com>
In-Reply-To: <CAKfTPtBYAibCdsPumTAPDKBwNfLP9nr5577MSNKffV_VDmHDgA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 3/21/24 1:14 PM, Vincent Guittot wrote:
> On Wed, 13 Mar 2024 at 16:02, Shrikanth Hegde <sshegde@linux.ibm.com> wrote:
>>

Hi Vincent. Thanks for taking a look and review it. 

>>
>> Perf probes prove that actual change in value is less often. So it would
>> be better to check the value before updating it. Detailed perf probes
>> and annotation can be found in the cover letter. CPU bus traffic reduces
> 
> the cover letter will not be merged. It's better to put the figures
> here if you want to mention them

ok. yes, since it was long i didnt put here. maybe i can put hackbench results 
here in the changelog. rest can be found in cover-letter if one is interested. 

> 
>> with the patch.
>>
>> Fixes: 630246a06ae2 ("sched/fair: Clean-up update_sg_lb_stats parameters")
> 
> I don't think it's worth a Fixes tag. This is an improvement more than
> a fix as the current code works fine but generates more bus traffic
> 

got it. will remove it. I wasn't sure if I have to put it.

> With these minor changes in the commit message:
> 
> Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>
> 

Thank you!  I will send out v2 with these minor edits. 

>> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
>> ---
>>  kernel/sched/fair.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 02d4d224b436..eeebadd7d9ae 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -10621,7 +10621,8 @@ static inline void update_sd_lb_stats(struct lb_env *env, struct sd_lb_stats *sd
>>
>>         if (!env->sd->parent) {
>>                 /* update overload indicator if we are at root domain */
>> -               WRITE_ONCE(env->dst_rq->rd->overload, sg_status & SG_OVERLOAD);
>> +               if (READ_ONCE(env->dst_rq->rd->overload) != (sg_status & SG_OVERLOAD))
>> +                       WRITE_ONCE(env->dst_rq->rd->overload, sg_status & SG_OVERLOAD);
>>
>>                 /* Update over-utilization (tipping point, U >= 0) indicator */
>>                 set_rd_overutilized_status(env->dst_rq->rd,
>> --
>> 2.39.3
>>