Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <71589225-a4fd-b3eb-14d5-81c9a19419a7@bytedance.com>
Date:   Tue, 25 Oct 2022 19:10:22 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.13.1
Subject: Re: [External] Re: [PATCH 1/2] sched/numa: Stop an exhastive search
 if an idle core is found
To:     Mel Gorman <mgorman@suse.de>
Cc:     mingo@redhat.com, peterz@infradead.org, mingo@kernel.org,
        juri.lelli@redhat.com, vincent.guittot@linaro.org,
        dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com,
        bristot@redhat.com, vschneid@redhat.com,
        mgorman@techsingularity.net, linux-kernel@vger.kernel.org
References: <20221021061558.34767-1-jiahao.os@bytedance.com>
 <20221021061558.34767-2-jiahao.os@bytedance.com>
 <20221024133435.e2kajx5k7jzznp25@suse.de>
 <ced7c05a-121b-a77d-0c57-3e60abaecacd@bytedance.com>
 <20221025093226.dm4sjvdq2tofkwvc@suse.de>
From:   Hao Jia <jiahao.os@bytedance.com>
In-Reply-To: <20221025093226.dm4sjvdq2tofkwvc@suse.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2022/10/25 Mel Gorman wrote:
> On Tue, Oct 25, 2022 at 11:16:29AM +0800, Hao Jia wrote:
>>> Remove the change in the first hunk and call break in the second hunk
>>> after updating ns->idle_cpu.
>>>
>>
>> Yes, thanks for your review.
>> If I understand correctly, some things might look like this.
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index e4a0b8bd941c..dfcb620bfe50 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -1792,7 +1792,7 @@ static void update_numa_stats(struct task_numa_env
>> *env,
>>                  ns->nr_running += rq->cfs.h_nr_running;
>>                  ns->compute_capacity += capacity_of(cpu);
>>
>> -               if (find_idle && !rq->nr_running && idle_cpu(cpu)) {
>> +               if (find_idle && idle_core < 0 && !rq->nr_running &&
>> idle_cpu(cpu)) {
>>                          if (READ_ONCE(rq->numa_migrate_on) ||
>>                              !cpumask_test_cpu(cpu, env->p->cpus_ptr))
>>                                  continue;
>>
> 
> I meant more like the below but today I wondered why did I not do this in
> the first place? The answer is because it's wrong and broken in concept.
> 
> The full loop is needed to calculate approximate NUMA stats at a
> point in time. For example, the src and dst nr_running is needed by
> task_numa_find_cpu. The search for an idle CPU or core in update_numa_stats
> is simply taking advantage of the fact we are scanning anyway to keep
> track of an idle CPU or core to avoid a second search as per ff7db0bf24db
> ("sched/numa: Prefer using an idle CPU as a migration target instead of
> comparing tasks")
> 
> The patch I had in mind is below but that said, for both your version and
> my initial suggestion
> 
> Naked-by: Mel Gorman <mgorman@suse.de>
> 
> For the record, this is what I was suggesting initially because it's more
> efficient but it's wrong, don't do it.
> 

Thanks for the detailed explanation, maybe my commit message misled you.

Yes, we can't stop the whole loop of scanning the CPU because we have a 
lot of NUMA information to count.

But we can stop looking for the next idle core or idle cpu after finding 
an idle core.

So, please review the previous code.


Thanks,
Hao

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index e4a0b8bd941c..7f1f6a1736a5 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -1800,7 +1800,12 @@ static void update_numa_stats(struct task_numa_env *env,
>   			if (ns->idle_cpu == -1)
>   				ns->idle_cpu = cpu;
>   
> +			/* If we find an idle core, stop searching. */
>   			idle_core = numa_idle_core(idle_core, cpu);
> +			if (idle_core >= 0) {
> +				ns->idle_cpu = idle_core;
> +				break;
> +			}
>   		}
>   	}
>   	rcu_read_unlock();
> @@ -1808,9 +1813,6 @@ static void update_numa_stats(struct task_numa_env *env,
>   	ns->weight = cpumask_weight(cpumask_of_node(nid));
>   
>   	ns->node_type = numa_classify(env->imbalance_pct, ns);
> -
> -	if (idle_core >= 0)
> -		ns->idle_cpu = idle_core;
>   }
>   
>   static void task_numa_assign(struct task_numa_env *env,
>