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

[Shrikanth Hegde <sshegde@linux.ibm.com>]

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@layalina.io>
> 
>