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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Thu, 21 Mar 2024 22:51:14 +0530
From: Shrikanth Hegde <sshegde@...ux.ibm.com>
To: Vincent Guittot <vincent.guittot@...aro.org>
Cc: mingo@...nel.org, peterz@...radead.org, dietmar.eggemann@....com,
        qyousef@...alina.io, linux-kernel@...r.kernel.org, vschneid@...hat.com,
        qperret@...gle.com, yu.c.chen@...el.com, srikar@...ux.ibm.com,
        pierre.gondois@....com
Subject: Re: [PATCH 1/2] sched/fair: Check rd->overload value before update



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

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

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