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