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/[email protected]/
Signed-off-by: Qian Cai <[email protected]>
---
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
Qian Cai <[email protected]> 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/[email protected]/
>
> Signed-off-by: Qian Cai <[email protected]>
Reviewed-by: Ben Segall <[email protected]>
> ---
>
> 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);
> }
On Wed, Aug 21, 2019 at 12:36 PM <[email protected]> wrote:
>
> Qian Cai <[email protected]> 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/[email protected]/
> >
> > Signed-off-by: Qian Cai <[email protected]>
>
> Reviewed-by: Ben Segall <[email protected]>
>
> > ---
> >
> > 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 <[email protected]>
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.
Dave Chiluk <[email protected]> writes:
> On Wed, Aug 21, 2019 at 12:36 PM <[email protected]> wrote:
>>
>> Qian Cai <[email protected]> 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/[email protected]/
>> >
>> > Signed-off-by: Qian Cai <[email protected]>
>>
>> Reviewed-by: Ben Segall <[email protected]>
>>
>> > ---
>> >
>> > 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 <[email protected]>
>
> 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.
On Fri, Aug 23, 2019 at 10:28:02AM -0700 [email protected] wrote:
> Dave Chiluk <[email protected]> writes:
>
> > On Wed, Aug 21, 2019 at 12:36 PM <[email protected]> wrote:
> >>
> >> Qian Cai <[email protected]> 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/[email protected]/
> >> >
> >> > Signed-off-by: Qian Cai <[email protected]>
> >>
> >> Reviewed-by: Ben Segall <[email protected]>
> >>
> >> > ---
> >> >
> >> > 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 <[email protected]>
> >
> > 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
--
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 <[email protected]>
> ---
>
> 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);
> }
>
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 <[email protected]>
Rewrote the Changelog like so:
---
Subject: sched/fair: Fix -Wunused-but-set-variable warnings
From: Qian Cai <[email protected]>
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 <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Reviewed-by: Ben Segall <[email protected]>
Reviewed-by: Dave Chiluk <[email protected]>
Cc: [email protected]
Cc: [email protected]
Link: https://lkml.kernel.org/r/[email protected]
---
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);
}
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 <[email protected]>
>
> 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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Reviewed-by: Ben Segall <[email protected]>
> Reviewed-by: Dave Chiluk <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Link: https://lkml.kernel.org/r/[email protected]
> ---
> 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);
> }
>
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/[email protected]
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 <[email protected]>
> >
> > 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 <[email protected]>
> > 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 <[email protected]>
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > Reviewed-by: Ben Segall <[email protected]>
> > Reviewed-by: Dave Chiluk <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Link: https://lkml.kernel.org/r/[email protected]
> > ---
> > 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);
> > }
> >
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 <[email protected]>
AuthorDate: Tue, 20 Aug 2019 14:40:55 -04:00
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Reviewed-by: Ben Segall <[email protected]>
Reviewed-by: Dave Chiluk <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Fixes: de53fd7aedb1 ("sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices")
Link: https://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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);
}