Received-SPF: pass (google.com: domain of linux-kernel+bounces-118598-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <7e6789d6-1e85-4945-8a7b-02cadc95cfa5@linux.ibm.com>
Date: Tue, 26 Mar 2024 14:30:30 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] sched/fair: Simplify continue_balancing for newidle
To: Ingo Molnar <mingo@kernel.org>
Cc: peterz@infradead.org, vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
        qyousef@layalina.io, linux-kernel@vger.kernel.org, vschneid@redhat.com,
        joshdon@google.com, riel@surriel.com
References: <20240325153926.274284-1-sshegde@linux.ibm.com>
 <ZgKCXrUbBIxp6+mu@gmail.com>
From: Shrikanth Hegde <sshegde@linux.ibm.com>
Content-Language: en-US
In-Reply-To: <ZgKCXrUbBIxp6+mu@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 3/26/24 1:37 PM, Ingo Molnar wrote:
> 
> * Shrikanth Hegde <sshegde@linux.ibm.com> wrote:
> 
>> newidle(CPU_NEWLY_IDLE) balancing doesn't stop the load balancing if the
>> continue_balancing flag is reset. Other two balancing (IDLE, BUSY) do
>> that. newidle balance stops the load balancing if rq has a task or there
>> is wakeup pending. The same checks are present in should_we_balance for
>> newidle. Hence use the return value and simplify continue_balancing
>> mechanism for newidle. Update the comment surrounding it as well.
> 
> Assuming there are no side-effects to balancing behavior.

I ran hackbench. More or less same results with patch. But thats very 
limited set of benchmarks. Let me do some more testing with it and send the 
results. 

> 
>> No change in functionality intended.
> 
> Is this actually true? Any change to behavior invalidates such a sentence.

From what i think, code path is same and I don't see any functionality changing. 
Correct me if i am wrong. 

Currently, sched_balance_newidle does the same check to bail out as the
should_we_balance check in case of newidle.  i.e  

should_we_balance
	if (env->dst_rq->nr_running > 0 || env->dst_rq->ttwu_pending)
			return 0;

sched_balance_newidle
		if (pulled_task || this_rq->nr_running > 0 ||
			this_rq->ttwu_pending)
			break;
		}

> 
>>  	/*
>> +	 * We must set idle_stamp _before_ calling sched_balance_rq()
>> +	 * for CPU_NEWLY_IDLE, such that we measure the this duration
>> +	 * as idle time.
>>  	 */
> 
> 'the this' ...?

Sorry for the typo. it should be.
"such that we measure this duration as idle time"

> 
> Thanks,
> 
> 	Ingo