[PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings

[PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings

[Qian Cai]

The linux-next commit "sched/fair: Fix low cpu usage with high
throttling by removing expiration of cpu-local slices" [1] introduced a
few compilation warnings,

kernel/sched/fair.c: In function '__refill_cfs_bandwidth_runtime':
kernel/sched/fair.c:4365:6: warning: variable 'now' set but not used
[-Wunused-but-set-variable]
kernel/sched/fair.c: In function 'start_cfs_bandwidth':
kernel/sched/fair.c:4992:6: warning: variable 'overrun' set but not used
[-Wunused-but-set-variable]

Also, __refill_cfs_bandwidth_runtime() does no longer update the
expiration time, so fix the comments accordingly.

[1] https://lore.kernel.org/lkml/1558121424-2914-1-git-send-email-chiluk+linux@indeed.com/

Signed-off-by: Qian Cai <cai@lca.pw>
---

v2: Keep hrtimer_forward_now() in start_cfs_bandwidth() per Ben.

 kernel/sched/fair.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 84959d3285d1..06782491691f 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4354,21 +4354,16 @@ static inline u64 sched_cfs_bandwidth_slice(void)
 }
 
 /*
- * Replenish runtime according to assigned quota and update expiration time.
- * We use sched_clock_cpu directly instead of rq->clock to avoid adding
- * additional synchronization around rq->lock.
+ * Replenish runtime according to assigned quota. We use sched_clock_cpu
+ * directly instead of rq->clock to avoid adding additional synchronization
+ * around rq->lock.
  *
  * requires cfs_b->lock
  */
 void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b)
 {
-	u64 now;
-
-	if (cfs_b->quota == RUNTIME_INF)
-		return;
-
-	now = sched_clock_cpu(smp_processor_id());
-	cfs_b->runtime = cfs_b->quota;
+	if (cfs_b->quota != RUNTIME_INF)
+		cfs_b->runtime = cfs_b->quota;
 }
 
 static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg)
@@ -4989,15 +4984,13 @@ static void init_cfs_rq_runtime(struct cfs_rq *cfs_rq)
 
 void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
 {
-	u64 overrun;
-
 	lockdep_assert_held(&cfs_b->lock);
 
 	if (cfs_b->period_active)
 		return;
 
 	cfs_b->period_active = 1;
-	overrun = hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
+	hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
 	hrtimer_start_expires(&cfs_b->period_timer, HRTIMER_MODE_ABS_PINNED);
 }
 
-- 
1.8.3.1

Re: [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings

[bsegall]

Qian Cai <cai@lca.pw> writes:

> The linux-next commit "sched/fair: Fix low cpu usage with high
> throttling by removing expiration of cpu-local slices" [1] introduced a
> few compilation warnings,
>
> kernel/sched/fair.c: In function '__refill_cfs_bandwidth_runtime':
> kernel/sched/fair.c:4365:6: warning: variable 'now' set but not used
> [-Wunused-but-set-variable]
> kernel/sched/fair.c: In function 'start_cfs_bandwidth':
> kernel/sched/fair.c:4992:6: warning: variable 'overrun' set but not used
> [-Wunused-but-set-variable]
>
> Also, __refill_cfs_bandwidth_runtime() does no longer update the
> expiration time, so fix the comments accordingly.
>
> [1] https://lore.kernel.org/lkml/1558121424-2914-1-git-send-email-chiluk+linux@indeed.com/
>
> Signed-off-by: Qian Cai <cai@lca.pw>

Reviewed-by: Ben Segall <bsegall@google.com>

> ---
>
> v2: Keep hrtimer_forward_now() in start_cfs_bandwidth() per Ben.
>
>  kernel/sched/fair.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 84959d3285d1..06782491691f 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4354,21 +4354,16 @@ static inline u64 sched_cfs_bandwidth_slice(void)
>  }
>  
>  /*
> - * Replenish runtime according to assigned quota and update expiration time.
> - * We use sched_clock_cpu directly instead of rq->clock to avoid adding
> - * additional synchronization around rq->lock.
> + * Replenish runtime according to assigned quota. We use sched_clock_cpu
> + * directly instead of rq->clock to avoid adding additional synchronization
> + * around rq->lock.
>   *
>   * requires cfs_b->lock
>   */
>  void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b)
>  {
> -	u64 now;
> -
> -	if (cfs_b->quota == RUNTIME_INF)
> -		return;
> -
> -	now = sched_clock_cpu(smp_processor_id());
> -	cfs_b->runtime = cfs_b->quota;
> +	if (cfs_b->quota != RUNTIME_INF)
> +		cfs_b->runtime = cfs_b->quota;
>  }
>  
>  static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg)
> @@ -4989,15 +4984,13 @@ static void init_cfs_rq_runtime(struct cfs_rq *cfs_rq)
>  
>  void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
>  {
> -	u64 overrun;
> -
>  	lockdep_assert_held(&cfs_b->lock);
>  
>  	if (cfs_b->period_active)
>  		return;
>  
>  	cfs_b->period_active = 1;
> -	overrun = hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
> +	hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
>  	hrtimer_start_expires(&cfs_b->period_timer, HRTIMER_MODE_ABS_PINNED);
>  }

