Re: [PATCH v3] sched/deadline: remove useless param from setup_new_dl_entity
On 11/07/16 16:16, Xunlei Pang wrote: > On 2016/07/11 at 16:01, luca abeni wrote: > > Hello, > > > > On Mon, 11 Jul 2016 13:03:56 +0800 > > Xunlei Pangwrote: > > > >> On 2016/07/08 at 19:28, Juri Lelli wrote: > > [...] > >>> @@ -363,6 +364,15 @@ 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 (dl_se->dl_boosted && > >>> + (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se))) && > >>> + dl_prio(pi_task->normal_prio)) > >>> + pi_se = _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 +1731,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(>dl, >dl); > >>> + setup_new_dl_entity(>dl); > >> I'm curious why we even call setup_new_dl_entity() for non-queued > >> cases? It seems more reasonable to do it when it really gets queued. > >> We can see that enqueue_task_dl()->update_dl_entity() also has the > >> same update logic as switched_to_dl(). > > I wondered the same when removing the dl_new field from > > sched_dl_entity... But then I realised that enqueue_dl_entity() does > > not always invoke update_dl_entity() or replenish_dl_entity()... For > > example, when a task switches from SCHED_OTHER (or RT) to -deadline due > > to sched_setattr() (or similar) these functions are not invoked. > > Yeah, but for wake-up cases it does, as ENQUEUE_WAKEUP is set. > What I meant is, can we only update for queued tasks in switched_to_dl()? > Looks sensible to do. I think we can use the already present task_on_rq_queued() check in there. I'll put together a v4 ASAP (if Luca doesn't have already something). Thanks, - Juri
Re: [PATCH v3] sched/deadline: remove useless param from setup_new_dl_entity
On 11/07/16 16:16, Xunlei Pang wrote: > On 2016/07/11 at 16:01, luca abeni wrote: > > Hello, > > > > On Mon, 11 Jul 2016 13:03:56 +0800 > > Xunlei Pang wrote: > > > >> On 2016/07/08 at 19:28, Juri Lelli wrote: > > [...] > >>> @@ -363,6 +364,15 @@ 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 (dl_se->dl_boosted && > >>> + (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se))) && > >>> + dl_prio(pi_task->normal_prio)) > >>> + pi_se = _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 +1731,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(>dl, >dl); > >>> + setup_new_dl_entity(>dl); > >> I'm curious why we even call setup_new_dl_entity() for non-queued > >> cases? It seems more reasonable to do it when it really gets queued. > >> We can see that enqueue_task_dl()->update_dl_entity() also has the > >> same update logic as switched_to_dl(). > > I wondered the same when removing the dl_new field from > > sched_dl_entity... But then I realised that enqueue_dl_entity() does > > not always invoke update_dl_entity() or replenish_dl_entity()... For > > example, when a task switches from SCHED_OTHER (or RT) to -deadline due > > to sched_setattr() (or similar) these functions are not invoked. > > Yeah, but for wake-up cases it does, as ENQUEUE_WAKEUP is set. > What I meant is, can we only update for queued tasks in switched_to_dl()? > Looks sensible to do. I think we can use the already present task_on_rq_queued() check in there. I'll put together a v4 ASAP (if Luca doesn't have already something). Thanks, - Juri
Re: [PATCH v3] sched/deadline: remove useless param from setup_new_dl_entity
On Mon, 11 Jul 2016 16:16:20 +0800 Xunlei Pangwrote: > On 2016/07/11 at 16:01, luca abeni wrote: > > Hello, > > > > On Mon, 11 Jul 2016 13:03:56 +0800 > > Xunlei Pang wrote: > > > >> On 2016/07/08 at 19:28, Juri Lelli wrote: > > [...] > >>> @@ -363,6 +364,15 @@ 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 (dl_se->dl_boosted && > >>> + (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se))) > >>> && > >>> + dl_prio(pi_task->normal_prio)) > >>> + pi_se = _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 +1731,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(>dl, >dl); > >>> + setup_new_dl_entity(>dl); > >> I'm curious why we even call setup_new_dl_entity() for non-queued > >> cases? It seems more reasonable to do it when it really gets > >> queued. We can see that enqueue_task_dl()->update_dl_entity() also > >> has the same update logic as switched_to_dl(). > > I wondered the same when removing the dl_new field from > > sched_dl_entity... But then I realised that enqueue_dl_entity() does > > not always invoke update_dl_entity() or replenish_dl_entity()... For > > example, when a task switches from SCHED_OTHER (or RT) to -deadline > > due to sched_setattr() (or similar) these functions are not > > invoked. > > Yeah, but for wake-up cases it does, as ENQUEUE_WAKEUP is set. > What I meant is, can we only update for queued tasks in > switched_to_dl()? Uhm... Looking at this code again, I am not sure anymore about it... (switched_to_dl() is invoked after enqueue enqueue_task_dl()?) Maybe the best thing to do is to move the "if (dl_time_before(p->dl.deadline, rq_clock(rq)))" check in enqueue_dl_entity() (Juri, what do you think)? I'll try this approach in the next days. Thanks, Luca
Re: [PATCH v3] sched/deadline: remove useless param from setup_new_dl_entity
On Mon, 11 Jul 2016 16:16:20 +0800 Xunlei Pang wrote: > On 2016/07/11 at 16:01, luca abeni wrote: > > Hello, > > > > On Mon, 11 Jul 2016 13:03:56 +0800 > > Xunlei Pang wrote: > > > >> On 2016/07/08 at 19:28, Juri Lelli wrote: > > [...] > >>> @@ -363,6 +364,15 @@ 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 (dl_se->dl_boosted && > >>> + (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se))) > >>> && > >>> + dl_prio(pi_task->normal_prio)) > >>> + pi_se = _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 +1731,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(>dl, >dl); > >>> + setup_new_dl_entity(>dl); > >> I'm curious why we even call setup_new_dl_entity() for non-queued > >> cases? It seems more reasonable to do it when it really gets > >> queued. We can see that enqueue_task_dl()->update_dl_entity() also > >> has the same update logic as switched_to_dl(). > > I wondered the same when removing the dl_new field from > > sched_dl_entity... But then I realised that enqueue_dl_entity() does > > not always invoke update_dl_entity() or replenish_dl_entity()... For > > example, when a task switches from SCHED_OTHER (or RT) to -deadline > > due to sched_setattr() (or similar) these functions are not > > invoked. > > Yeah, but for wake-up cases it does, as ENQUEUE_WAKEUP is set. > What I meant is, can we only update for queued tasks in > switched_to_dl()? Uhm... Looking at this code again, I am not sure anymore about it... (switched_to_dl() is invoked after enqueue enqueue_task_dl()?) Maybe the best thing to do is to move the "if (dl_time_before(p->dl.deadline, rq_clock(rq)))" check in enqueue_dl_entity() (Juri, what do you think)? I'll try this approach in the next days. Thanks, Luca
Re: [PATCH v3] sched/deadline: remove useless param from setup_new_dl_entity
On 2016/07/11 at 16:01, luca abeni wrote: > Hello, > > On Mon, 11 Jul 2016 13:03:56 +0800 > Xunlei Pangwrote: > >> On 2016/07/08 at 19:28, Juri Lelli wrote: > [...] >>> @@ -363,6 +364,15 @@ 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 (dl_se->dl_boosted && >>> + (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se))) && >>> + dl_prio(pi_task->normal_prio)) >>> + pi_se = _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 +1731,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(>dl, >dl); >>> + setup_new_dl_entity(>dl); >> I'm curious why we even call setup_new_dl_entity() for non-queued >> cases? It seems more reasonable to do it when it really gets queued. >> We can see that enqueue_task_dl()->update_dl_entity() also has the >> same update logic as switched_to_dl(). > I wondered the same when removing the dl_new field from > sched_dl_entity... But then I realised that enqueue_dl_entity() does > not always invoke update_dl_entity() or replenish_dl_entity()... For > example, when a task switches from SCHED_OTHER (or RT) to -deadline due > to sched_setattr() (or similar) these functions are not invoked. Yeah, but for wake-up cases it does, as ENQUEUE_WAKEUP is set. What I meant is, can we only update for queued tasks in switched_to_dl()? Regards, Xunlei > > > Luca > >> If so, for already queued and boosted cases, rt_mutex_setprio() will >> call enqueue_task() with ENQUEUE_REPLENISH set, so enqueue_dl_entity() >> ->replenish_dl_entity() will advance p->dl.deadline beforehand, see >> code: replenish_dl_entity(): >> if (dl_se->dl_deadline == 0) { >> dl_se->deadline = rq_clock(rq) + pi_se->dl_deadline; >> dl_se->runtime = pi_se->dl_runtime; >> } >> >> IOW, we don't need to handle !dl boosted cases in >> setup_new_dl_entity(). >> >> Regards, >> Xunlei >> >>> >>> if (task_on_rq_queued(p) && rq->curr != p) { >>> #ifdef CONFIG_SMP
Re: [PATCH v3] sched/deadline: remove useless param from setup_new_dl_entity
On 2016/07/11 at 16:01, luca abeni wrote: > Hello, > > On Mon, 11 Jul 2016 13:03:56 +0800 > Xunlei Pang wrote: > >> On 2016/07/08 at 19:28, Juri Lelli wrote: > [...] >>> @@ -363,6 +364,15 @@ 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 (dl_se->dl_boosted && >>> + (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se))) && >>> + dl_prio(pi_task->normal_prio)) >>> + pi_se = _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 +1731,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(>dl, >dl); >>> + setup_new_dl_entity(>dl); >> I'm curious why we even call setup_new_dl_entity() for non-queued >> cases? It seems more reasonable to do it when it really gets queued. >> We can see that enqueue_task_dl()->update_dl_entity() also has the >> same update logic as switched_to_dl(). > I wondered the same when removing the dl_new field from > sched_dl_entity... But then I realised that enqueue_dl_entity() does > not always invoke update_dl_entity() or replenish_dl_entity()... For > example, when a task switches from SCHED_OTHER (or RT) to -deadline due > to sched_setattr() (or similar) these functions are not invoked. Yeah, but for wake-up cases it does, as ENQUEUE_WAKEUP is set. What I meant is, can we only update for queued tasks in switched_to_dl()? Regards, Xunlei > > > Luca > >> If so, for already queued and boosted cases, rt_mutex_setprio() will >> call enqueue_task() with ENQUEUE_REPLENISH set, so enqueue_dl_entity() >> ->replenish_dl_entity() will advance p->dl.deadline beforehand, see >> code: replenish_dl_entity(): >> if (dl_se->dl_deadline == 0) { >> dl_se->deadline = rq_clock(rq) + pi_se->dl_deadline; >> dl_se->runtime = pi_se->dl_runtime; >> } >> >> IOW, we don't need to handle !dl boosted cases in >> setup_new_dl_entity(). >> >> Regards, >> Xunlei >> >>> >>> if (task_on_rq_queued(p) && rq->curr != p) { >>> #ifdef CONFIG_SMP
Re: [PATCH v3] sched/deadline: remove useless param from setup_new_dl_entity
Hello, On Mon, 11 Jul 2016 13:03:56 +0800 Xunlei Pangwrote: > On 2016/07/08 at 19:28, Juri Lelli wrote: [...] > > @@ -363,6 +364,15 @@ 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 (dl_se->dl_boosted && > > + (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se))) && > > + dl_prio(pi_task->normal_prio)) > > + pi_se = _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 +1731,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(>dl, >dl); > > + setup_new_dl_entity(>dl); > > I'm curious why we even call setup_new_dl_entity() for non-queued > cases? It seems more reasonable to do it when it really gets queued. > We can see that enqueue_task_dl()->update_dl_entity() also has the > same update logic as switched_to_dl(). I wondered the same when removing the dl_new field from sched_dl_entity... But then I realised that enqueue_dl_entity() does not always invoke update_dl_entity() or replenish_dl_entity()... For example, when a task switches from SCHED_OTHER (or RT) to -deadline due to sched_setattr() (or similar) these functions are not invoked. Luca > If so, for already queued and boosted cases, rt_mutex_setprio() will > call enqueue_task() with ENQUEUE_REPLENISH set, so enqueue_dl_entity() > ->replenish_dl_entity() will advance p->dl.deadline beforehand, see > code: replenish_dl_entity(): > if (dl_se->dl_deadline == 0) { > dl_se->deadline = rq_clock(rq) + pi_se->dl_deadline; > dl_se->runtime = pi_se->dl_runtime; > } > > IOW, we don't need to handle !dl boosted cases in > setup_new_dl_entity(). > > Regards, > Xunlei > > > > > if (task_on_rq_queued(p) && rq->curr != p) { > > #ifdef CONFIG_SMP >
Re: [PATCH v3] sched/deadline: remove useless param from setup_new_dl_entity
Hello, On Mon, 11 Jul 2016 13:03:56 +0800 Xunlei Pang wrote: > On 2016/07/08 at 19:28, Juri Lelli wrote: [...] > > @@ -363,6 +364,15 @@ 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 (dl_se->dl_boosted && > > + (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se))) && > > + dl_prio(pi_task->normal_prio)) > > + pi_se = _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 +1731,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(>dl, >dl); > > + setup_new_dl_entity(>dl); > > I'm curious why we even call setup_new_dl_entity() for non-queued > cases? It seems more reasonable to do it when it really gets queued. > We can see that enqueue_task_dl()->update_dl_entity() also has the > same update logic as switched_to_dl(). I wondered the same when removing the dl_new field from sched_dl_entity... But then I realised that enqueue_dl_entity() does not always invoke update_dl_entity() or replenish_dl_entity()... For example, when a task switches from SCHED_OTHER (or RT) to -deadline due to sched_setattr() (or similar) these functions are not invoked. Luca > If so, for already queued and boosted cases, rt_mutex_setprio() will > call enqueue_task() with ENQUEUE_REPLENISH set, so enqueue_dl_entity() > ->replenish_dl_entity() will advance p->dl.deadline beforehand, see > code: replenish_dl_entity(): > if (dl_se->dl_deadline == 0) { > dl_se->deadline = rq_clock(rq) + pi_se->dl_deadline; > dl_se->runtime = pi_se->dl_runtime; > } > > IOW, we don't need to handle !dl boosted cases in > setup_new_dl_entity(). > > Regards, > Xunlei > > > > > if (task_on_rq_queued(p) && rq->curr != p) { > > #ifdef CONFIG_SMP >
Re: [PATCH v3] sched/deadline: remove useless param from setup_new_dl_entity
On 2016/07/08 at 19:28, Juri Lelli 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(>dl, >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> Cc: Peter Zijlstra > Cc: Steven Rostedt > Cc: Luca Abeni > Signed-off-by: Juri Lelli > > --- > Changes from v2: >- optimize by calling rt_mutex_get_top_task only if the task is > boosted (as suggested by Steve) > > 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 | 16 +--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c > index fcb7f0217ff4..7aeb21963d3c 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; > + struct sched_dl_entity *pi_se = dl_se; > > WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline)); > > @@ -363,6 +364,15 @@ 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 (dl_se->dl_boosted && > + (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se))) && > + dl_prio(pi_task->normal_prio)) > + pi_se = _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 +1731,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(>dl, >dl); > + setup_new_dl_entity(>dl); I'm curious why we even call setup_new_dl_entity() for non-queued cases? It seems more reasonable to do it when it really gets queued. We can see that enqueue_task_dl()->update_dl_entity() also has the same update logic as switched_to_dl(). If so, for already queued and boosted cases, rt_mutex_setprio() will call enqueue_task() with ENQUEUE_REPLENISH set, so enqueue_dl_entity() ->replenish_dl_entity() will advance p->dl.deadline beforehand, see code: replenish_dl_entity(): if (dl_se->dl_deadline == 0) { dl_se->deadline = rq_clock(rq) + pi_se->dl_deadline; dl_se->runtime = pi_se->dl_runtime; } IOW, we don't need to handle !dl boosted cases in setup_new_dl_entity(). Regards, Xunlei > > if (task_on_rq_queued(p) && rq->curr != p) { > #ifdef CONFIG_SMP
Re: [PATCH v3] sched/deadline: remove useless param from setup_new_dl_entity
On 2016/07/08 at 19:28, Juri Lelli 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(>dl, >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 > Cc: Peter Zijlstra > Cc: Steven Rostedt > Cc: Luca Abeni > Signed-off-by: Juri Lelli > > --- > Changes from v2: >- optimize by calling rt_mutex_get_top_task only if the task is > boosted (as suggested by Steve) > > 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 | 16 +--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c > index fcb7f0217ff4..7aeb21963d3c 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; > + struct sched_dl_entity *pi_se = dl_se; > > WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline)); > > @@ -363,6 +364,15 @@ 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 (dl_se->dl_boosted && > + (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se))) && > + dl_prio(pi_task->normal_prio)) > + pi_se = _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 +1731,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(>dl, >dl); > + setup_new_dl_entity(>dl); I'm curious why we even call setup_new_dl_entity() for non-queued cases? It seems more reasonable to do it when it really gets queued. We can see that enqueue_task_dl()->update_dl_entity() also has the same update logic as switched_to_dl(). If so, for already queued and boosted cases, rt_mutex_setprio() will call enqueue_task() with ENQUEUE_REPLENISH set, so enqueue_dl_entity() ->replenish_dl_entity() will advance p->dl.deadline beforehand, see code: replenish_dl_entity(): if (dl_se->dl_deadline == 0) { dl_se->deadline = rq_clock(rq) + pi_se->dl_deadline; dl_se->runtime = pi_se->dl_runtime; } IOW, we don't need to handle !dl boosted cases in setup_new_dl_entity(). Regards, Xunlei > > if (task_on_rq_queued(p) && rq->curr != p) { > #ifdef CONFIG_SMP
[PATCH v3] sched/deadline: remove useless param from setup_new_dl_entity
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(>dl, >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 MolnarCc: Peter Zijlstra Cc: Steven Rostedt Cc: Luca Abeni Signed-off-by: Juri Lelli --- Changes from v2: - optimize by calling rt_mutex_get_top_task only if the task is boosted (as suggested by Steve) 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 | 16 +--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c index fcb7f0217ff4..7aeb21963d3c 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; + struct sched_dl_entity *pi_se = dl_se; WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline)); @@ -363,6 +364,15 @@ 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 (dl_se->dl_boosted && + (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se))) && + dl_prio(pi_task->normal_prio)) + pi_se = _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 +1731,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(>dl, >dl); + setup_new_dl_entity(>dl); if (task_on_rq_queued(p) && rq->curr != p) { #ifdef CONFIG_SMP -- 2.7.4
[PATCH v3] sched/deadline: remove useless param from setup_new_dl_entity
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(>dl, >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 Cc: Peter Zijlstra Cc: Steven Rostedt Cc: Luca Abeni Signed-off-by: Juri Lelli --- Changes from v2: - optimize by calling rt_mutex_get_top_task only if the task is boosted (as suggested by Steve) 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 | 16 +--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c index fcb7f0217ff4..7aeb21963d3c 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; + struct sched_dl_entity *pi_se = dl_se; WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline)); @@ -363,6 +364,15 @@ 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 (dl_se->dl_boosted && + (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se))) && + dl_prio(pi_task->normal_prio)) + pi_se = _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 +1731,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(>dl, >dl); + setup_new_dl_entity(>dl); if (task_on_rq_queued(p) && rq->curr != p) { #ifdef CONFIG_SMP -- 2.7.4