[PATCH] sched/fair: fix variable "done" set but not used

[PATCH] sched/fair: fix variable "done" set but not used

[Qian Cai]

The commit f643ea220701 ("sched/nohz: Stop NOHZ stats when decayed")
introduced a compilation warning if CONFIG_NO_HZ_COMMON=n,

kernel/sched/fair.c: In function 'update_blocked_averages':
kernel/sched/fair.c:7750:7: warning: variable 'done' set but not used
[-Wunused-but-set-variable]

Fix it by adding a couple of "ifdef" macros as the variable is only
needed when CONFIG_NO_HZ_COMMON=y.

Signed-off-by: Qian Cai <cai@lca.pw>
---
 kernel/sched/fair.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index f35930f5e528..c8682acf4508 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7747,7 +7747,9 @@ static void update_blocked_averages(int cpu)
 	struct cfs_rq *cfs_rq, *pos;
 	const struct sched_class *curr_class;
 	struct rq_flags rf;
+#ifdef CONFIG_NO_HZ_COMMON
 	bool done = true;
+#endif
 
 	rq_lock_irqsave(rq, &rf);
 	update_rq_clock(rq);
@@ -7774,20 +7776,22 @@ static void update_blocked_averages(int cpu)
 		if (cfs_rq_is_decayed(cfs_rq))
 			list_del_leaf_cfs_rq(cfs_rq);
 
+#ifdef CONFIG_NO_HZ_COMMON
 		/* Don't need periodic decay once load/util_avg are null */
 		if (cfs_rq_has_blocked(cfs_rq))
 			done = false;
 	}
+#endif
 
 	curr_class = rq->curr->sched_class;
 	update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
 	update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
 	update_irq_load_avg(rq, 0);
+#ifdef CONFIG_NO_HZ_COMMON
 	/* Don't need periodic decay once load/util_avg are null */
 	if (others_have_blocked(rq))
 		done = false;
 
-#ifdef CONFIG_NO_HZ_COMMON
 	rq->last_blocked_load_update_tick = jiffies;
 	if (done)
 		rq->has_blocked_load = 0;
-- 
2.20.1 (Apple Git-117)

Re: [PATCH] sched/fair: fix variable "done" set but not used

[Valentin Schneider]

Hi,

On 25/05/2019 17:18, Qian Cai wrote:
> The commit f643ea220701 ("sched/nohz: Stop NOHZ stats when decayed")
> introduced a compilation warning if CONFIG_NO_HZ_COMMON=n,
> 
> kernel/sched/fair.c: In function 'update_blocked_averages':
> kernel/sched/fair.c:7750:7: warning: variable 'done' set but not used
> [-Wunused-but-set-variable]
> 

For some reason I can't get this warning to fire on my end (arm64 defconfig
+ all the NO_HZ stuff set to nope + GCC 8.1). However I do think there are
things we could improve here.

cfs_rq_has_blocked() is only used here and in a CONFIG_NO_HZ_COMMON block
within the !CONFIG_FAIR_GROUP_SCHED update_blocked_averages(). Same goes for
others_have_blocked(), so maybe these two should only be defined for
CONFIG_NO_HZ_COMMON, so we get an obvious splat when they end up in
!CONFIG_NO_HZ_COMMON paths. 

Otherwise we can have them defined as straight up false, in which case we
may be able to save ourselves some inline ifdeffery with something like the
following. It's barely compile-tested, but the objdump seems okay.

----->8-----
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index f35930f5e528..d3d6a36316f9 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7695,6 +7695,7 @@ static void attach_tasks(struct lb_env *env)
 	rq_unlock(env->dst_rq, &rf);
 }
 
+#ifdef CONFIG_NO_HZ_COMMON
 static inline bool cfs_rq_has_blocked(struct cfs_rq *cfs_rq)
 {
 	if (cfs_rq->avg.load_avg)
@@ -7705,7 +7706,11 @@ static inline bool cfs_rq_has_blocked(struct cfs_rq *cfs_rq)
 
 	return false;
 }
+#else
+static inline bool cfs_rq_has_blocked(struct cfs_rq *cfs_rq) { return false; }
+#endif
 
+#ifdef CONFIG_NO_HZ_COMMON
 static inline bool others_have_blocked(struct rq *rq)
 {
 	if (READ_ONCE(rq->avg_rt.util_avg))
@@ -7721,6 +7726,9 @@ static inline bool others_have_blocked(struct rq *rq)
 
 	return false;
 }