Re: [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings

[Dave Chiluk]

On Wed, Aug 21, 2019 at 12:36 PM <bsegall@google.com> wrote:
>
> Qian Cai <cai@lca.pw> writes:
>
> > The linux-next commit "sched/fair: Fix low cpu usage with high
> > throttling by removing expiration of cpu-local slices" [1] introduced a
> > few compilation warnings,
> >
> > kernel/sched/fair.c: In function '__refill_cfs_bandwidth_runtime':
> > kernel/sched/fair.c:4365:6: warning: variable 'now' set but not used
> > [-Wunused-but-set-variable]
> > kernel/sched/fair.c: In function 'start_cfs_bandwidth':
> > kernel/sched/fair.c:4992:6: warning: variable 'overrun' set but not used
> > [-Wunused-but-set-variable]
> >
> > Also, __refill_cfs_bandwidth_runtime() does no longer update the
> > expiration time, so fix the comments accordingly.
> >
> > [1] https://lore.kernel.org/lkml/1558121424-2914-1-git-send-email-chiluk+linux@indeed.com/
> >
> > Signed-off-by: Qian Cai <cai@lca.pw>
>
> Reviewed-by: Ben Segall <bsegall@google.com>
>
> > ---
> >
> > v2: Keep hrtimer_forward_now() in start_cfs_bandwidth() per Ben.
> >
> >  kernel/sched/fair.c | 19 ++++++-------------
> >  1 file changed, 6 insertions(+), 13 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 84959d3285d1..06782491691f 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -4354,21 +4354,16 @@ static inline u64 sched_cfs_bandwidth_slice(void)
> >  }
> >
> >  /*
> > - * Replenish runtime according to assigned quota and update expiration time.
> > - * We use sched_clock_cpu directly instead of rq->clock to avoid adding
> > - * additional synchronization around rq->lock.
> > + * Replenish runtime according to assigned quota. We use sched_clock_cpu
> > + * directly instead of rq->clock to avoid adding additional synchronization
> > + * around rq->lock.
> >   *
> >   * requires cfs_b->lock
> >   */
> >  void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b)
> >  {
> > -     u64 now;
> > -
> > -     if (cfs_b->quota == RUNTIME_INF)
> > -             return;
> > -
> > -     now = sched_clock_cpu(smp_processor_id());
> > -     cfs_b->runtime = cfs_b->quota;
> > +     if (cfs_b->quota != RUNTIME_INF)
> > +             cfs_b->runtime = cfs_b->quota;
> >  }
> >
> >  static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg)
> > @@ -4989,15 +4984,13 @@ static void init_cfs_rq_runtime(struct cfs_rq *cfs_rq)
> >
> >  void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
> >  {
> > -     u64 overrun;
> > -
> >       lockdep_assert_held(&cfs_b->lock);
> >
> >       if (cfs_b->period_active)
> >               return;
> >
> >       cfs_b->period_active = 1;
> > -     overrun = hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
> > +     hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
> >       hrtimer_start_expires(&cfs_b->period_timer, HRTIMER_MODE_ABS_PINNED);
> >  }

Looks good.
Reviewed-by: Dave Chiluk <chiluk+linux@indeed.com>

Sorry for the slow response, I was on vacation.

@Ben do you think it would be useful to still capture overrun, and
WARN on any overruns?  We wouldn't expect overruns, but their
existence would indicate an over-loaded node or too short of a
cfs_period.  Additionally, it would be interesting to see if we could
capture the offset between when the bandwidth was refilled, and when
the timer was supposed to fire.  I've always done all my calculations
assuming that the timer fires and is handled exceedingly close to the
time it was supposed to fire.  Although, if the node is running that
overloaded you probably have many more problems than worrying about
timer warnings.

