2008-08-19 10:46:16

by Peter Zijlstra

Subject: [PATCH 2/6] sched: rt-bandwidth accounting fix

It fixes an accounting bug where we would continue accumulating runtime
even though the bandwidth control is disabled. This would lead to very long
throttle periods once bandwidth control gets turned on again.

Signed-off-by: Peter Zijlstra <[email protected]>
---
kernel/sched_rt.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)

Index: linux-2.6/kernel/sched_rt.c
===================================================================
--- linux-2.6.orig/kernel/sched_rt.c
+++ linux-2.6/kernel/sched_rt.c
@@ -438,9 +438,6 @@ static int sched_rt_runtime_exceeded(str
{
u64 runtime = sched_rt_runtime(rt_rq);

- if (runtime == RUNTIME_INF)
- return 0;
-
if (rt_rq->rt_throttled)
return rt_rq_throttled(rt_rq);

@@ -491,9 +488,11 @@ static void update_curr_rt(struct rq *rq
rt_rq = rt_rq_of_se(rt_se);

spin_lock(&rt_rq->rt_runtime_lock);
- rt_rq->rt_time += delta_exec;
- if (sched_rt_runtime_exceeded(rt_rq))
- resched_task(curr);
+ if (sched_rt_runtime(rt_rq) != RUNTIME_INF) {
+ rt_rq->rt_time += delta_exec;
+ if (sched_rt_runtime_exceeded(rt_rq))
+ resched_task(curr);
+ }
spin_unlock(&rt_rq->rt_runtime_lock);
}
}

--

2008-08-19 18:34:16

by Max Krasnyansky

Subject: Re: [PATCH 2/6] sched: rt-bandwidth accounting fix

Peter Zijlstra wrote:
> It fixes an accounting bug where we would continue accumulating runtime
> even though the bandwidth control is disabled. This would lead to very long
> throttle periods once bandwidth control gets turned on again.
>
> Signed-off-by: Peter Zijlstra <[email protected]>
> ---
> kernel/sched_rt.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> Index: linux-2.6/kernel/sched_rt.c
> ===================================================================
> --- linux-2.6.orig/kernel/sched_rt.c
> +++ linux-2.6/kernel/sched_rt.c
> @@ -438,9 +438,6 @@ static int sched_rt_runtime_exceeded(str
> {
> u64 runtime = sched_rt_runtime(rt_rq);
>
> - if (runtime == RUNTIME_INF)
> - return 0;
> -
> if (rt_rq->rt_throttled)
> return rt_rq_throttled(rt_rq);
>
> @@ -491,9 +488,11 @@ static void update_curr_rt(struct rq *rq
> rt_rq = rt_rq_of_se(rt_se);
>
> spin_lock(&rt_rq->rt_runtime_lock);
> - rt_rq->rt_time += delta_exec;
> - if (sched_rt_runtime_exceeded(rt_rq))
> - resched_task(curr);
> + if (sched_rt_runtime(rt_rq) != RUNTIME_INF) {
> + rt_rq->rt_time += delta_exec;
> + if (sched_rt_runtime_exceeded(rt_rq))
> + resched_task(curr);
> + }
> spin_unlock(&rt_rq->rt_runtime_lock);
> }
> }

This will make 'disabled' case more expensive, will it not ?
I mean now we'll have to run balance_runtime() even if throttling is
disabled.

Do you guys mind if I make this stuff configurable ? ie Just like
CONFIG_RT_GROUP_SCHED we could add CONFIG_RT_BANDWIDTH_THROTTLE.

Max

2008-08-19 18:39:29

by Peter Zijlstra

Subject: Re: [PATCH 2/6] sched: rt-bandwidth accounting fix

On Tue, 2008-08-19 at 11:33 -0700, Max Krasnyansky wrote:
> Peter Zijlstra wrote:
> > It fixes an accounting bug where we would continue accumulating runtime
> > even though the bandwidth control is disabled. This would lead to very long
> > throttle periods once bandwidth control gets turned on again.
> >
> > Signed-off-by: Peter Zijlstra <[email protected]>
> > ---
> > kernel/sched_rt.c | 11 +++++------
> > 1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > Index: linux-2.6/kernel/sched_rt.c
> > ===================================================================
> > --- linux-2.6.orig/kernel/sched_rt.c
> > +++ linux-2.6/kernel/sched_rt.c
> > @@ -438,9 +438,6 @@ static int sched_rt_runtime_exceeded(str
> > {
> > u64 runtime = sched_rt_runtime(rt_rq);
> >
> > - if (runtime == RUNTIME_INF)
> > - return 0;
> > -
> > if (rt_rq->rt_throttled)
> > return rt_rq_throttled(rt_rq);
> >
> > @@ -491,9 +488,11 @@ static void update_curr_rt(struct rq *rq
> > rt_rq = rt_rq_of_se(rt_se);
> >
> > spin_lock(&rt_rq->rt_runtime_lock);
> > - rt_rq->rt_time += delta_exec;
> > - if (sched_rt_runtime_exceeded(rt_rq))
> > - resched_task(curr);
> > + if (sched_rt_runtime(rt_rq) != RUNTIME_INF) {
> > + rt_rq->rt_time += delta_exec;
> > + if (sched_rt_runtime_exceeded(rt_rq))
> > + resched_task(curr);
> > + }
> > spin_unlock(&rt_rq->rt_runtime_lock);
> > }
> > }
>
> This will make 'disabled' case more expensive, will it not ?
> I mean now we'll have to run balance_runtime() even if throttling is
> disabled.

It should not, its cheaper now. We should never end up in
balance_runtime as we'll never exceed and hit the throttle path.

> Do you guys mind if I make this stuff configurable ? ie Just like
> CONFIG_RT_GROUP_SCHED we could add CONFIG_RT_BANDWIDTH_THROTTLE.

Yeah - please don't do that, its ifdef fest in there - we really should
reduce the clutter, not add to it.