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

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

[Mel Gorman]

Building against tip/sched/core as of 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]
Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Mel Gorman <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 fcc968669aea..10f9e6729fcf 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

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

[Lukasz Luba]

On 2/27/20 2:02 PM, Mel Gorman wrote:
> Building against tip/sched/core as of 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]
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> Signed-off-by: Mel Gorman <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 fcc968669aea..10f9e6729fcf 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
> 

Looks good (apart from odd formatting got in '’') and calms down
yesterday's build of next (next-20200227 were I spotted it) and today's
also.

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Regards,
Lukasz

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

[Valentin Schneider]

On 2/27/20 2:02 PM, Mel Gorman wrote:
> Building against tip/sched/core as of 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]
                                          ^               ^          ^      ^
For some reason the quotes got turned into â in your edit. Doesn't really
matter TBH.

>    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]
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>

Could we get this in tip (and hopefully -next will follow shortly)? We're at
like the third duplicate fix on the list already.

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

[Mel Gorman]

On Thu, Feb 27, 2020 at 12:08:46PM +0000, Lukasz Luba wrote:
> Hi Valentin,
> 
> On 2/27/20 11:04 AM, Valentin Schneider wrote:
> > Hi Lukasz,
> > 
> > On 2/27/20 11:00 AM, Lukasz Luba wrote:
> > > Fix kernel build warning in kernel/sched/fair.c when CONFIG_SCHED_SMT is
> > > not set and CONFIG_NUMA_BALANCING is set.
> > > 
> > > kernel/sched/fair.c:1524:20: warning: ???test_idle_cores??? declared ???static??? but never defined [-Wunused-function]
> > > 
> > 
> > I've sent a similar fix yesterday at:
> > 
> > https://lore.kernel.org/lkml/20200226121244.7524-1-valentin.schneider@arm.com/
> > 
> 
> I've missed it. You are referring in the commit message to wrong change
> (probably HEAD of that branch), while when you try to bisect, it
> will point you to
> ff7db0bf24db sched/numa: Prefer using an idle CPU as a migration target
> instead of comparing tasks
> 
> I think Mel can simply resend the patches with correct build if Ingo
> is OK.
> 
> If Mel would decide to go with extended approach of ifdefs, then maybe
> it's also good to put in there  numa_idle_core(), which actually uses
> these test_idle_cores() and is_core_idle()
> 

I preferred the first fix. I'll send just it on with an editted changelog
to identify the exact patch that caused the problem. Ingo will hopefully
let me know if he prefers to pick up the fix on top or a resend of the
series with the fix folded in.

-- 
Mel Gorman
SUSE Labs

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

[Lukasz Luba]

Hi Valentin,

On 2/27/20 11:04 AM, Valentin Schneider wrote:
> Hi Lukasz,
> 
> On 2/27/20 11:00 AM, Lukasz Luba wrote:
>> Fix kernel build warning in kernel/sched/fair.c when CONFIG_SCHED_SMT is
>> not set and CONFIG_NUMA_BALANCING is set.
>>
>> kernel/sched/fair.c:1524:20: warning: ‘test_idle_cores’ declared ‘static’ but never defined [-Wunused-function]
>>
> 
> I've sent a similar fix yesterday at:
> 
> https://lore.kernel.org/lkml/20200226121244.7524-1-valentin.schneider@arm.com/
> 

I've missed it. You are referring in the commit message to wrong change
(probably HEAD of that branch), while when you try to bisect, it
will point you to
ff7db0bf24db sched/numa: Prefer using an idle CPU as a migration target 
instead of comparing tasks

I think Mel can simply resend the patches with correct build if Ingo
is OK.

If Mel would decide to go with extended approach of ifdefs, then maybe
it's also good to put in there  numa_idle_core(), which actually uses
these test_idle_cores() and is_core_idle()

Regards,
Lukasz

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

[Valentin Schneider]

Hi Lukasz,

On 2/27/20 11:00 AM, Lukasz Luba wrote:
> Fix kernel build warning in kernel/sched/fair.c when CONFIG_SCHED_SMT is
> not set and CONFIG_NUMA_BALANCING is set.
> 
> kernel/sched/fair.c:1524:20: warning: ‘test_idle_cores’ declared ‘static’ but never defined [-Wunused-function]
> 

I've sent a similar fix yesterday at:

https://lore.kernel.org/lkml/20200226121244.7524-1-valentin.schneider@arm.com/

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

[Lukasz Luba]

Fix kernel build warning in kernel/sched/fair.c when CONFIG_SCHED_SMT is
not set and CONFIG_NUMA_BALANCING is set.

kernel/sched/fair.c:1524:20: warning: ‘test_idle_cores’ declared ‘static’ but never defined [-Wunused-function]

Fixes: ff7db0bf24db9 ("sched/numa: Prefer using an idle CPU as a migration target instead of comparing tasks")
Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Vincent Guittot <vincent.guittot@linaro.org> (SCHED_NORMAL)
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> (SCHED_NORMAL)
Cc: Mel Gorman <mgorman@techsingularity.net>
---

It's on top of tip [1] master branch (which has a merge of sched/urgent).

[1] https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git

 kernel/sched/fair.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 4b5d5e5e701e..4441003b1ad1 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1520,8 +1520,10 @@ static inline bool is_core_idle(int cpu)
 	return true;
 }
 
+#ifdef CONFIG_SCHED_SMT
 /* Forward declarations of select_idle_sibling helpers */
 static inline bool test_idle_cores(int cpu, bool def);
+#endif
 
 struct task_numa_env {
 	struct task_struct *p;
-- 
2.17.1