+#else
+static inline bool others_have_blocked(struct rq *rq) { return false; }
+#endif
 
 #ifdef CONFIG_FAIR_GROUP_SCHED
 
@@ -7741,6 +7749,18 @@ static inline bool cfs_rq_is_decayed(struct cfs_rq *cfs_rq)
 	return true;
 }
 
+#ifdef CONFIG_NO_HZ_COMMON
+static inline void update_blocked_load_status(struct rq *rq, bool has_blocked)
+{
+	rq->last_blocked_load_update_tick = jiffies;
+
+	if (!has_blocked)
+		rq->has_blocked_load = 0;
+}
+#else
+static inline void update_blocked_load_status(struct rq *rq, bool has_blocked) {}
+#endif
+
 static void update_blocked_averages(int cpu)
 {
 	struct rq *rq = cpu_rq(cpu);
@@ -7787,11 +7807,7 @@ static void update_blocked_averages(int cpu)
 	if (others_have_blocked(rq))
 		done = false;
 
-#ifdef CONFIG_NO_HZ_COMMON
-	rq->last_blocked_load_update_tick = jiffies;
-	if (done)
-		rq->has_blocked_load = 0;
-#endif
+	update_blocked_load_status(rq, !done);
 	rq_unlock_irqrestore(rq, &rf);
 }
 
-----8<-----
[...]

Re: [PATCH] sched/fair: fix variable "done" set but not used

[Qian Cai]

> On May 26, 2019, at 7:56 PM, Valentin Schneider <valentin.schneider@arm.com> wrote:
> 
> Hi,
> 
> On 25/05/2019 17:18, Qian Cai wrote:
>> The commit f643ea220701 ("sched/nohz: Stop NOHZ stats when decayed")
>> introduced a compilation warning if CONFIG_NO_HZ_COMMON=n,
>> 
>> kernel/sched/fair.c: In function 'update_blocked_averages':
>> kernel/sched/fair.c:7750:7: warning: variable 'done' set but not used
>> [-Wunused-but-set-variable]
>> 
> 
> For some reason I can't get this warning to fire on my end (arm64 defconfig
> + all the NO_HZ stuff set to nope + GCC 8.1). However I do think there are
> things we could improve here.

I like your approach more if it works. The warning can be reproduced by compiling with W=1.

> cfs_rq_has_blocked() is only used here and in a CONFIG_NO_HZ_COMMON block
> within the !CONFIG_FAIR_GROUP_SCHED update_blocked_averages(). Same goes for
> others_have_blocked(), so maybe these two should only be defined for
> CONFIG_NO_HZ_COMMON, so we get an obvious splat when they end up in
> !CONFIG_NO_HZ_COMMON paths. 
> 
> Otherwise we can have them defined as straight up false, in which case we
> may be able to save ourselves some inline ifdeffery with something like the
> following. It's barely compile-tested, but the objdump seems okay.
> 
> ----->8-----
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index f35930f5e528..d3d6a36316f9 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7695,6 +7695,7 @@ static void attach_tasks(struct lb_env *env)
> 	rq_unlock(env->dst_rq, &rf);
> }
> 
> +#ifdef CONFIG_NO_HZ_COMMON
> static inline bool cfs_rq_has_blocked(struct cfs_rq *cfs_rq)
> {
> 	if (cfs_rq->avg.load_avg)
> @@ -7705,7 +7706,11 @@ static inline bool cfs_rq_has_blocked(struct cfs_rq *cfs_rq)
> 
> 	return false;
> }
> +#else
> +static inline bool cfs_rq_has_blocked(struct cfs_rq *cfs_rq) { return false; }
> +#endif
> 
> +#ifdef CONFIG_NO_HZ_COMMON
> static inline bool others_have_blocked(struct rq *rq)
> {
> 	if (READ_ONCE(rq->avg_rt.util_avg))
> @@ -7721,6 +7726,9 @@ static inline bool others_have_blocked(struct rq *rq)
> 
> 	return false;
> }
> +#else
> +static inline bool others_have_blocked(struct rq *rq) { return false; }
> +#endif
> 
> #ifdef CONFIG_FAIR_GROUP_SCHED
> 
> @@ -7741,6 +7749,18 @@ static inline bool cfs_rq_is_decayed(struct cfs_rq *cfs_rq)
> 	return true;
> }
> 
> +#ifdef CONFIG_NO_HZ_COMMON
> +static inline void update_blocked_load_status(struct rq *rq, bool has_blocked)
> +{
> +	rq->last_blocked_load_update_tick = jiffies;
> +
> +	if (!has_blocked)
> +		rq->has_blocked_load = 0;
> +}
> +#else
> +static inline void update_blocked_load_status(struct rq *rq, bool has_blocked) {}
> +#endif
> +
> static void update_blocked_averages(int cpu)
> {
> 	struct rq *rq = cpu_rq(cpu);
> @@ -7787,11 +7807,7 @@ static void update_blocked_averages(int cpu)
> 	if (others_have_blocked(rq))
> 		done = false;
> 
> -#ifdef CONFIG_NO_HZ_COMMON
> -	rq->last_blocked_load_update_tick = jiffies;
> -	if (done)
> -		rq->has_blocked_load = 0;
> -#endif
> +	update_blocked_load_status(rq, !done);
> 	rq_unlock_irqrestore(rq, &rf);
> }
> 
> -----8<-----
> [...]

