| Message ID | 1467227263-31349-1-git-send-email-juri.lelli@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
On 04/07/16 11:03, Luca Abeni wrote: > Hi Juri, > > On Wed, 29 Jun 2016 20:07:43 +0100 > Juri Lelli <juri.lelli@arm.com> wrote: > > > setup_new_dl_entity() takes two parameters, but it only actually uses > > one of them, under a different name, to setup a new dl_entity, after: > > > > 2f9f3fdc928 "sched/deadline: Remove dl_new from struct > > sched_dl_entity" > > > > as we currently do > > > > setup_new_dl_entity(&p->dl, &p->dl) > > > > However, before Luca's change we were doing > > > > setup_new_dl_entity(dl_se, pi_se) > > > > in update_dl_entity() for a dl_se->new entity: we were using pi_se's > > parameters (the potential PI donor) for setting up a new entity. > > > > Restore this behaviour (as we want to correctly initialize parameters > > of a boosted task that enters DEADLINE) by removing the useless second > > parameter of setup_new_dl_entity() and retrieving the top waiter > > directly from inside that function. > I did not have time to test this patch yet, but it still looks good to > me. > Thanks for reviewing it. Best, - Juri
On 05/07/16 15:37, Wanpeng Li wrote: > 2016-06-30 3:07 GMT+08:00 Juri Lelli <juri.lelli@arm.com>: > > setup_new_dl_entity() takes two parameters, but it only actually uses > > one of them, under a different name, to setup a new dl_entity, after: > > > > 2f9f3fdc928 "sched/deadline: Remove dl_new from struct sched_dl_entity" > > > > as we currently do > > > > setup_new_dl_entity(&p->dl, &p->dl) > > > > However, before Luca's change we were doing > > > > setup_new_dl_entity(dl_se, pi_se) > > > > in update_dl_entity() for a dl_se->new entity: we were using pi_se's > > parameters (the potential PI donor) for setting up a new entity. > > > > Restore this behaviour (as we want to correctly initialize parameters of > > a boosted task that enters DEADLINE) by removing the useless second > > parameter of setup_new_dl_entity() and retrieving the top waiter > > directly from inside that function. > > > > Cc: Ingo Molnar <mingo@redhat.com> > > Cc: Peter Zijlstra <peterz@infradead.org> > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: Luca Abeni <luca.abeni@unitn.it> > > Signed-off-by: Juri Lelli <juri.lelli@arm.com> > > > > Reviewed-by: Wanpeng Li <wanpeng.li@hotmail.com> > Thanks! - Juri
On 05/07/16 10:20, Steven Rostedt wrote: > On Wed, 29 Jun 2016 20:07:43 +0100 > Juri Lelli <juri.lelli@arm.com> wrote: > > > > --- > > kernel/sched/deadline.c | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c > > index fcb7f0217ff4..2000ad2294d5 100644 > > --- a/kernel/sched/deadline.c > > +++ b/kernel/sched/deadline.c > > @@ -346,11 +346,12 @@ static void check_preempt_curr_dl(struct rq *rq, struct task_struct *p, > > * one, and to (try to!) reconcile itself with its own scheduling > > * parameters. > > */ > > -static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se, > > - struct sched_dl_entity *pi_se) > > +static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se) > > { > > struct dl_rq *dl_rq = dl_rq_of_se(dl_se); > > struct rq *rq = rq_of_dl_rq(dl_rq); > > + struct task_struct *pi_task = rt_mutex_get_top_task(dl_task_of(dl_se)); > > + struct sched_dl_entity *pi_se = dl_se; > > > > WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline)); > > > > @@ -363,6 +364,13 @@ static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se, > > return; > > > > /* > > + * Use the scheduling parameters of the top pi-waiter task, > > + * if we have one from which we can inherit a deadline. > > + */ > > + if (pi_task && dl_se->dl_boosted && dl_prio(pi_task->normal_prio)) > > + pi_se = &pi_task->dl; > > + > > OK, I'm micro-optimizing now, but hey, isn't this a fast path? > > What about changing the above to: > > struct task_struct *pi_task; > [...] > > if (dl_se->dl_boosted && dl_prio(pi_task->normal_prio && ^ OK, we need to reorder these two V > (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se))) > pe_se = &pi_task->dl; > > This way we don't need to do any work of looking at > rt_mutex_get_top_task() for the normal case. > But, yes. Looks good to me. I'll shoot a v3 ASAP. Thanks, - Juri
On 05/07/16 12:47, Steven Rostedt wrote: > On Tue, 5 Jul 2016 15:39:33 +0100 > Juri Lelli <juri.lelli@arm.com> wrote: > > return; > > > > > > > > /* > > > > + * Use the scheduling parameters of the top pi-waiter task, > > > > + * if we have one from which we can inherit a deadline. > > > > + */ > > > > + if (pi_task && dl_se->dl_boosted && dl_prio(pi_task->normal_prio)) > > > > + pi_se = &pi_task->dl; > > > > + > > > > > > OK, I'm micro-optimizing now, but hey, isn't this a fast path? > > > > > > What about changing the above to: > > > > > > struct task_struct *pi_task; > > > [...] > > > > > > if (dl_se->dl_boosted && dl_prio(pi_task->normal_prio && > > ^ > > OK, we need to reorder these two > > V > > > (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se))) > > > pe_se = &pi_task->dl; > > Opps, you're right. > > > > > > > This way we don't need to do any work of looking at > > > rt_mutex_get_top_task() for the normal case. > > > > > > > But, yes. Looks good to me. I'll shoot a v3 ASAP. > > I have to ask, should there be any check if the dl_se has a shorter > deadline than the pi one? > Yeah. I wondered the same actually. I convinced myself that, since the task is boosted, we assume that the donor will have a shorter deadline. We seem to be doing the same elsewhere, but Luca was saying some time ago that the DI thing my have some problems and needs to be revised. Is is fair enough fixing this bit in accordance with the current (maybe broken) behaviour and then spend time reviewing the whole thing, or do we want to do both at the same time (which will of course require more time)? Best, - Juri
On 06/07/16 10:44, Luca Abeni wrote: > On Tue, 5 Jul 2016 17:58:30 +0100 > Juri Lelli <juri.lelli@arm.com> wrote: > > > On 05/07/16 12:47, Steven Rostedt wrote: > > > On Tue, 5 Jul 2016 15:39:33 +0100 > > > Juri Lelli <juri.lelli@arm.com> wrote: > > > > > > return; > > > > > > > > > > > > /* > > > > > > + * Use the scheduling parameters of the top > > > > > > pi-waiter task, > > > > > > + * if we have one from which we can inherit a > > > > > > deadline. > > > > > > + */ > > > > > > + if (pi_task && dl_se->dl_boosted && > > > > > > dl_prio(pi_task->normal_prio)) > > > > > > + pi_se = &pi_task->dl; > > > > > > + > > > > > > > > > > OK, I'm micro-optimizing now, but hey, isn't this a fast path? > > > > > > > > > > What about changing the above to: > > > > > > > > > > struct task_struct *pi_task; > > > > > [...] > > > > > > > > > > if (dl_se->dl_boosted && dl_prio(pi_task->normal_prio > > > > > && > > > > ^ > > > > OK, we need to reorder these two > > > > V > > > > > (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se))) > > > > > pe_se = &pi_task->dl; > > > > > > Opps, you're right. > > > > > > > > > > > > > This way we don't need to do any work of looking at > > > > > rt_mutex_get_top_task() for the normal case. > > > > > > > > > > > > > But, yes. Looks good to me. I'll shoot a v3 ASAP. > > > > > > I have to ask, should there be any check if the dl_se has a shorter > > > deadline than the pi one? > > > > > > > Yeah. I wondered the same actually. I convinced myself that, since the > > task is boosted, we assume that the donor will have a shorter > > deadline. > > Do you mean relative deadline (dl_se->dl_deadline) or absolute > (scheduling) dealine (dl_se->deadline)? > > If I understand well, here we are in setup_new_dl_entity(), right? > This should be called only from switched_to_dl(); so, dl_se is from a > task that is switching to -deadline. If it is dl_boosted, it means that > it is switching from SCHED_OTHER (or RT) to -deadline because of > inheritance... So, it is very likely that dl_se->dl_deadline is not > meaningful. > Right, very same thought I also had (and forgot to mention). So, we cannot really do here the check Steve was wondering about. > Moreover, setup_new_dl_entity() is only called if the current > scheduling deadline of the task is not usable (that is, if > "dl_time_before(p->dl.deadline, rq_clock(rq)"). So, dl_se->deadline > will be surely smaller than pi_se->deadline... But the inheritance has > to happen anyway. > > > > We seem to be doing the same elsewhere, but Luca was saying > > some time ago that the DI thing my have some problems and needs to be > > revised. > > My doubts regarding the inheritance code currently used for -deadline > tasks are due to the fact that it is not clear which kind of > inheritance algorithm is used... > I think it should use deadline inheritance, that, AFAIK, says that when > task T1 block waiting for task T2, T2 can inherit T1's _absolute_ > deadline - if it is earlier than T2's one. > But the current code seems to be using relative deadlines (dl_deadline) > to decide the inheritance... > True. Problem is however that, even if enforcing is disabled for a boosted task, we keep postponing the task's deadline when it depletes its runtime (soft-CBS). So, which one should we use to do so? At the instant of time the task gets a new potential donor this donor might have a shorter absolute deadline. But the task's relative deadline might be better (shorter w.r.t. donor's relative) to postpone the absolute one when needed. > Having a better look at this is in my TODO list... But I still need to > find some time :) > Same here. No spare cycles right now to have a thorough look at this. :( Thanks, - Juri
On 07/07/16 09:47, Steven Rostedt wrote: > On Thu, 7 Jul 2016 09:39:09 +0100 > Juri Lelli <juri.lelli@arm.com> wrote: > > > > > Having a better look at this is in my TODO list... But I still need to > > > find some time :) > > > > > > > Same here. No spare cycles right now to have a thorough look at this. :( > > All in all, this should not hold up the current patch set. Maybe mark > it with a "TODO" and try to remember to look into it at another time. Added to my TODO list (right after reviewing Luca's reclaiming bits next version and fix the cpuset issue :/) and posted v3. > The chances that we are switching from a SCHED_OTHER/RT to DEADLINE > that is already boosted is extremely rare. It can happen, but it's not > critical enough to hold this up. > Thanks, - Juri
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c index fcb7f0217ff4..2000ad2294d5 100644 --- a/kernel/sched/deadline.c +++ b/kernel/sched/deadline.c @@ -346,11 +346,12 @@ static void check_preempt_curr_dl(struct rq *rq, struct task_struct *p, * one, and to (try to!) reconcile itself with its own scheduling * parameters. */ -static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se, - struct sched_dl_entity *pi_se) +static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se) { struct dl_rq *dl_rq = dl_rq_of_se(dl_se); struct rq *rq = rq_of_dl_rq(dl_rq); + struct task_struct *pi_task = rt_mutex_get_top_task(dl_task_of(dl_se)); + struct sched_dl_entity *pi_se = dl_se; WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline)); @@ -363,6 +364,13 @@ static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se, return; /* + * Use the scheduling parameters of the top pi-waiter task, + * if we have one from which we can inherit a deadline. + */ + if (pi_task && dl_se->dl_boosted && dl_prio(pi_task->normal_prio)) + pi_se = &pi_task->dl; + + /* * We use the regular wall clock time to set deadlines in the * future; in fact, we must consider execution overheads (time * spent on hardirq context, etc.). @@ -1721,7 +1729,7 @@ static void switched_from_dl(struct rq *rq, struct task_struct *p) static void switched_to_dl(struct rq *rq, struct task_struct *p) { if (dl_time_before(p->dl.deadline, rq_clock(rq))) - setup_new_dl_entity(&p->dl, &p->dl); + setup_new_dl_entity(&p->dl); if (task_on_rq_queued(p) && rq->curr != p) { #ifdef CONFIG_SMP
setup_new_dl_entity() takes two parameters, but it only actually uses one of them, under a different name, to setup a new dl_entity, after: 2f9f3fdc928 "sched/deadline: Remove dl_new from struct sched_dl_entity" as we currently do setup_new_dl_entity(&p->dl, &p->dl) However, before Luca's change we were doing setup_new_dl_entity(dl_se, pi_se) in update_dl_entity() for a dl_se->new entity: we were using pi_se's parameters (the potential PI donor) for setting up a new entity. Restore this behaviour (as we want to correctly initialize parameters of a boosted task that enters DEADLINE) by removing the useless second parameter of setup_new_dl_entity() and retrieving the top waiter directly from inside that function. Cc: Ingo Molnar <mingo@redhat.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Luca Abeni <luca.abeni@unitn.it> Signed-off-by: Juri Lelli <juri.lelli@arm.com> --- Changes from v1: - Steve pointed out that we were actually using the second parameter to permorm initialization - Luca confirmed that behavior is slightly changed w.r.t. before his change - changelog updated and original behavior restored --- kernel/sched/deadline.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) -- 2.7.0