Re: [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings

[bsegall]

Dave Chiluk <chiluk+linux@indeed.com> writes:

> On Wed, Aug 21, 2019 at 12:36 PM <bsegall@google.com> wrote:
>>
>> Qian Cai <cai@lca.pw> writes:
>>
>> > The linux-next commit "sched/fair: Fix low cpu usage with high
>> > throttling by removing expiration of cpu-local slices" [1] introduced a
>> > few compilation warnings,
>> >
>> > kernel/sched/fair.c: In function '__refill_cfs_bandwidth_runtime':
>> > kernel/sched/fair.c:4365:6: warning: variable 'now' set but not used
>> > [-Wunused-but-set-variable]
>> > kernel/sched/fair.c: In function 'start_cfs_bandwidth':
>> > kernel/sched/fair.c:4992:6: warning: variable 'overrun' set but not used
>> > [-Wunused-but-set-variable]
>> >
>> > Also, __refill_cfs_bandwidth_runtime() does no longer update the
>> > expiration time, so fix the comments accordingly.
>> >
>> > [1] https://lore.kernel.org/lkml/1558121424-2914-1-git-send-email-chiluk+linux@indeed.com/
>> >
>> > Signed-off-by: Qian Cai <cai@lca.pw>
>>
>> Reviewed-by: Ben Segall <bsegall@google.com>
>>
>> > ---
>> >
>> > v2: Keep hrtimer_forward_now() in start_cfs_bandwidth() per Ben.
>> >
>> >  kernel/sched/fair.c | 19 ++++++-------------
>> >  1 file changed, 6 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> > index 84959d3285d1..06782491691f 100644
>> > --- a/kernel/sched/fair.c
>> > +++ b/kernel/sched/fair.c
>> > @@ -4354,21 +4354,16 @@ static inline u64 sched_cfs_bandwidth_slice(void)
>> >  }
>> >
>> >  /*
>> > - * Replenish runtime according to assigned quota and update expiration time.
>> > - * We use sched_clock_cpu directly instead of rq->clock to avoid adding
>> > - * additional synchronization around rq->lock.
>> > + * Replenish runtime according to assigned quota. We use sched_clock_cpu
>> > + * directly instead of rq->clock to avoid adding additional synchronization
>> > + * around rq->lock.
>> >   *
>> >   * requires cfs_b->lock
>> >   */
>> >  void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b)
>> >  {
>> > -     u64 now;
>> > -
>> > -     if (cfs_b->quota == RUNTIME_INF)
>> > -             return;
>> > -
>> > -     now = sched_clock_cpu(smp_processor_id());
>> > -     cfs_b->runtime = cfs_b->quota;
>> > +     if (cfs_b->quota != RUNTIME_INF)
>> > +             cfs_b->runtime = cfs_b->quota;
>> >  }
>> >
>> >  static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg)
>> > @@ -4989,15 +4984,13 @@ static void init_cfs_rq_runtime(struct cfs_rq *cfs_rq)
>> >
>> >  void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
>> >  {
>> > -     u64 overrun;
>> > -
>> >       lockdep_assert_held(&cfs_b->lock);
>> >
>> >       if (cfs_b->period_active)
>> >               return;
>> >
>> >       cfs_b->period_active = 1;
>> > -     overrun = hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
>> > +     hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
>> >       hrtimer_start_expires(&cfs_b->period_timer, HRTIMER_MODE_ABS_PINNED);
>> >  }
>
> Looks good.
> Reviewed-by: Dave Chiluk <chiluk+linux@indeed.com>
>
> Sorry for the slow response, I was on vacation.
>
> @Ben do you think it would be useful to still capture overrun, and
> WARN on any overruns?  We wouldn't expect overruns, but their
> existence would indicate an over-loaded node or too short of a
> cfs_period.  Additionally, it would be interesting to see if we could
> capture the offset between when the bandwidth was refilled, and when
> the timer was supposed to fire.  I've always done all my calculations
> assuming that the timer fires and is handled exceedingly close to the
> time it was supposed to fire.  Although, if the node is running that
> overloaded you probably have many more problems than worrying about
> timer warnings.

That "overrun" there is not really an overrun - it's the number of
complete periods the timer has been inactive for. It was used so that a
given tg's period timer would keep the same
phase/offset/whatever-you-call-it, even if it goes idle for a while,
rather than having the next period start N ms after a task wakes up.

Also, poor choices by userspace is not generally something the kernel
generally WARNs on, as I understand it.

