[PATCH 0/3] Accumulated fixes for Load/NUMA Balancing reconcilation series

[PATCH 0/3] Accumulated fixes for Load/NUMA Balancing reconcilation series

[Mel Gorman]

The following are three fixes already posted and reviewed that are needed
in tip/sched/core to address a load balancing issue, a build warning and
an RCU warning. They've already been posted but could easily have be lost
in the noise.

 kernel/sched/fair.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

-- 
2.16.4

[PATCH 1/3] sched/fair: Fix statistics for find_idlest_group()

[Mel Gorman]

From: Vincent Guittot <vincent.guittot@linaro.org>

From: Vincent Guittot <vincent.guittot@linaro.org>

sgs->group_weight is not set while gathering statistics in
update_sg_wakeup_stats(). This means that a group can be classified as
fully busy with 0 running tasks if utilization is high enough.

This path is mainly used for fork and exec.

Fixes: 57abff067a08 ("sched/fair: Rework find_idlest_group()")
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Acked-by: Mel Gorman <mgorman@techsingularity.net>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
Link: https://lore.kernel.org/r/20200218144534.4564-1-vincent.guittot@linaro.org
---
 kernel/sched/fair.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index fcc968669aea..4b5d5e5e701e 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8561,6 +8561,8 @@ static inline void update_sg_wakeup_stats(struct sched_domain *sd,
 
 	sgs->group_capacity = group->sgc->capacity;
 
+	sgs->group_weight = group->group_weight;
+
 	sgs->group_type = group_classify(sd->imbalance_pct, group, sgs);
 
 	/*
-- 
2.16.4

[PATCH 2/3] sched/fair: Fix kernel build warning in test_idle_cores() for !SMT NUMA

[Mel Gorman]

From: Valentin Schneider <valentin.schneider@arm.com>

From: Valentin Schneider <valentin.schneider@arm.com>

Building against the tip/sched/core as ff7db0bf24db ("sched/numa: Prefer
using an idle CPU as a migration target instead of comparing tasks") with
the arm64 defconfig (which doesn't have CONFIG_SCHED_SMT set) leads to:

  kernel/sched/fair.c:1525:20: warning: 'test_idle_cores' declared 'static' but never defined [-Wunused-function]
   static inline bool test_idle_cores(int cpu, bool def);
		      ^~~~~~~~~~~~~~~

Rather than define it in its own CONFIG_SCHED_SMT #define island, bunch it
up with test_idle_cores().

Fixes: ff7db0bf24db ("sched/numa: Prefer using an idle CPU as a migration target instead of comparing tasks")
[mgorman@techsingularity.net: Edit changelog, minor style change]
Reported-by: Anshuman Khandual <anshuman.khandual@arm.com>
Reported-by: Lukasz Luba <lukasz.luba@arm.com>
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
---
 kernel/sched/fair.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 4b5d5e5e701e..33622bbfcea7 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1520,9 +1520,6 @@ static inline bool is_core_idle(int cpu)
 	return true;
 }
 
-/* Forward declarations of select_idle_sibling helpers */
-static inline bool test_idle_cores(int cpu, bool def);
-
 struct task_numa_env {
 	struct task_struct *p;
 
@@ -1558,9 +1555,11 @@ numa_type numa_classify(unsigned int imbalance_pct,
 	return node_fully_busy;
 }
 
+#ifdef CONFIG_SCHED_SMT
+/* Forward declarations of select_idle_sibling helpers */
+static inline bool test_idle_cores(int cpu, bool def);
 static inline int numa_idle_core(int idle_core, int cpu)
 {
-#ifdef CONFIG_SCHED_SMT
 	if (!static_branch_likely(&sched_smt_present) ||
 	    idle_core >= 0 || !test_idle_cores(cpu, false))
 		return idle_core;
@@ -1571,10 +1570,15 @@ static inline int numa_idle_core(int idle_core, int cpu)
 	 */
 	if (is_core_idle(cpu))
 		idle_core = cpu;
-#endif
 
 	return idle_core;
 }
+#else
+static inline int numa_idle_core(int idle_core, int cpu)
+{
+	return idle_core;
+}
+#endif
 
 /*
  * Gather all necessary information to make NUMA balancing placement
-- 
2.16.4

[PATCH 3/3] sched/numa: Acquire RCU lock when checking idle cores during NUMA balancing

[Mel Gorman]

Qian Cai reported the following

  The linux-next commit ff7db0bf24db ("sched/numa: Prefer using an idle CPU as a
migration target instead of comparing tasks") introduced a boot warning,

  [   86.520534][    T1] WARNING: suspicious RCU usage
  [   86.520540][    T1] 5.6.0-rc3-next-20200227 #7 Not tainted
  [   86.520545][    T1] -----------------------------
  [   86.520551][    T1] kernel/sched/fair.c:5914 suspicious rcu_dereference_check() usage!
  [   86.520555][    T1]
  [   86.520555][    T1] other info that might help us debug this:
  [   86.520555][    T1]
  [   86.520561][    T1]
  [   86.520561][    T1] rcu_scheduler_active = 2, debug_locks = 1
  [   86.520567][    T1] 1 lock held by systemd/1:
  [   86.520571][    T1]  #0: ffff8887f4b14848 (&mm->mmap_sem#2){++++}, at: do_page_fault+0x1d2/0x998
  [   86.520594][    T1]
  [   86.520594][    T1] stack backtrace:
  [   86.520602][    T1] CPU: 1 PID: 1 Comm: systemd Not tainted 5.6.0-rc3-next-20200227 #7

task_numa_migrate() checks for idle cores when updating NUMA-related statistics.
This relies on reading a RCU-protected structure in test_idle_cores() via this
call chain

task_numa_migrate
  -> update_numa_stats
    -> numa_idle_core
      -> test_idle_cores

While the locking could be fine-grained, it is more appropriate to acquire
the RCU lock for the entire scan of the domain. This patch removes the
warning triggered at boot time.

Fixes: ff7db0bf24db ("sched/numa: Prefer using an idle CPU as a migration target instead of comparing tasks")
Reported-by: Qian Cai <cai@lca.pw>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
---
 kernel/sched/fair.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 33622bbfcea7..23a6fe298720 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1595,6 +1595,7 @@ static void update_numa_stats(struct task_numa_env *env,
 	memset(ns, 0, sizeof(*ns));
 	ns->idle_cpu = -1;
 
+	rcu_read_lock();
 	for_each_cpu(cpu, cpumask_of_node(nid)) {
 		struct rq *rq = cpu_rq(cpu);
 
@@ -1614,6 +1615,7 @@ static void update_numa_stats(struct task_numa_env *env,
 			idle_core = numa_idle_core(idle_core, cpu);
 		}
 	}
+	rcu_read_unlock();
 
 	ns->weight = cpumask_weight(cpumask_of_node(nid));
 
-- 
2.16.4

Re: [PATCH 1/3] sched/fair: Fix statistics for find_idlest_group()

[Vincent Guittot]

Hi Mel,

On Tue, 3 Mar 2020 at 12:03, Mel Gorman <mgorman@techsingularity.net> wrote:
>
> From: Vincent Guittot <vincent.guittot@linaro.org>
>
> From: Vincent Guittot <vincent.guittot@linaro.org>
>
> sgs->group_weight is not set while gathering statistics in
> update_sg_wakeup_stats(). This means that a group can be classified as
> fully busy with 0 running tasks if utilization is high enough.
>
> This path is mainly used for fork and exec.
>
> Fixes: 57abff067a08 ("sched/fair: Rework find_idlest_group()")
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Acked-by: Mel Gorman <mgorman@techsingularity.net>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> Link: https://lore.kernel.org/r/20200218144534.4564-1-vincent.guittot@linaro.org

This one has been merged in tip/sched/urgent

> ---
>  kernel/sched/fair.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index fcc968669aea..4b5d5e5e701e 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8561,6 +8561,8 @@ static inline void update_sg_wakeup_stats(struct sched_domain *sd,
>
>         sgs->group_capacity = group->sgc->capacity;
>
> +       sgs->group_weight = group->group_weight;
> +
>         sgs->group_type = group_classify(sd->imbalance_pct, group, sgs);
>
>         /*
> --
> 2.16.4
>

Re: [PATCH 1/3] sched/fair: Fix statistics for find_idlest_group()

[Mel Gorman]

On Tue, Mar 03, 2020 at 12:14:07PM +0100, Vincent Guittot wrote:
> Hi Mel,
> 
> On Tue, 3 Mar 2020 at 12:03, Mel Gorman <mgorman@techsingularity.net> wrote:
> >
> > From: Vincent Guittot <vincent.guittot@linaro.org>
> >
> > From: Vincent Guittot <vincent.guittot@linaro.org>
> >
> > sgs->group_weight is not set while gathering statistics in
> > update_sg_wakeup_stats(). This means that a group can be classified as
> > fully busy with 0 running tasks if utilization is high enough.
> >
> > This path is mainly used for fork and exec.
> >
> > Fixes: 57abff067a08 ("sched/fair: Rework find_idlest_group()")
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > Signed-off-by: Ingo Molnar <mingo@kernel.org>
> > Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Acked-by: Mel Gorman <mgorman@techsingularity.net>
> > Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> > Link: https://lore.kernel.org/r/20200218144534.4564-1-vincent.guittot@linaro.org
> 
> This one has been merged in tip/sched/urgent
> 

I know and it appears in next but not in mainline yet. As tip/sched/core
is the development baseline for scheduler patches, it should have the
patch -- most likely via mainline to preserve git history. By including
it here, I wanted to highlight that anyone working on tip/sched/core at
the moment should include the patch if they want to avoid invalidating
any test results.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 1/3] sched/fair: Fix statistics for find_idlest_group()

[Vincent Guittot]

On Tue, 3 Mar 2020 at 12:44, Mel Gorman <mgorman@techsingularity.net> wrote:
>
> On Tue, Mar 03, 2020 at 12:14:07PM +0100, Vincent Guittot wrote:
> > Hi Mel,
> >
> > On Tue, 3 Mar 2020 at 12:03, Mel Gorman <mgorman@techsingularity.net> wrote:
> > >
> > > From: Vincent Guittot <vincent.guittot@linaro.org>
> > >
> > > From: Vincent Guittot <vincent.guittot@linaro.org>
> > >
> > > sgs->group_weight is not set while gathering statistics in
> > > update_sg_wakeup_stats(). This means that a group can be classified as
> > > fully busy with 0 running tasks if utilization is high enough.
> > >
> > > This path is mainly used for fork and exec.
> > >
> > > Fixes: 57abff067a08 ("sched/fair: Rework find_idlest_group()")
> > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > Signed-off-by: Ingo Molnar <mingo@kernel.org>
> > > Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > > Acked-by: Mel Gorman <mgorman@techsingularity.net>
> > > Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> > > Link: https://lore.kernel.org/r/20200218144534.4564-1-vincent.guittot@linaro.org
> >
> > This one has been merged in tip/sched/urgent
> >
>
> I know and it appears in next but not in mainline yet. As tip/sched/core
> is the development baseline for scheduler patches, it should have the
> patch -- most likely via mainline to preserve git history. By including
> it here, I wanted to highlight that anyone working on tip/sched/core at
> the moment should include the patch if they want to avoid invalidating
> any test results.

ok. Make sense

>
> --
> Mel Gorman
> SUSE Labs

[tip: sched/core] sched/fair: Fix kernel build warning in test_idle_cores() for !SMT NUMA

[tip-bot2 for Valentin Schneider]

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     76c389ab2b5e300698eab87f9d4b7916f14117ba
Gitweb:        https://git.kernel.org/tip/76c389ab2b5e300698eab87f9d4b7916f14117ba
Author:        Valentin Schneider <valentin.schneider@arm.com>
AuthorDate:    Tue, 03 Mar 2020 11:02:57 
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Fri, 06 Mar 2020 12:57:22 +01:00

sched/fair: Fix kernel build warning in test_idle_cores() for !SMT NUMA

Building against the tip/sched/core as ff7db0bf24db ("sched/numa: Prefer
using an idle CPU as a migration target instead of comparing tasks") with
the arm64 defconfig (which doesn't have CONFIG_SCHED_SMT set) leads to:

  kernel/sched/fair.c:1525:20: warning: 'test_idle_cores' declared 'static' but never defined [-Wunused-function]
   static inline bool test_idle_cores(int cpu, bool def);
		      ^~~~~~~~~~~~~~~

Rather than define it in its own CONFIG_SCHED_SMT #define island, bunch it
up with test_idle_cores().

Reported-by: Anshuman Khandual <anshuman.khandual@arm.com>
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
[mgorman@techsingularity.net: Edit changelog, minor style change]
Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Fixes: ff7db0bf24db ("sched/numa: Prefer using an idle CPU as a migration target instead of comparing tasks")
Link: https://lkml.kernel.org/r/20200303110258.1092-3-mgorman@techsingularity.net
---
 kernel/sched/fair.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 79bb423..bba9452 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1533,9 +1533,6 @@ static inline bool is_core_idle(int cpu)
 	return true;
 }
 
-/* Forward declarations of select_idle_sibling helpers */
-static inline bool test_idle_cores(int cpu, bool def);
-
 struct task_numa_env {
 	struct task_struct *p;
 
@@ -1571,9 +1568,11 @@ numa_type numa_classify(unsigned int imbalance_pct,
 	return node_fully_busy;
 }
 
+#ifdef CONFIG_SCHED_SMT
+/* Forward declarations of select_idle_sibling helpers */
+static inline bool test_idle_cores(int cpu, bool def);
 static inline int numa_idle_core(int idle_core, int cpu)
 {
-#ifdef CONFIG_SCHED_SMT
 	if (!static_branch_likely(&sched_smt_present) ||
 	    idle_core >= 0 || !test_idle_cores(cpu, false))
 		return idle_core;
@@ -1584,10 +1583,15 @@ static inline int numa_idle_core(int idle_core, int cpu)
 	 */
 	if (is_core_idle(cpu))
 		idle_core = cpu;
-#endif
 
 	return idle_core;
 }
+#else
+static inline int numa_idle_core(int idle_core, int cpu)
+{
+	return idle_core;
+}
+#endif
 
 /*
  * Gather all necessary information to make NUMA balancing placement