Received-SPF: pass (google.com: domain of linux-kernel+bounces-212552-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C
Message-ID: <65c19534-6eaa-a7c5-2131-b9e6cea8e7c9@amd.com>
Date: Thu, 13 Jun 2024 10:10:26 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.1
Subject: Re: [PATCH] sched/fair: prefer available idle cpu in select_idle_core
Content-Language: en-US
To: <zhangwei123171@gmail.com>
CC: <linux-kernel@vger.kernel.org>, zhangwei123171 <zhangwei123171@jd.com>,
	<mingo@redhat.com>, <peterz@infradead.org>, <juri.lelli@redhat.com>,
	<vincent.guittot@linaro.org>, <dietmar.eggemann@arm.com>,
	<rostedt@goodmis.org>
References: <20240612115410.1659149-1-zhangwei123171@jd.com>
From: K Prateek Nayak <kprateek.nayak@amd.com>
In-Reply-To: <20240612115410.1659149-1-zhangwei123171@jd.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Jun 2024 04:40:37.1632
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 89b57d7f-aac3-499b-ceca-08dc8b62f8b1
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com]
X-MS-Exchange-CrossTenant-AuthSource:
	MWH0EPF000989EC.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB6163

Hello there,

On 6/12/2024 5:24 PM, zhangwei123171@gmail.com wrote:
> From: zhangwei123171 <zhangwei123171@jd.com>
> 
> When the idle core cannot be found, the first sched idle cpu
> or first available idle cpu will be used if exsit.
> 
> We can use the available idle cpu detected later to ensure it
> can be used if exsit.

Is there any particular advantage of the same? Based on my understanding
the check exists to prevent unnecessary calls to cpumask_test_cpu() if
an idle CPU is already found. On a large core count system with a large
number of cores in the LLC domain, this may result in a lot more calls
to cpumask_test_cpu() if only one core is in fact idle and there is a
storm of wakeups.

For SMT-2 system, I believe any idle thread on a busy core would be the
same (if we consider all task to have same behavior). On a larger SMT
system, it takes more overhead to consider which core is the most idle.
Consider the following case:

o CPUs of core: 0-7; Only CPU1 is busy (i is idle, b is busy)

   +---+---+---+---+---+---+---+---+
   | i | b | i | i | i | i | i | i |
   +---+---+---+---+---+---+---+---+
         ^
   select idle core bails out at first busy CPU which is CPU1 however
   this core is only 1/8th busy.

o CPUs of core: 8-15; CPU10 to CPU15 are busy (i is idle, b is busy)

   +---+---+---+---+---+---+---+---+
   | i | i | b | b | b | b | b | b |
   +---+---+---+---+---+---+---+---+
             ^
   select idle core bails out at first busy CPU which is CPU10 however
   this core is in fact 5/8th busy.

Technically, core with CPU0 is better but with your change, we'll select
core of CPU8. Bottom line being, there does not seem to exist a good
case where selecting the last idle thread is better than selecting the
first one. The best the scheduler can do is reduce the number of calls
to cpumask_test_cpu() once an idle CPU is found unless it decides to
scan all the CPUs of the core to find the core which is the idlest and
in a large, busy system, that is a big hammer.

Thoughts?

> 
> Signed-off-by: zhangwei123171 <zhangwei123171@jd.com>
> ---
>   kernel/sched/fair.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 41b58387023d..653ca3ea09b6 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7341,7 +7341,7 @@ static int select_idle_core(struct task_struct *p, int core, struct cpumask *cpu
>   			}
>   			break;
>   		}
> -		if (*idle_cpu == -1 && cpumask_test_cpu(cpu, cpus))
> +		if (cpumask_test_cpu(cpu, cpus))
>   			*idle_cpu = cpu;
>   	}
>   

--
Thanks and Regards,
Prateek