Re: [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings

[Phil Auld]

On Fri, Aug 23, 2019 at 10:28:02AM -0700 bsegall@google.com wrote:
> Dave Chiluk <chiluk+linux@indeed.com> writes:
> 
> > On Wed, Aug 21, 2019 at 12:36 PM <bsegall@google.com> wrote:
> >>
> >> Qian Cai <cai@lca.pw> writes:
> >>
> >> > The linux-next commit "sched/fair: Fix low cpu usage with high
> >> > throttling by removing expiration of cpu-local slices" [1] introduced a
> >> > few compilation warnings,
> >> >
> >> > kernel/sched/fair.c: In function '__refill_cfs_bandwidth_runtime':
> >> > kernel/sched/fair.c:4365:6: warning: variable 'now' set but not used
> >> > [-Wunused-but-set-variable]
> >> > kernel/sched/fair.c: In function 'start_cfs_bandwidth':
> >> > kernel/sched/fair.c:4992:6: warning: variable 'overrun' set but not used
> >> > [-Wunused-but-set-variable]
> >> >
> >> > Also, __refill_cfs_bandwidth_runtime() does no longer update the
> >> > expiration time, so fix the comments accordingly.
> >> >
> >> > [1] https://lore.kernel.org/lkml/1558121424-2914-1-git-send-email-chiluk+linux@indeed.com/
> >> >
> >> > Signed-off-by: Qian Cai <cai@lca.pw>
> >>
> >> Reviewed-by: Ben Segall <bsegall@google.com>
> >>
> >> > ---
> >> >
> >> > v2: Keep hrtimer_forward_now() in start_cfs_bandwidth() per Ben.
> >> >
> >> >  kernel/sched/fair.c | 19 ++++++-------------
> >> >  1 file changed, 6 insertions(+), 13 deletions(-)
> >> >
> >> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> >> > index 84959d3285d1..06782491691f 100644
> >> > --- a/kernel/sched/fair.c
> >> > +++ b/kernel/sched/fair.c
> >> > @@ -4354,21 +4354,16 @@ static inline u64 sched_cfs_bandwidth_slice(void)
> >> >  }
> >> >
> >> >  /*
> >> > - * Replenish runtime according to assigned quota and update expiration time.
> >> > - * We use sched_clock_cpu directly instead of rq->clock to avoid adding
> >> > - * additional synchronization around rq->lock.
> >> > + * Replenish runtime according to assigned quota. We use sched_clock_cpu
> >> > + * directly instead of rq->clock to avoid adding additional synchronization
> >> > + * around rq->lock.
> >> >   *
> >> >   * requires cfs_b->lock
> >> >   */
> >> >  void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b)
> >> >  {
> >> > -     u64 now;
> >> > -
> >> > -     if (cfs_b->quota == RUNTIME_INF)
> >> > -             return;
> >> > -
> >> > -     now = sched_clock_cpu(smp_processor_id());
> >> > -     cfs_b->runtime = cfs_b->quota;
> >> > +     if (cfs_b->quota != RUNTIME_INF)
> >> > +             cfs_b->runtime = cfs_b->quota;
> >> >  }
> >> >
> >> >  static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg)
> >> > @@ -4989,15 +4984,13 @@ static void init_cfs_rq_runtime(struct cfs_rq *cfs_rq)
> >> >
> >> >  void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
> >> >  {
> >> > -     u64 overrun;
> >> > -
> >> >       lockdep_assert_held(&cfs_b->lock);
> >> >
> >> >       if (cfs_b->period_active)
> >> >               return;
> >> >
> >> >       cfs_b->period_active = 1;
> >> > -     overrun = hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
> >> > +     hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
> >> >       hrtimer_start_expires(&cfs_b->period_timer, HRTIMER_MODE_ABS_PINNED);
> >> >  }
> >
> > Looks good.
> > Reviewed-by: Dave Chiluk <chiluk+linux@indeed.com>
> >
> > Sorry for the slow response, I was on vacation.
> >
> > @Ben do you think it would be useful to still capture overrun, and
> > WARN on any overruns?  We wouldn't expect overruns, but their
> > existence would indicate an over-loaded node or too short of a
> > cfs_period.  Additionally, it would be interesting to see if we could
> > capture the offset between when the bandwidth was refilled, and when
> > the timer was supposed to fire.  I've always done all my calculations
> > assuming that the timer fires and is handled exceedingly close to the
> > time it was supposed to fire.  Although, if the node is running that
> > overloaded you probably have many more problems than worrying about
> > timer warnings.
> 
> That "overrun" there is not really an overrun - it's the number of
> complete periods the timer has been inactive for. It was used so that a
> given tg's period timer would keep the same
> phase/offset/whatever-you-call-it, even if it goes idle for a while,
> rather than having the next period start N ms after a task wakes up.
> 
> Also, poor choices by userspace is not generally something the kernel
> generally WARNs on, as I understand it.

I don't think it matters in the start_cfs_bandwidth case, anyway. We do 
effectively check in sched_cfs_period_timer. 

Cleanup looks okay to me as well.


Cheers,
Phil

-- 

