* [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings
@ 2019-08-20 18:40 Qian Cai
2019-08-21 17:36 ` bsegall
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Qian Cai @ 2019-08-20 18:40 UTC (permalink / raw)
To: mingo, peterz; +Cc: bsegall, chiluk+linux, pauld, linux-kernel, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings
2019-08-20 18:40 [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings Qian Cai
@ 2019-08-21 17:36 ` bsegall
2019-08-23 14:48 ` Dave Chiluk
2019-09-03 13:03 ` Qian Cai
2019-09-27 8:10 ` [tip: sched/urgent] sched/fair: Fix " tip-bot2 for Qian Cai
2 siblings, 1 reply; 10+ messages in thread
From: bsegall @ 2019-08-21 17:36 UTC (permalink / raw)
To: Qian Cai; +Cc: mingo, peterz, chiluk+linux, pauld, linux-kernel
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);
> }
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings
2019-08-21 17:36 ` bsegall
@ 2019-08-23 14:48 ` Dave Chiluk
2019-08-23 17:28 ` bsegall
0 siblings, 1 reply; 10+ messages in thread
From: Dave Chiluk @ 2019-08-23 14:48 UTC (permalink / raw)
To: Ben Segall
Cc: Qian Cai, Ingo Molnar, Peter Zijlstra, Phil Auld,
Linux Kernel Mailing List
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings
2019-08-23 14:48 ` Dave Chiluk
@ 2019-08-23 17:28 ` bsegall
2019-08-23 18:03 ` Phil Auld
0 siblings, 1 reply; 10+ messages in thread
From: bsegall @ 2019-08-23 17:28 UTC (permalink / raw)
To: Dave Chiluk
Cc: Qian Cai, Ingo Molnar, Peter Zijlstra, Phil Auld,
Linux Kernel Mailing List
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings
2019-08-23 17:28 ` bsegall
@ 2019-08-23 18:03 ` Phil Auld
0 siblings, 0 replies; 10+ messages in thread
From: Phil Auld @ 2019-08-23 18:03 UTC (permalink / raw)
To: bsegall
Cc: Dave Chiluk, Qian Cai, Ingo Molnar, Peter Zijlstra,
Linux Kernel Mailing List
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
--
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings
2019-08-20 18:40 [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings Qian Cai
2019-08-21 17:36 ` bsegall
@ 2019-09-03 13:03 ` Qian Cai
2019-09-03 14:15 ` Peter Zijlstra
2019-09-27 8:10 ` [tip: sched/urgent] sched/fair: Fix " tip-bot2 for Qian Cai
2 siblings, 1 reply; 10+ messages in thread
From: Qian Cai @ 2019-09-03 13:03 UTC (permalink / raw)
To: mingo, peterz; +Cc: bsegall, chiluk+linux, pauld, linux-kernel
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);
> }
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings
2019-09-03 13:03 ` Qian Cai
@ 2019-09-03 14:15 ` Peter Zijlstra
2019-09-10 20:58 ` Qian Cai
0 siblings, 1 reply; 10+ messages in thread
From: Peter Zijlstra @ 2019-09-03 14:15 UTC (permalink / raw)
To: Qian Cai; +Cc: mingo, bsegall, chiluk+linux, pauld, linux-kernel
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);
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings
2019-09-03 14:15 ` Peter Zijlstra
@ 2019-09-10 20:58 ` Qian Cai
2019-09-16 13:45 ` Qian Cai
0 siblings, 1 reply; 10+ messages in thread
From: Qian Cai @ 2019-09-10 20:58 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: mingo, bsegall, chiluk+linux, pauld, linux-kernel
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);
> }
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings
2019-09-10 20:58 ` Qian Cai
@ 2019-09-16 13:45 ` Qian Cai
0 siblings, 0 replies; 10+ messages in thread
From: Qian Cai @ 2019-09-16 13:45 UTC (permalink / raw)
To: Linus Torvalds, Peter Zijlstra
Cc: mingo, bsegall, chiluk+linux, pauld, linux-kernel
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);
> > }
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tip: sched/urgent] sched/fair: Fix -Wunused-but-set-variable warnings
2019-08-20 18:40 [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings Qian Cai
2019-08-21 17:36 ` bsegall
2019-09-03 13:03 ` Qian Cai
@ 2019-09-27 8:10 ` tip-bot2 for Qian Cai
2 siblings, 0 replies; 10+ messages in thread
From: tip-bot2 for Qian Cai @ 2019-09-27 8:10 UTC (permalink / raw)
To: linux-tip-commits
Cc: Qian Cai, Peter Zijlstra (Intel),
Ben Segall, Dave Chiluk, Linus Torvalds, Thomas Gleixner, pauld,
Ingo Molnar, Borislav Petkov, linux-kernel
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);
}
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-09-27 8:11 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-20 18:40 [PATCH -next v2] sched/fair: fix -Wunused-but-set-variable warnings Qian Cai
2019-08-21 17:36 ` bsegall
2019-08-23 14:48 ` Dave Chiluk
2019-08-23 17:28 ` bsegall
2019-08-23 18:03 ` Phil Auld
2019-09-03 13:03 ` Qian Cai
2019-09-03 14:15 ` Peter Zijlstra
2019-09-10 20:58 ` Qian Cai
2019-09-16 13:45 ` Qian Cai
2019-09-27 8:10 ` [tip: sched/urgent] sched/fair: Fix " tip-bot2 for Qian Cai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).