Received-SPF: pass (google.com: domain of linux-kernel+bounces-22521-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240110131707.437301-1-keisuke.nishimura@inria.fr>
In-Reply-To: <20240110131707.437301-1-keisuke.nishimura@inria.fr>
From: Vincent Guittot <vincent.guittot@linaro.org>
Date: Wed, 10 Jan 2024 18:21:02 +0100
Message-ID: <CAKfTPtCgqkUUtWMknyHrNeb994V_L4T9bxV9A=bUXCQu-+S1SQ@mail.gmail.com>
Subject: Re: [PATCH v2 1/2 RESEND] sched/fair: take into account scheduling
 domain in select_idle_smt()
To: Keisuke Nishimura <keisuke.nishimura@inria.fr>
Cc: Ingo Molnar <mingo@redhat.com>, Peter Zijlstra <peterz@infradead.org>, linux-kernel@vger.kernel.org, 
	Dietmar Eggemann <dietmar.eggemann@arm.com>, Mel Gorman <mgorman@suse.de>, 
	Valentin Schneider <vschneid@redhat.com>, Julia Lawall <julia.lawall@inria.fr>, 
	Xunlei Pang <xlpang@linux.alibaba.com>, Abel Wu <wuyun.abel@bytedance.com>
Content-Type: text/plain; charset="UTF-8"

On Wed, 10 Jan 2024 at 14:19, Keisuke Nishimura
<keisuke.nishimura@inria.fr> wrote:
>
> When picking out a CPU on a task wakeup, select_idle_smt() has to take
> into account the scheduling domain of @target. This is because the
> "isolcpus" kernel command line option can remove CPUs from the domain to
> isolate them from other SMT siblings.
>
> This fix checks if the candidate CPU is in the target scheduling domain.
>
> The commit df3cb4ea1fb6 ("sched/fair: Fix wrong cpu selecting from isolated
> domain") originally proposed this fix by adding the check of the scheduling
> domain in the loop. However, the commit 3e6efe87cd5cc ("sched/fair: Remove
> redundant check in select_idle_smt()") accidentally removed the check.
> This commit brings the check back.
>
> Fixes: 3e6efe87cd5c ("sched/fair: Remove redundant check in select_idle_smt()")
> Signed-off-by: Keisuke Nishimura <keisuke.nishimura@inria.fr>
> Signed-off-by: Julia Lawall <julia.lawall@inria.fr>
> ---
> v2: - Changed the log message to mention only isolcpus
>     - Moved the check in the loop according to the original fix
>
>  kernel/sched/fair.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 533547e3c90a..66457d4b8965 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7311,13 +7311,19 @@ static int select_idle_core(struct task_struct *p, int core, struct cpumask *cpu
>  /*
>   * Scan the local SMT mask for idle CPUs.
>   */
> -static int select_idle_smt(struct task_struct *p, int target)
> +static int select_idle_smt(struct task_struct *p, struct sched_domain *sd, int target)
>  {
>         int cpu;
>
>         for_each_cpu_and(cpu, cpu_smt_mask(target), p->cpus_ptr) {
>                 if (cpu == target)
>                         continue;
> +               /*
> +                * Check if the CPU is in the LLC scheduling domain of @target.
> +                * Due to isolcpus, there is no guarantee that all the siblings are in the domain.
> +                */
> +               if (!cpumask_test_cpu(cpu, sched_domain_span(sd)))

commit df3cb4ea1fb6 ("sched/fair: Fix wrong cpu selecting from isolated domain")
also checked if (!cpumask_test_cpu(cpu, p->cpus_ptr) ||

Why didn't you also re-add this test ?


> +                       continue;
>                 if (available_idle_cpu(cpu) || sched_idle_cpu(cpu))
>                         return cpu;
>         }
> @@ -7341,7 +7347,7 @@ static inline int select_idle_core(struct task_struct *p, int core, struct cpuma
>         return __select_idle_cpu(core, p);
>  }
>
> -static inline int select_idle_smt(struct task_struct *p, int target)
> +static inline int select_idle_smt(struct task_struct *p, struct sched_domain *sd, int target)
>  {
>         return -1;
>  }
> @@ -7591,7 +7597,7 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
>                 has_idle_core = test_idle_cores(target);
>
>                 if (!has_idle_core && cpus_share_cache(prev, target)) {
> -                       i = select_idle_smt(p, prev);
> +                       i = select_idle_smt(p, sd, prev);
>                         if ((unsigned int)i < nr_cpumask_bits)
>                                 return i;
>                 }
> --
> 2.34.1
>