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: <090C3AE4-55E4-45F3-AEAB-3E7F26FB7D6D@lca.pw>
Date:   Sun, 26 May 2019 20:53:05 -0400
From:   Qian Cai <cai@....pw>
To:     Valentin Schneider <valentin.schneider@....com>
Cc:     peterz@...radead.org, mingo@...nel.org, vincent.guittot@...aro.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sched/fair: fix variable "done" set but not used



> On May 26, 2019, at 7:56 PM, Valentin Schneider <valentin.schneider@....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<-----
> [...]