Re: [PATCH] sched/fair: Simplify continue_balancing for newidle

[Shrikanth Hegde]

On 3/26/24 1:37 PM, Ingo Molnar wrote:
> 
> * Shrikanth Hegde <sshegde@xxxxxxxxxxxxx> 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