Messages in this thread | | | Subject | Re: [PATCH] sched/fair: fix variable "done" set but not used | From | Qian Cai <> | Date | Sun, 26 May 2019 20:53:05 -0400 |
| |
> 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<----- > [...]
| |