Re: [RFC v5 5/9] sched/deadline: do not reclaim the whole CPU bandwidth

[Peter Zijlstra]

On Fri, Mar 24, 2017 at 10:38:31PM -0400, Steven Rostedt wrote:

> > > > @@ -6716,6 +6716,12 @@ static void sched_dl_do_global(void)
> > > > raw_spin_unlock_irqrestore(_b->lock, flags);
> > > >  
> > > > rcu_read_unlock_sched();
> > > > +   if (dl_b->bw == -1)
> > > > +   cpu_rq(cpu)->dl.deadline_bw_inv = 1 << 8;
> > > > +   else
> > > > +   cpu_rq(cpu)->dl.deadline_bw_inv =
> > > > +   to_ratio(global_rt_runtime(),
> > > > +global_rt_period()) >>
> > > > 12;
> > > 
> > > Coding style requires braces here (on both legs of the condition)..  

> I'm not sure it's completely documented anywhere.

Two parts;

 1) I prefer braces over any multi line block, irrespective if its a
 single statement or not. This is, afaik, not strictly documented in
 coding style.

 Rationale is that missing braces are bad, and the larger the single
 statement, the harder it is to be sure it is in fact a single
 statement.


 2) If one leg needs braces, then both should get it. This is in fact
 part of CodingStyle.

Re: [RFC v5 5/9] sched/deadline: do not reclaim the whole CPU bandwidth

[Peter Zijlstra]

On Fri, Mar 24, 2017 at 10:38:31PM -0400, Steven Rostedt wrote:

> > > > @@ -6716,6 +6716,12 @@ static void sched_dl_do_global(void)
> > > > raw_spin_unlock_irqrestore(_b->lock, flags);
> > > >  
> > > > rcu_read_unlock_sched();
> > > > +   if (dl_b->bw == -1)
> > > > +   cpu_rq(cpu)->dl.deadline_bw_inv = 1 << 8;
> > > > +   else
> > > > +   cpu_rq(cpu)->dl.deadline_bw_inv =
> > > > +   to_ratio(global_rt_runtime(),
> > > > +global_rt_period()) >>
> > > > 12;
> > > 
> > > Coding style requires braces here (on both legs of the condition)..  

> I'm not sure it's completely documented anywhere.

Two parts;

 1) I prefer braces over any multi line block, irrespective if its a
 single statement or not. This is, afaik, not strictly documented in
 coding style.

 Rationale is that missing braces are bad, and the larger the single
 statement, the harder it is to be sure it is in fact a single
 statement.


 2) If one leg needs braces, then both should get it. This is in fact
 part of CodingStyle.

Re: [RFC v5 5/9] sched/deadline: do not reclaim the whole CPU bandwidth

[Steven Rostedt]

On Fri, 24 Mar 2017 22:58:27 +0100
luca abeni  wrote:

> Hi Peter,
> 
> On Fri, 24 Mar 2017 15:00:15 +0100
> Peter Zijlstra  wrote:
> 
> > On Fri, Mar 24, 2017 at 04:52:58AM +0100, luca abeni wrote:
> >   
> > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > > index 20c62e7..efa88eb 100644
> > > --- a/kernel/sched/core.c
> > > +++ b/kernel/sched/core.c
> > > @@ -6716,6 +6716,12 @@ static void sched_dl_do_global(void)
> > >   raw_spin_unlock_irqrestore(_b->lock, flags);
> > >  
> > >   rcu_read_unlock_sched();
> > > + if (dl_b->bw == -1)
> > > + cpu_rq(cpu)->dl.deadline_bw_inv = 1 << 8;
> > > + else
> > > + cpu_rq(cpu)->dl.deadline_bw_inv =
> > > + to_ratio(global_rt_runtime(),
> > > +  global_rt_period()) >>
> > > 12;
> > 
> > Coding style requires braces here (on both legs of the condition)..  
> 
> Sorry about this; checkpatch did not complain and I did not check the
> coding rules. I'll add the braces.

I'm not sure it's completely documented anywhere.

