Re: [RFC v5 5/9] sched/deadline: do not reclaim the whole CPU bandwidth
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(&dl_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
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(&dl_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
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(&dl_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
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(&dl_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.