Re: [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings

[Qian Cai]

Ingo or Peter, please take a look at this trivial patch. Still see the warning
in linux-next every day.

On Tue, 2019-08-20 at 14:40 -0400, Qian Cai wrote:
> The linux-next commit "sched/fair: Fix low cpu usage with high
> throttling by removing expiration of cpu-local slices" [1] introduced a
> few compilation warnings,
> 
> kernel/sched/fair.c: In function '__refill_cfs_bandwidth_runtime':
> kernel/sched/fair.c:4365:6: warning: variable 'now' set but not used
> [-Wunused-but-set-variable]
> kernel/sched/fair.c: In function 'start_cfs_bandwidth':
> kernel/sched/fair.c:4992:6: warning: variable 'overrun' set but not used
> [-Wunused-but-set-variable]
> 
> Also, __refill_cfs_bandwidth_runtime() does no longer update the
> expiration time, so fix the comments accordingly.
> 
> [1] https://lore.kernel.org/lkml/1558121424-2914-1-git-send-email-chiluk+linux
> @indeed.com/
> 
> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
> 
> v2: Keep hrtimer_forward_now() in start_cfs_bandwidth() per Ben.
> 
>  kernel/sched/fair.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 84959d3285d1..06782491691f 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4354,21 +4354,16 @@ static inline u64 sched_cfs_bandwidth_slice(void)
>  }
>  
>  /*
> - * Replenish runtime according to assigned quota and update expiration time.
> - * We use sched_clock_cpu directly instead of rq->clock to avoid adding
> - * additional synchronization around rq->lock.
> + * Replenish runtime according to assigned quota. We use sched_clock_cpu
> + * directly instead of rq->clock to avoid adding additional synchronization
> + * around rq->lock.
>   *
>   * requires cfs_b->lock
>   */
>  void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b)
>  {
> -	u64 now;
> -
> -	if (cfs_b->quota == RUNTIME_INF)
> -		return;
> -
> -	now = sched_clock_cpu(smp_processor_id());
> -	cfs_b->runtime = cfs_b->quota;
> +	if (cfs_b->quota != RUNTIME_INF)
> +		cfs_b->runtime = cfs_b->quota;
>  }
>  
>  static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg)
> @@ -4989,15 +4984,13 @@ static void init_cfs_rq_runtime(struct cfs_rq *cfs_rq)
>  
>  void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
>  {
> -	u64 overrun;
> -
>  	lockdep_assert_held(&cfs_b->lock);
>  
>  	if (cfs_b->period_active)
>  		return;
>  
>  	cfs_b->period_active = 1;
> -	overrun = hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
> +	hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
>  	hrtimer_start_expires(&cfs_b->period_timer, HRTIMER_MODE_ABS_PINNED);
>  }
>  

Re: [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings

[Peter Zijlstra]

On Tue, Sep 03, 2019 at 09:03:26AM -0400, Qian Cai wrote:
> Ingo or Peter, please take a look at this trivial patch. Still see the warning
> in linux-next every day.
> 
> On Tue, 2019-08-20 at 14:40 -0400, Qian Cai wrote:
> > The linux-next commit "sched/fair: Fix low cpu usage with high
> > throttling by removing expiration of cpu-local slices" [1] introduced a
> > few compilation warnings,
> > 
> > kernel/sched/fair.c: In function '__refill_cfs_bandwidth_runtime':
> > kernel/sched/fair.c:4365:6: warning: variable 'now' set but not used
> > [-Wunused-but-set-variable]
> > kernel/sched/fair.c: In function 'start_cfs_bandwidth':
> > kernel/sched/fair.c:4992:6: warning: variable 'overrun' set but not used
> > [-Wunused-but-set-variable]
> > 
> > Also, __refill_cfs_bandwidth_runtime() does no longer update the
> > expiration time, so fix the comments accordingly.
> > 
> > [1] https://lore.kernel.org/lkml/1558121424-2914-1-git-send-email-chiluk+linux
> > @indeed.com/
> > 
> > Signed-off-by: Qian Cai <cai@lca.pw>

Rewrote the Changelog like so:

---
Subject: sched/fair: Fix -Wunused-but-set-variable warnings
From: Qian Cai <cai@lca.pw>
Date: Tue, 20 Aug 2019 14:40:55 -0400

Commit de53fd7aedb1 ("sched/fair: Fix low cpu usage with high
throttling by removing expiration of cpu-local slices") introduced a
few compilation warnings:

  kernel/sched/fair.c: In function '__refill_cfs_bandwidth_runtime':
  kernel/sched/fair.c:4365:6: warning: variable 'now' set but not used [-Wunused-but-set-variable]
  kernel/sched/fair.c: In function 'start_cfs_bandwidth':
  kernel/sched/fair.c:4992:6: warning: variable 'overrun' set but not used [-Wunused-but-set-variable]

Also, __refill_cfs_bandwidth_runtime() does no longer update the
expiration time, so fix the comments accordingly.

Fixes: de53fd7aedb1 ("sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices")
Signed-off-by: Qian Cai <cai@lca.pw>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ben Segall <bsegall@google.com>
Reviewed-by: Dave Chiluk <chiluk+linux@indeed.com>
Cc: mingo@redhat.com
Cc: pauld@redhat.com
Link: https://lkml.kernel.org/r/1566326455-8038-1-git-send-email-cai@lca.pw
---
 kernel/sched/fair.c |   19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4386,21 +4386,16 @@ static inline u64 sched_cfs_bandwidth_sl
 }
 
 /*
- * Replenish runtime according to assigned quota and update expiration time.
- * We use sched_clock_cpu directly instead of rq->clock to avoid adding
- * additional synchronization around rq->lock.
+ * Replenish runtime according to assigned quota. We use sched_clock_cpu
+ * directly instead of rq->clock to avoid adding additional synchronization
+ * around rq->lock.
  *
  * requires cfs_b->lock
  */
 void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b)
 {
-	u64 now;
-
-	if (cfs_b->quota == RUNTIME_INF)
-		return;
-
-	now = sched_clock_cpu(smp_processor_id());
-	cfs_b->runtime = cfs_b->quota;
+	if (cfs_b->quota != RUNTIME_INF)
+		cfs_b->runtime = cfs_b->quota;
 }
 
 static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg)