The brackets are not needed if there's one statement after the if, but
for readability, it's sometimes best to put brackets in if there's more
than one line. That can even include comments. It's not a hard rule,
but more of a preference. I'm personally OK with the above, but Peter
being the maintainer, has the say to give the preference of this kind
of rule.

-- Steve

Re: [RFC v5 5/9] sched/deadline: do not reclaim the whole CPU bandwidth

[Steven Rostedt]

On Fri, 24 Mar 2017 22:58:27 +0100
luca abeni  wrote:

> Hi Peter,
> 
> On Fri, 24 Mar 2017 15:00:15 +0100
> Peter Zijlstra  wrote:
> 
> > On Fri, Mar 24, 2017 at 04:52:58AM +0100, luca abeni wrote:
> >   
> > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > > index 20c62e7..efa88eb 100644
> > > --- a/kernel/sched/core.c
> > > +++ b/kernel/sched/core.c
> > > @@ -6716,6 +6716,12 @@ static void sched_dl_do_global(void)
> > >   raw_spin_unlock_irqrestore(_b->lock, flags);
> > >  
> > >   rcu_read_unlock_sched();
> > > + if (dl_b->bw == -1)
> > > + cpu_rq(cpu)->dl.deadline_bw_inv = 1 << 8;
> > > + else
> > > + cpu_rq(cpu)->dl.deadline_bw_inv =
> > > + to_ratio(global_rt_runtime(),
> > > +  global_rt_period()) >>
> > > 12;
> > 
> > Coding style requires braces here (on both legs of the condition)..  
> 
> Sorry about this; checkpatch did not complain and I did not check the
> coding rules. I'll add the braces.

I'm not sure it's completely documented anywhere.

The brackets are not needed if there's one statement after the if, but
for readability, it's sometimes best to put brackets in if there's more
than one line. That can even include comments. It's not a hard rule,
but more of a preference. I'm personally OK with the above, but Peter
being the maintainer, has the say to give the preference of this kind
of rule.

-- Steve

Re: [RFC v5 5/9] sched/deadline: do not reclaim the whole CPU bandwidth

[luca abeni]

Hi Peter,

On Fri, 24 Mar 2017 15:00:15 +0100
Peter Zijlstra  wrote:

> On Fri, Mar 24, 2017 at 04:52:58AM +0100, luca abeni wrote:
> 
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 20c62e7..efa88eb 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -6716,6 +6716,12 @@ static void sched_dl_do_global(void)
> > raw_spin_unlock_irqrestore(_b->lock, flags);
> >  
> > rcu_read_unlock_sched();
> > +   if (dl_b->bw == -1)
> > +   cpu_rq(cpu)->dl.deadline_bw_inv = 1 << 8;
> > +   else
> > +   cpu_rq(cpu)->dl.deadline_bw_inv =
> > +   to_ratio(global_rt_runtime(),
> > +global_rt_period()) >>
> > 12;  
> 
> Coding style requires braces here (on both legs of the condition)..

Sorry about this; checkpatch did not complain and I did not check the
coding rules. I'll add the braces.


> Also, I find deadline_bw_inv an awkward name; would something like
> bw_ratio or so be more accurate?

