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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0dd5f59e-bd7e-69d8-e3e8-dbc73820b110@arm.com>
Date:   Mon, 27 May 2019 00:56:08 +0100
From:   Valentin Schneider <valentin.schneider@....com>
To:     Qian Cai <cai@....pw>, peterz@...radead.org, mingo@...nel.org
Cc:     vincent.guittot@...aro.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sched/fair: fix variable "done" set but not used

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<-----
[...]