Re: [PATCH] sched/fair: fix variable "done" set but not used

[Valentin Schneider]

On 27/05/2019 01:53, Qian Cai wrote:
[...]
>> For some reason I can't get this warning to fire on my end (arm64 defconfig
>> + all the NO_HZ stuff set to nope + GCC 8.1). However I do think there are
>> things we could improve here.
> 
> I like your approach more if it works. The warning can be reproduced by compiling with W=1.
> 

Oh, duh, I thought this one would show up in the regular warnings. I gave it
a spin and the warning does disappear.

[...]

[PATCH] sched/fair: Cleanup definition of NOHZ blocked load functions

[Valentin Schneider]

cfs_rq_has_blocked() and others_have_blocked() are only used within
update_blocked_averages(). The !CONFIG_FAIR_GROUP_SCHED version of the
latter calls them within a #define CONFIG_NO_HZ_COMMON block, whereas
the CONFIG_FAIR_GROUP_SCHED one calls them unconditionnally.

As reported by Qian, the above leads to this warning in
!CONFIG_NO_HZ_COMMON configs:

  kernel/sched/fair.c: In function 'update_blocked_averages':
  kernel/sched/fair.c:7750:7: warning: variable 'done' set but not used
  [-Wunused-but-set-variable]

It wouldn't be wrong to keep cfs_rq_has_blocked() and
others_have_blocked() as they are, but since their only current use is
to figure out when we can stop calling update_blocked_averages() on
fully decayed NOHZ idle CPUs, we can give them a new definition for
!CONFIG_NO_HZ_COMMON.

Change the definition of cfs_rq_has_blocked() and
others_have_blocked() for !CONFIG_NO_HZ_COMMON so that the
NOHZ-specific blocks of update_blocked_averages() become no-ops and
the 'done' variable gets optimised out.

No change in functionality intended.

Reported-by: Qian Cai <cai@lca.pw>
Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
---
 kernel/sched/fair.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index f35930f5e528..03919a316a03 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7695,6 +7695,7 @@ static void attach_tasks(struct lb_env *env)
 	rq_unlock(env->dst_rq, &rf);
 }
 
+#ifdef CONFIG_NO_HZ_COMMON
 static inline bool cfs_rq_has_blocked(struct cfs_rq *cfs_rq)
 {
 	if (cfs_rq->avg.load_avg)
@@ -7721,6 +7722,10 @@ static inline bool others_have_blocked(struct rq *rq)
 
 	return false;
 }
+#else
+static inline bool cfs_rq_has_blocked(struct cfs_rq *cfs_rq) { return false; }
+static inline bool others_have_blocked(struct rq *rq) { return false; }
+#endif
 
 #ifdef CONFIG_FAIR_GROUP_SCHED
 
@@ -7741,6 +7746,18 @@ static inline bool cfs_rq_is_decayed(struct cfs_rq *cfs_rq)
 	return true;
 }
 