I am not good at finding names :)
(I used "deadline_bw_inv" because it represents the inverse of the
deadline tasks bandwidth")
I'll change the name in bw_ratio or something better (suggestions?) 


> > +   if (global_rt_runtime() == RUNTIME_INF)
> > +   dl_rq->deadline_bw_inv = 1 << 8;
> > +   else
> > +   dl_rq->deadline_bw_inv =
> > +   to_ratio(global_rt_runtime(),
> > global_rt_period()) >> 12;  
> 
> That's almost the same code; do we want a helper function?

OK, I'll look at this.


> >  u64 grub_reclaim(u64 delta, struct rq *rq)
> >  {
> > +   return (delta * rq->dl.running_bw *
> > rq->dl.deadline_bw_inv) >> 20 >> 8; }  
> 
> At which point we might want a note about how this doesn't overflow I
> suppose.

I'll add it on Monday.


> 
> Also:
> 
>   delta *= rq->dl.running_bw;
>   delta *= rq->dl.bw_ratio;
>   delta >>= 20 + 8;
> 
>   return delta;
> 
> Might be more readable ?
> 
> Alternatively:
> 
>   delta = (delta * rq->dl.running_bw) >> 8;
>   delta = (delta * rq->dl.bw_ratio) >> 20;
> 
>   return delta;
> 
> But I doubt we care about those extra 8 bit of space; delta should not
> be over 36 bits (~64 seconds) anyway I suppose.

I think the version with all the shifts after the multiplications is
more precise, right?


Thanks,
Luca

Re: [RFC v5 5/9] sched/deadline: do not reclaim the whole CPU bandwidth

[luca abeni]

Hi Peter,

On Fri, 24 Mar 2017 15:00:15 +0100
Peter Zijlstra  wrote:

> On Fri, Mar 24, 2017 at 04:52:58AM +0100, luca abeni wrote:
> 
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 20c62e7..efa88eb 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -6716,6 +6716,12 @@ static void sched_dl_do_global(void)
> > raw_spin_unlock_irqrestore(_b->lock, flags);
> >  
> > rcu_read_unlock_sched();
> > +   if (dl_b->bw == -1)
> > +   cpu_rq(cpu)->dl.deadline_bw_inv = 1 << 8;
> > +   else
> > +   cpu_rq(cpu)->dl.deadline_bw_inv =
> > +   to_ratio(global_rt_runtime(),
> > +global_rt_period()) >>
> > 12;  
> 
> Coding style requires braces here (on both legs of the condition)..

Sorry about this; checkpatch did not complain and I did not check the
coding rules. I'll add the braces.


> Also, I find deadline_bw_inv an awkward name; would something like
> bw_ratio or so be more accurate?

I am not good at finding names :)
(I used "deadline_bw_inv" because it represents the inverse of the
deadline tasks bandwidth")
I'll change the name in bw_ratio or something better (suggestions?) 


> > +   if (global_rt_runtime() == RUNTIME_INF)
> > +   dl_rq->deadline_bw_inv = 1 << 8;
> > +   else
> > +   dl_rq->deadline_bw_inv =
> > +   to_ratio(global_rt_runtime(),
> > global_rt_period()) >> 12;  
> 
> That's almost the same code; do we want a helper function?

OK, I'll look at this.


> >  u64 grub_reclaim(u64 delta, struct rq *rq)
> >  {
> > +   return (delta * rq->dl.running_bw *
> > rq->dl.deadline_bw_inv) >> 20 >> 8; }  
> 
> At which point we might want a note about how this doesn't overflow I
> suppose.

I'll add it on Monday.


> 
> Also:
> 
>   delta *= rq->dl.running_bw;
>   delta *= rq->dl.bw_ratio;
>   delta >>= 20 + 8;
> 
>   return delta;
> 
> Might be more readable ?
> 
> Alternatively:
> 
>   delta = (delta * rq->dl.running_bw) >> 8;
>   delta = (delta * rq->dl.bw_ratio) >> 20;
> 
>   return delta;
> 
> But I doubt we care about those extra 8 bit of space; delta should not
> be over 36 bits (~64 seconds) anyway I suppose.

I think the version with all the shifts after the multiplications is
more precise, right?


Thanks,
Luca

Re: [RFC v5 5/9] sched/deadline: do not reclaim the whole CPU bandwidth

[Peter Zijlstra]

On Fri, Mar 24, 2017 at 04:52:58AM +0100, luca abeni wrote:

> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 20c62e7..efa88eb 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -6716,6 +6716,12 @@ static void sched_dl_do_global(void)
>   raw_spin_unlock_irqrestore(_b->lock, flags);
>  
>   rcu_read_unlock_sched();
> + if (dl_b->bw == -1)
> + cpu_rq(cpu)->dl.deadline_bw_inv = 1 << 8;
> + else
> + cpu_rq(cpu)->dl.deadline_bw_inv =
> + to_ratio(global_rt_runtime(),
> +  global_rt_period()) >> 12;

Coding style requires braces here (on both legs of the condition)..

Also, I find deadline_bw_inv an awkward name; would something like
bw_ratio or so be more accurate?

> + if (global_rt_runtime() == RUNTIME_INF)
> + dl_rq->deadline_bw_inv = 1 << 8;
> + else
> + dl_rq->deadline_bw_inv =
> + to_ratio(global_rt_runtime(), global_rt_period()) >> 12;

That's almost the same code; do we want a helper function?

>  
>  u64 grub_reclaim(u64 delta, struct rq *rq)
>  {
> + return (delta * rq->dl.running_bw * rq->dl.deadline_bw_inv) >> 20 >> 8;
>  }

At which point we might want a note about how this doesn't overflow I
suppose.

Also:

delta *= rq->dl.running_bw;
delta *= rq->dl.bw_ratio;
delta >>= 20 + 8;

return delta;

Might be more readable ?

Alternatively:

delta = (delta * rq->dl.running_bw) >> 8;
delta = (delta * rq->dl.bw_ratio) >> 20;

return delta;

But I doubt we care about those extra 8 bit of space; delta should not
be over 36 bits (~64 seconds) anyway I suppose.

Re: [RFC v5 5/9] sched/deadline: do not reclaim the whole CPU bandwidth

[Peter Zijlstra]

On Fri, Mar 24, 2017 at 04:52:58AM +0100, luca abeni wrote:

> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 20c62e7..efa88eb 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -6716,6 +6716,12 @@ static void sched_dl_do_global(void)
>   raw_spin_unlock_irqrestore(_b->lock, flags);
>  
>   rcu_read_unlock_sched();
> + if (dl_b->bw == -1)
> + cpu_rq(cpu)->dl.deadline_bw_inv = 1 << 8;
> + else
> + cpu_rq(cpu)->dl.deadline_bw_inv =
> + to_ratio(global_rt_runtime(),
> +  global_rt_period()) >> 12;

Coding style requires braces here (on both legs of the condition)..

Also, I find deadline_bw_inv an awkward name; would something like
bw_ratio or so be more accurate?

> + if (global_rt_runtime() == RUNTIME_INF)
> + dl_rq->deadline_bw_inv = 1 << 8;
> + else
> + dl_rq->deadline_bw_inv =
> + to_ratio(global_rt_runtime(), global_rt_period()) >> 12;

That's almost the same code; do we want a helper function?

>  
>  u64 grub_reclaim(u64 delta, struct rq *rq)
>  {
> + return (delta * rq->dl.running_bw * rq->dl.deadline_bw_inv) >> 20 >> 8;
>  }

At which point we might want a note about how this doesn't overflow I
suppose.

Also:

delta *= rq->dl.running_bw;
delta *= rq->dl.bw_ratio;
delta >>= 20 + 8;

return delta;

Might be more readable ?

Alternatively:

delta = (delta * rq->dl.running_bw) >> 8;
delta = (delta * rq->dl.bw_ratio) >> 20;

return delta;

But I doubt we care about those extra 8 bit of space; delta should not
be over 36 bits (~64 seconds) anyway I suppose.

[RFC v5 5/9] sched/deadline: do not reclaim the whole CPU bandwidth

[luca abeni]

From: Luca Abeni 

Original GRUB tends to reclaim 100% of the CPU time... And this
allows a CPU hog to starve non-deadline tasks.
To address this issue, allow the scheduler to reclaim only a
specified fraction of CPU time.

Signed-off-by: Luca Abeni 
Tested-by: Daniel Bristot de Oliveira 
---
 kernel/sched/core.c | 6 ++
 kernel/sched/deadline.c | 7 ++-
 kernel/sched/sched.h| 6 ++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 20c62e7..efa88eb 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6716,6 +6716,12 @@ static void sched_dl_do_global(void)
raw_spin_unlock_irqrestore(_b->lock, flags);
 
rcu_read_unlock_sched();
+   if (dl_b->bw == -1)
+   cpu_rq(cpu)->dl.deadline_bw_inv = 1 << 8;
+   else
+   cpu_rq(cpu)->dl.deadline_bw_inv =
+   to_ratio(global_rt_runtime(),
+global_rt_period()) >> 12;
}
 }
 
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 6035311..e964051 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -212,6 +212,11 @@ void init_dl_rq(struct dl_rq *dl_rq)
 #else
