Received-SPF: pass (google.com: domain of linux-kernel+bounces-116465-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <59224c52-1864-4920-8bac-9da308d257db@linux.ibm.com>
Date: Mon, 25 Mar 2024 10:42:05 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 2/2] sched/fair: Use helper functions to access
 rd->overload
To: Qais Yousef <qyousef@layalina.io>
Cc: mingo@kernel.org, peterz@infradead.org, vincent.guittot@linaro.org,
        dietmar.eggemann@arm.com, linux-kernel@vger.kernel.org,
        vschneid@redhat.com, qperret@google.com
References: <20240322141632.28654-1-sshegde@linux.ibm.com>
 <20240322141632.28654-3-sshegde@linux.ibm.com>
 <20240324011620.f3mmywh44htvlb5m@airbuntu>
Content-Language: en-US
From: Shrikanth Hegde <sshegde@linux.ibm.com>
In-Reply-To: <20240324011620.f3mmywh44htvlb5m@airbuntu>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


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>
> 
>