+#ifdef CONFIG_NO_HZ_COMMON
+static inline void update_blocked_load_status(struct rq *rq, bool has_blocked)
+{
+	rq->last_blocked_load_update_tick = jiffies;
+
+	if (!has_blocked)
+		rq->has_blocked_load = 0;
+}
+#else
+static inline void update_blocked_load_status(struct rq *rq, bool has_blocked) {}
+#endif
+
 static void update_blocked_averages(int cpu)
 {
 	struct rq *rq = cpu_rq(cpu);
@@ -7787,11 +7804,7 @@ static void update_blocked_averages(int cpu)
 	if (others_have_blocked(rq))
 		done = false;
 
-#ifdef CONFIG_NO_HZ_COMMON
-	rq->last_blocked_load_update_tick = jiffies;
-	if (done)
-		rq->has_blocked_load = 0;
-#endif
+	update_blocked_load_status(rq, !done);
 	rq_unlock_irqrestore(rq, &rf);
 }
 
-- 
2.20.1

Re: [PATCH] sched/fair: Cleanup definition of NOHZ blocked load functions

[Peter Zijlstra]

On Sun, Jun 02, 2019 at 05:41:10PM +0100, Valentin Schneider wrote:
> cfs_rq_has_blocked() and others_have_blocked() are only used within
> update_blocked_averages(). The !CONFIG_FAIR_GROUP_SCHED version of the
> latter calls them within a #define CONFIG_NO_HZ_COMMON block, whereas
> the CONFIG_FAIR_GROUP_SCHED one calls them unconditionnally.
> 
> As reported by Qian, the above leads to this warning in
> !CONFIG_NO_HZ_COMMON configs:
> 
>   kernel/sched/fair.c: In function 'update_blocked_averages':
>   kernel/sched/fair.c:7750:7: warning: variable 'done' set but not used
>   [-Wunused-but-set-variable]
> 
> It wouldn't be wrong to keep cfs_rq_has_blocked() and
> others_have_blocked() as they are, but since their only current use is
> to figure out when we can stop calling update_blocked_averages() on
> fully decayed NOHZ idle CPUs, we can give them a new definition for
> !CONFIG_NO_HZ_COMMON.
> 
> Change the definition of cfs_rq_has_blocked() and
> others_have_blocked() for !CONFIG_NO_HZ_COMMON so that the
> NOHZ-specific blocks of update_blocked_averages() become no-ops and
> the 'done' variable gets optimised out.
> 
> No change in functionality intended.
> 
> Reported-by: Qian Cai <cai@lca.pw>
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>

I'm thinking the below can go on top to further clean up?

--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7722,9 +7722,18 @@ static inline bool others_have_blocked(s
 
 	return false;
 }
+
+static inline void update_blocked_load_status(struct rq *rq, bool has_blocked)
+{
+	rq->last_blocked_load_update_tick = jiffies;
+
+	if (!has_blocked)
+		rq->has_blocked_load = 0;
+}
 #else
 static inline bool cfs_rq_has_blocked(struct cfs_rq *cfs_rq) { return false; }
 static inline bool others_have_blocked(struct rq *rq) { return false; }
+static inline void update_blocked_load_status(struct rq *rq, bool has_blocked) {}
 #endif
 
 #ifdef CONFIG_FAIR_GROUP_SCHED
@@ -7746,18 +7755,6 @@ static inline bool cfs_rq_is_decayed(str
 	return true;
 }
 
-#ifdef CONFIG_NO_HZ_COMMON
-static inline void update_blocked_load_status(struct rq *rq, bool has_blocked)
-{
-	rq->last_blocked_load_update_tick = jiffies;
-
-	if (!has_blocked)
-		rq->has_blocked_load = 0;
-}
-#else
-static inline void update_blocked_load_status(struct rq *rq, bool has_blocked) {}
-#endif
-
 static void update_blocked_averages(int cpu)
 {
 	struct rq *rq = cpu_rq(cpu);
@@ -7870,11 +7867,7 @@ static inline void update_blocked_averag
 	update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
 	update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
 	update_irq_load_avg(rq, 0);
-#ifdef CONFIG_NO_HZ_COMMON
-	rq->last_blocked_load_update_tick = jiffies;
-	if (!cfs_rq_has_blocked(cfs_rq) && !others_have_blocked(rq))
-		rq->has_blocked_load = 0;
-#endif
+	update_blocked_load_status(rq, cfs_rq_has_blocked(cfs_rq) || others_have_blocked(rq));
 	rq_unlock_irqrestore(rq, &rf);
 }
 

Re: [PATCH] sched/fair: Cleanup definition of NOHZ blocked load functions

[Vincent Guittot]