init_dl_bw(_rq->dl_bw);
 #endif
+   if (global_rt_runtime() == RUNTIME_INF)
+   dl_rq->deadline_bw_inv = 1 << 8;
+   else
+   dl_rq->deadline_bw_inv =
+   to_ratio(global_rt_runtime(), global_rt_period()) >> 12;
 }
 
 #ifdef CONFIG_SMP
@@ -871,7 +876,7 @@ extern bool sched_rt_bandwidth_account(struct rt_rq *rt_rq);
  */
 u64 grub_reclaim(u64 delta, struct rq *rq)
 {
-   return (delta * rq->dl.running_bw) >> 20;
+   return (delta * rq->dl.running_bw * rq->dl.deadline_bw_inv) >> 20 >> 8;
 }
 
 /*
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 57bb79b..141549b 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -565,6 +565,12 @@ struct dl_rq {
 * task blocks
 */
u64 running_bw;
+
+   /*
+* Inverse of the fraction of CPU utilization that can be reclaimed
+* by the GRUB algorithm.
+*/
+   u64 deadline_bw_inv;
 };
 
 #ifdef CONFIG_SMP
-- 
2.7.4

[RFC v5 5/9] sched/deadline: do not reclaim the whole CPU bandwidth

[luca abeni]

From: Luca Abeni 