@@ -5021,15 +5016,13 @@ static void init_cfs_rq_runtime(struct c
 
 void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
 {
-	u64 overrun;
-
 	lockdep_assert_held(&cfs_b->lock);
 
 	if (cfs_b->period_active)
 		return;
 
 	cfs_b->period_active = 1;
-	overrun = hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
+	hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
 	hrtimer_start_expires(&cfs_b->period_timer, HRTIMER_MODE_ABS_PINNED);
 }
 

Re: [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings

[Qian Cai]

On Tue, 2019-09-03 at 16:15 +0200, Peter Zijlstra wrote:
> On Tue, Sep 03, 2019 at 09:03:26AM -0400, Qian Cai wrote:
> > Ingo or Peter, please take a look at this trivial patch. Still see the warning
> > in linux-next every day.
> > 
> > On Tue, 2019-08-20 at 14:40 -0400, Qian Cai wrote:
> > > The linux-next commit "sched/fair: Fix low cpu usage with high
> > > throttling by removing expiration of cpu-local slices" [1] introduced a
> > > few compilation warnings,
> > > 
> > > kernel/sched/fair.c: In function '__refill_cfs_bandwidth_runtime':
> > > kernel/sched/fair.c:4365:6: warning: variable 'now' set but not used
> > > [-Wunused-but-set-variable]
> > > kernel/sched/fair.c: In function 'start_cfs_bandwidth':
> > > kernel/sched/fair.c:4992:6: warning: variable 'overrun' set but not used
> > > [-Wunused-but-set-variable]
> > > 
> > > Also, __refill_cfs_bandwidth_runtime() does no longer update the
> > > expiration time, so fix the comments accordingly.
> > > 
> > > [1] https://lore.kernel.org/lkml/1558121424-2914-1-git-send-email-chiluk+linux
> > > @indeed.com/
> > > 
> > > Signed-off-by: Qian Cai <cai@lca.pw>
> 
> Rewrote the Changelog like so:

Looks good. I suppose it still need Ingo to pick it up, as today's tip/auto-
latest still show those warnings.

> 
> ---
> Subject: sched/fair: Fix -Wunused-but-set-variable warnings
> From: Qian Cai <cai@lca.pw>
> Date: Tue, 20 Aug 2019 14:40:55 -0400
> 
> Commit de53fd7aedb1 ("sched/fair: Fix low cpu usage with high
> throttling by removing expiration of cpu-local slices") introduced a
> few compilation warnings:
> 
>   kernel/sched/fair.c: In function '__refill_cfs_bandwidth_runtime':
>   kernel/sched/fair.c:4365:6: warning: variable 'now' set but not used [-Wunused-but-set-variable]
>   kernel/sched/fair.c: In function 'start_cfs_bandwidth':
>   kernel/sched/fair.c:4992:6: warning: variable 'overrun' set but not used [-Wunused-but-set-variable]
> 
> Also, __refill_cfs_bandwidth_runtime() does no longer update the
> expiration time, so fix the comments accordingly.
> 
> Fixes: de53fd7aedb1 ("sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices")
> Signed-off-by: Qian Cai <cai@lca.pw>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ben Segall <bsegall@google.com>
> Reviewed-by: Dave Chiluk <chiluk+linux@indeed.com>
> Cc: mingo@redhat.com
> Cc: pauld@redhat.com
> Link: https://lkml.kernel.org/r/1566326455-8038-1-git-send-email-cai@lca.pw
> ---
>  kernel/sched/fair.c |   19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4386,21 +4386,16 @@ static inline u64 sched_cfs_bandwidth_sl
>  }
>  
>  /*
> - * Replenish runtime according to assigned quota and update expiration time.
> - * We use sched_clock_cpu directly instead of rq->clock to avoid adding
> - * additional synchronization around rq->lock.
> + * Replenish runtime according to assigned quota. We use sched_clock_cpu
> + * directly instead of rq->clock to avoid adding additional synchronization
> + * around rq->lock.
>   *
>   * requires cfs_b->lock
>   */
>  void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b)
>  {
> -	u64 now;
> -
> -	if (cfs_b->quota == RUNTIME_INF)
> -		return;
> -
> -	now = sched_clock_cpu(smp_processor_id());
> -	cfs_b->runtime = cfs_b->quota;
> +	if (cfs_b->quota != RUNTIME_INF)
> +		cfs_b->runtime = cfs_b->quota;
>  }
>  
>  static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg)
> @@ -5021,15 +5016,13 @@ static void init_cfs_rq_runtime(struct c
>  
>  void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
>  {
> -	u64 overrun;
> -
>  	lockdep_assert_held(&cfs_b->lock);
>  
>  	if (cfs_b->period_active)
>  		return;
>  
>  	cfs_b->period_active = 1;
> -	overrun = hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
> +	hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
>  	hrtimer_start_expires(&cfs_b->period_timer, HRTIMER_MODE_ABS_PINNED);
>  }
>  

Re: [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings

[Qian Cai]

Ingo, I saw you sent a pull request to Linus including the commit introduced the
warnings,

https://lkml.org/lkml/2019/9/16/309

but seems not yet picked up the fix here,

https://lore.kernel.org/lkml/20190903141554.GS2349@hirez.programming.kicks-ass.n
et/

On Tue, 2019-09-10 at 16:58 -0400, Qian Cai wrote:
> On Tue, 2019-09-03 at 16:15 +0200, Peter Zijlstra wrote:
> > On Tue, Sep 03, 2019 at 09:03:26AM -0400, Qian Cai wrote:
> > > Ingo or Peter, please take a look at this trivial patch. Still see the warning
> > > in linux-next every day.
> > > 
> > > On Tue, 2019-08-20 at 14:40 -0400, Qian Cai wrote:
> > > > The linux-next commit "sched/fair: Fix low cpu usage with high
> > > > throttling by removing expiration of cpu-local slices" [1] introduced a
> > > > few compilation warnings,
> > > > 
> > > > kernel/sched/fair.c: In function '__refill_cfs_bandwidth_runtime':
> > > > kernel/sched/fair.c:4365:6: warning: variable 'now' set but not used
> > > > [-Wunused-but-set-variable]
> > > > kernel/sched/fair.c: In function 'start_cfs_bandwidth':
> > > > kernel/sched/fair.c:4992:6: warning: variable 'overrun' set but not used
> > > > [-Wunused-but-set-variable]
> > > > 
> > > > Also, __refill_cfs_bandwidth_runtime() does no longer update the
> > > > expiration time, so fix the comments accordingly.
> > > > 
> > > > [1] https://lore.kernel.org/lkml/1558121424-2914-1-git-send-email-chiluk+linux
> > > > @indeed.com/
> > > > 
> > > > Signed-off-by: Qian Cai <cai@lca.pw>
> > 
> > Rewrote the Changelog like so:
> 
> Looks good. I suppose it still need Ingo to pick it up, as today's tip/auto-
> latest still show those warnings.
> 
> > 
> > ---
> > Subject: sched/fair: Fix -Wunused-but-set-variable warnings
> > From: Qian Cai <cai@lca.pw>
> > Date: Tue, 20 Aug 2019 14:40:55 -0400
> > 
> > Commit de53fd7aedb1 ("sched/fair: Fix low cpu usage with high
> > throttling by removing expiration of cpu-local slices") introduced a
> > few compilation warnings:
> > 
> >   kernel/sched/fair.c: In function '__refill_cfs_bandwidth_runtime':
> >   kernel/sched/fair.c:4365:6: warning: variable 'now' set but not used [-Wunused-but-set-variable]
> >   kernel/sched/fair.c: In function 'start_cfs_bandwidth':
> >   kernel/sched/fair.c:4992:6: warning: variable 'overrun' set but not used [-Wunused-but-set-variable]
> > 
> > Also, __refill_cfs_bandwidth_runtime() does no longer update the
> > expiration time, so fix the comments accordingly.
> > 
> > Fixes: de53fd7aedb1 ("sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices")
> > Signed-off-by: Qian Cai <cai@lca.pw>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Reviewed-by: Ben Segall <bsegall@google.com>
> > Reviewed-by: Dave Chiluk <chiluk+linux@indeed.com>
> > Cc: mingo@redhat.com
> > Cc: pauld@redhat.com
> > Link: https://lkml.kernel.org/r/1566326455-8038-1-git-send-email-cai@lca.pw
> > ---
> >  kernel/sched/fair.c |   19 ++++++-------------
> >  1 file changed, 6 insertions(+), 13 deletions(-)
> > 
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -4386,21 +4386,16 @@ static inline u64 sched_cfs_bandwidth_sl
> >  }
> >  
> >  /*
> > - * Replenish runtime according to assigned quota and update expiration time.
> > - * We use sched_clock_cpu directly instead of rq->clock to avoid adding
> > - * additional synchronization around rq->lock.
> > + * Replenish runtime according to assigned quota. We use sched_clock_cpu
> > + * directly instead of rq->clock to avoid adding additional synchronization
> > + * around rq->lock.
> >   *
> >   * requires cfs_b->lock
> >   */
> >  void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b)
> >  {
> > -	u64 now;
> > -
> > -	if (cfs_b->quota == RUNTIME_INF)
> > -		return;
> > -
> > -	now = sched_clock_cpu(smp_processor_id());
> > -	cfs_b->runtime = cfs_b->quota;
> > +	if (cfs_b->quota != RUNTIME_INF)
> > +		cfs_b->runtime = cfs_b->quota;
> >  }
> >  
> >  static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg)
> > @@ -5021,15 +5016,13 @@ static void init_cfs_rq_runtime(struct c
> >  
> >  void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
> >  {
> > -	u64 overrun;
> > -
> >  	lockdep_assert_held(&cfs_b->lock);
> >  
> >  	if (cfs_b->period_active)
> >  		return;
> >  
> >  	cfs_b->period_active = 1;
> > -	overrun = hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
> > +	hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
> >  	hrtimer_start_expires(&cfs_b->period_timer, HRTIMER_MODE_ABS_PINNED);
> >  }
> >  

[tip: sched/urgent] sched/fair: Fix -Wunused-but-set-variable warnings

[tip-bot2 for Qian Cai]

The following commit has been merged into the sched/urgent branch of tip:

Commit-ID:     763a9ec06c409dcde2a761aac4bb83ff3938e0b3
Gitweb:        https://git.kernel.org/tip/763a9ec06c409dcde2a761aac4bb83ff3938e0b3
Author:        Qian Cai <cai@lca.pw>
AuthorDate:    Tue, 20 Aug 2019 14:40:55 -04:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Wed, 25 Sep 2019 17:42:31 +02:00

sched/fair: Fix -Wunused-but-set-variable warnings

Commit:

   de53fd7aedb1 ("sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices")

introduced a few compilation warnings:

  kernel/sched/fair.c: In function '__refill_cfs_bandwidth_runtime':
  kernel/sched/fair.c:4365:6: warning: variable 'now' set but not used [-Wunused-but-set-variable]
  kernel/sched/fair.c: In function 'start_cfs_bandwidth':
  kernel/sched/fair.c:4992:6: warning: variable 'overrun' set but not used [-Wunused-but-set-variable]

Also, __refill_cfs_bandwidth_runtime() does no longer update the
expiration time, so fix the comments accordingly.

Signed-off-by: Qian Cai <cai@lca.pw>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ben Segall <bsegall@google.com>
Reviewed-by: Dave Chiluk <chiluk+linux@indeed.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: pauld@redhat.com
Fixes: de53fd7aedb1 ("sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices")
Link: https://lkml.kernel.org/r/1566326455-8038-1-git-send-email-cai@lca.pw
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/fair.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 5bc2399..dfdac90 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4353,21 +4353,16 @@ static inline u64 sched_cfs_bandwidth_slice(void)
 }
 
 /*
- * Replenish runtime according to assigned quota and update expiration time.
- * We use sched_clock_cpu directly instead of rq->clock to avoid adding
- * additional synchronization around rq->lock.
+ * Replenish runtime according to assigned quota. We use sched_clock_cpu
+ * directly instead of rq->clock to avoid adding additional synchronization
+ * around rq->lock.
  *
  * requires cfs_b->lock
  */
 void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b)
 {
-	u64 now;
-
-	if (cfs_b->quota == RUNTIME_INF)
-		return;
-
-	now = sched_clock_cpu(smp_processor_id());
-	cfs_b->runtime = cfs_b->quota;
+	if (cfs_b->quota != RUNTIME_INF)
+		cfs_b->runtime = cfs_b->quota;
 }
 
 static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg)
@@ -4983,15 +4978,13 @@ static void init_cfs_rq_runtime(struct cfs_rq *cfs_rq)
 
 void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
 {
-	u64 overrun;
-
 	lockdep_assert_held(&cfs_b->lock);
 
 	if (cfs_b->period_active)
 		return;
 
 	cfs_b->period_active = 1;
-	overrun = hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
+	hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
 	hrtimer_start_expires(&cfs_b->period_timer, HRTIMER_MODE_ABS_PINNED);
 }