Re: [PATCH 1/2] sched/fair: Check rd->overload value before update

[Shrikanth Hegde]

On 3/21/24 1:14 PM, Vincent Guittot wrote:
> On Wed, 13 Mar 2024 at 16:02, Shrikanth Hegde <sshegde@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
> 

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

>> Signed-off-by: Shrikanth Hegde <sshegde@xxxxxxxxxxxxx>
>> ---
>>  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
>>