linux-kernel - Re: [PATCH] sched/fair: Simplify continue

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <7e6789d6-1e85-4945-8a7b-02cadc95cfa5@linux.ibm.com>
Date: Tue, 26 Mar 2024 14:30:30 +0530
From: Shrikanth Hegde <sshegde@...ux.ibm.com>
To: Ingo Molnar <mingo@...nel.org>
Cc: peterz@...radead.org, vincent.guittot@...aro.org, dietmar.eggemann@....com,
        qyousef@...alina.io, linux-kernel@...r.kernel.org, vschneid@...hat.com,
        joshdon@...gle.com, riel@...riel.com
Subject: Re: [PATCH] sched/fair: Simplify continue_balancing for newidle



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