linux-kernel - Re: [PATCH v2 2/2] sched/fair: Use helper functions to access rd->overload

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]

Message-ID: <59224c52-1864-4920-8bac-9da308d257db@linux.ibm.com>
Date: Mon, 25 Mar 2024 10:42:05 +0530
From: Shrikanth Hegde <sshegde@...ux.ibm.com>
To: Qais Yousef <qyousef@...alina.io>
Cc: mingo@...nel.org, peterz@...radead.org, vincent.guittot@...aro.org,
        dietmar.eggemann@....com, linux-kernel@...r.kernel.org,
        vschneid@...hat.com, qperret@...gle.com
Subject: Re: [PATCH v2 2/2] sched/fair: Use helper functions to access
 rd->overload



On 3/24/24 6:46 AM, Qais Yousef wrote:
> On 03/22/24 19:46, Shrikanth Hegde wrote:

>>
>>  	if (!env->sd->parent) {
>>  		/* update overload indicator if we are at root domain */
>> -		if (READ_ONCE(env->dst_rq->rd->overload) != (sg_status & SG_OVERLOAD))
>> -			WRITE_ONCE(env->dst_rq->rd->overload, sg_status & SG_OVERLOAD);
>> +		if (is_rd_overloaded(env->dst_rq->rd) != (sg_status & SG_OVERLOAD))
>> +			set_rd_overload_status(env->dst_rq->rd,
>> +					       sg_status & SG_OVERLOAD);
> 
> A bit picky, but..
> 
> Wouldn't it be better to encapsulate the check of whether we're writing a new
> value inside set_rd_overload_status()? Only write if it the value changed and
> all future users wouldn't care then.
> 
> I think no need to wrap the line too.
> 

Yes. Makes sense all the places do the check and update, can be wrapped. 
I will send out v3.  

>>
>>  		/* Update over-utilization (tipping point, U >= 0) indicator */
>>  		set_rd_overutilized_status(env->dst_rq->rd,
>> @@ -12344,7 +12345,7 @@ static int sched_balance_newidle(struct rq *this_rq, struct rq_flags *rf)
>>  	rcu_read_lock();
>>  	sd = rcu_dereference_check_sched_domain(this_rq->sd);
>>
>> -	if (!READ_ONCE(this_rq->rd->overload) ||
>> +	if (!is_rd_overloaded(this_rq->rd) ||
>>  	    (sd && this_rq->avg_idle < sd->max_newidle_lb_cost)) {
>>
>>  		if (sd)
>> @@ -2518,8 +2528,8 @@ static inline void add_nr_running(struct rq *rq, unsigned count)
>>
>>  #ifdef CONFIG_SMP
>>  	if (prev_nr < 2 && rq->nr_running >= 2) {
>> -		if (!READ_ONCE(rq->rd->overload))
>> -			WRITE_ONCE(rq->rd->overload, 1);
>> +		if (!is_rd_overloaded(rq->rd))
>> +			set_rd_overload_status(rq->rd, 1);
> 
> While at it, could you write SG_OVERLOAD instead of 1? 
Done.  
> 
> Both patches LGTM otherwise
> 
> Reviewed-by: Qais Yousef <qyousef@...alina.io>
> 
>