On Mon, 3 Jun 2019 at 11:38, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Sun, Jun 02, 2019 at 05:41:10PM +0100, Valentin Schneider wrote:
> > cfs_rq_has_blocked() and others_have_blocked() are only used within
> > update_blocked_averages(). The !CONFIG_FAIR_GROUP_SCHED version of the
> > latter calls them within a #define CONFIG_NO_HZ_COMMON block, whereas
> > the CONFIG_FAIR_GROUP_SCHED one calls them unconditionnally.
> >
> > As reported by Qian, the above leads to this warning in
> > !CONFIG_NO_HZ_COMMON configs:
> >
> >   kernel/sched/fair.c: In function 'update_blocked_averages':
> >   kernel/sched/fair.c:7750:7: warning: variable 'done' set but not used
> >   [-Wunused-but-set-variable]
> >
> > It wouldn't be wrong to keep cfs_rq_has_blocked() and
> > others_have_blocked() as they are, but since their only current use is
> > to figure out when we can stop calling update_blocked_averages() on
> > fully decayed NOHZ idle CPUs, we can give them a new definition for
> > !CONFIG_NO_HZ_COMMON.
> >
> > Change the definition of cfs_rq_has_blocked() and
> > others_have_blocked() for !CONFIG_NO_HZ_COMMON so that the
> > NOHZ-specific blocks of update_blocked_averages() become no-ops and
> > the 'done' variable gets optimised out.
> >
> > No change in functionality intended.
> >
> > Reported-by: Qian Cai <cai@lca.pw>
> > Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
>
> I'm thinking the below can go on top to further clean up?

For both patches
Acked-by: Vincent Guittot <vincent.guitto@linaro.org>

>
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7722,9 +7722,18 @@ static inline bool others_have_blocked(s
>
>         return false;
>  }
> +
> +static inline void update_blocked_load_status(struct rq *rq, bool has_blocked)
> +{
> +       rq->last_blocked_load_update_tick = jiffies;
> +
> +       if (!has_blocked)
> +               rq->has_blocked_load = 0;
> +}
>  #else
>  static inline bool cfs_rq_has_blocked(struct cfs_rq *cfs_rq) { return false; }
>  static inline bool others_have_blocked(struct rq *rq) { return false; }
> +static inline void update_blocked_load_status(struct rq *rq, bool has_blocked) {}
>  #endif
>
>  #ifdef CONFIG_FAIR_GROUP_SCHED
> @@ -7746,18 +7755,6 @@ static inline bool cfs_rq_is_decayed(str
>         return true;
>  }
>
> -#ifdef CONFIG_NO_HZ_COMMON
> -static inline void update_blocked_load_status(struct rq *rq, bool has_blocked)
> -{
> -       rq->last_blocked_load_update_tick = jiffies;
> -
> -       if (!has_blocked)
> -               rq->has_blocked_load = 0;
> -}
> -#else
> -static inline void update_blocked_load_status(struct rq *rq, bool has_blocked) {}
> -#endif
> -
>  static void update_blocked_averages(int cpu)
>  {
>         struct rq *rq = cpu_rq(cpu);
> @@ -7870,11 +7867,7 @@ static inline void update_blocked_averag
>         update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
>         update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
>         update_irq_load_avg(rq, 0);
> -#ifdef CONFIG_NO_HZ_COMMON
> -       rq->last_blocked_load_update_tick = jiffies;
> -       if (!cfs_rq_has_blocked(cfs_rq) && !others_have_blocked(rq))
> -               rq->has_blocked_load = 0;
> -#endif
> +       update_blocked_load_status(rq, cfs_rq_has_blocked(cfs_rq) || others_have_blocked(rq));
>         rq_unlock_irqrestore(rq, &rf);
>  }
>

Re: [PATCH] sched/fair: Cleanup definition of NOHZ blocked load functions

[Valentin Schneider]

On 03/06/2019 10:38, Peter Zijlstra wrote:
[...]
> 
> I'm thinking the below can go on top to further clean up?
> 

Yep, that's even better indeed! Want me to resend with that extra diff?

[...]

Re: [PATCH] sched/fair: Cleanup definition of NOHZ blocked load functions

[Peter Zijlstra]

On Mon, Jun 03, 2019 at 11:22:40AM +0100, Valentin Schneider wrote:
> On 03/06/2019 10:38, Peter Zijlstra wrote:
> [...]
> > 
> > I'm thinking the below can go on top to further clean up?
> > 
> 
> Yep, that's even better indeed! Want me to resend with that extra diff?

Yes please, I didn't even get it near a compiler, so who konws what it
will actually do ;-)