Original GRUB tends to reclaim 100% of the CPU time... And this
allows a CPU hog to starve non-deadline tasks.
To address this issue, allow the scheduler to reclaim only a
specified fraction of CPU time.

Signed-off-by: Luca Abeni 
Tested-by: Daniel Bristot de Oliveira 
---
 kernel/sched/core.c | 6 ++
 kernel/sched/deadline.c | 7 ++-
 kernel/sched/sched.h| 6 ++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 20c62e7..efa88eb 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6716,6 +6716,12 @@ static void sched_dl_do_global(void)
raw_spin_unlock_irqrestore(_b->lock, flags);
 
rcu_read_unlock_sched();
+   if (dl_b->bw == -1)
+   cpu_rq(cpu)->dl.deadline_bw_inv = 1 << 8;
+   else
+   cpu_rq(cpu)->dl.deadline_bw_inv =
+   to_ratio(global_rt_runtime(),
+global_rt_period()) >> 12;
}
 }
 
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 6035311..e964051 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -212,6 +212,11 @@ void init_dl_rq(struct dl_rq *dl_rq)
 #else
init_dl_bw(_rq->dl_bw);
 #endif
+   if (global_rt_runtime() == RUNTIME_INF)
+   dl_rq->deadline_bw_inv = 1 << 8;
+   else
+   dl_rq->deadline_bw_inv =
+   to_ratio(global_rt_runtime(), global_rt_period()) >> 12;
 }
 
 #ifdef CONFIG_SMP
@@ -871,7 +876,7 @@ extern bool sched_rt_bandwidth_account(struct rt_rq *rt_rq);
  */
 u64 grub_reclaim(u64 delta, struct rq *rq)
 {
-   return (delta * rq->dl.running_bw) >> 20;
+   return (delta * rq->dl.running_bw * rq->dl.deadline_bw_inv) >> 20 >> 8;
 }
 
 /*
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 57bb79b..141549b 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -565,6 +565,12 @@ struct dl_rq {
 * task blocks
 */
u64 running_bw;
+
+   /*
+* Inverse of the fraction of CPU utilization that can be reclaimed
+* by the GRUB algorithm.
+*/
+   u64 deadline_bw_inv;
 };
 
 #ifdef CONFIG_SMP
-- 
2.7.4