On Mon, Oct 15, 2018 at 11:42:20AM -0400, Steven Rostedt wrote:
> On Mon, 15 Oct 2018 11:20:32 +0200
> Peter Zijlstra <[email protected]> wrote:
>
> > > index 2e2955a..be0fc43 100644
> > > --- a/kernel/sched/rt.c
> > > +++ b/kernel/sched/rt.c
> > > @@ -1754,7 +1754,7 @@ static struct rq *find_lock_lowest_rq(struct
> > > task_struct *task, struct rq *rq)
> > > !task_on_rq_queued(task))) {
> > >
> > > double_unlock_balance(rq, lowest_rq);
> > > - lowest_rq = NULL;
> > > + lowest_rq = RETRY_TASK;
> > > break;
> > > }
> > > }
> >
> > I'm confused.. should not:
> >
> > /* try again */
> > double_unlock_balance(rq, lowest_rq);
> > lowest_rq = NULL;
> >
> > also return RETRY_TASK? That also is in the double_lock_balance() path
> > and will this have had rq->lock() released.
>
> I thought the same thing at first, but this is in the loop path, where
> it does everything again. But now looking closer, I think there's a bug
> in the original code.
So I find that whole thing utterly confusing; what about we start with
something like so?
---
kernel/sched/rt.c | 51 +++++++++++++++++++++++----------------------------
1 file changed, 23 insertions(+), 28 deletions(-)
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 2e2955a8cf8f..237c84c2b042 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -1714,6 +1714,26 @@ static int find_lowest_rq(struct task_struct *task)
return -1;
}
+static struct task_struct *first_pushable_task(struct rq *rq)
+{
+ struct task_struct *p;
+
+ if (!has_pushable_tasks(rq))
+ return NULL;
+
+ p = plist_first_entry(&rq->rt.pushable_tasks,
+ struct task_struct, pushable_tasks);
+
+ BUG_ON(rq->cpu != task_cpu(p));
+ BUG_ON(task_current(rq, p));
+ BUG_ON(p->nr_cpus_allowed <= 1);
+
+ BUG_ON(!task_on_rq_queued(p));
+ BUG_ON(!rt_task(p));
+
+ return p;
+}
+
/* Will lock the rq it finds */
static struct rq *find_lock_lowest_rq(struct task_struct *task, struct rq *rq)
{
@@ -1747,12 +1767,7 @@ static struct rq *find_lock_lowest_rq(struct task_struct
*task, struct rq *rq)
* migrated already or had its affinity changed.
* Also make sure that it wasn't scheduled on its rq.
*/
- if (unlikely(task_rq(task) != rq ||
- !cpumask_test_cpu(lowest_rq->cpu,
&task->cpus_allowed) ||
- task_running(rq, task) ||
- !rt_task(task) ||
- !task_on_rq_queued(task))) {
-
+ if (first_pushable_task(rq) != task)
double_unlock_balance(rq, lowest_rq);
lowest_rq = NULL;
break;
@@ -1771,26 +1786,6 @@ static struct rq *find_lock_lowest_rq(struct task_struct
*task, struct rq *rq)
return lowest_rq;
}
-static struct task_struct *pick_next_pushable_task(struct rq *rq)
-{
- struct task_struct *p;
-
- if (!has_pushable_tasks(rq))
- return NULL;
-
- p = plist_first_entry(&rq->rt.pushable_tasks,
- struct task_struct, pushable_tasks);
-
- BUG_ON(rq->cpu != task_cpu(p));
- BUG_ON(task_current(rq, p));
- BUG_ON(p->nr_cpus_allowed <= 1);
-
- BUG_ON(!task_on_rq_queued(p));
- BUG_ON(!rt_task(p));
-
- return p;
-}
-
/*
* If the current CPU has more than one RT task, see if the non
* running task can migrate over to a CPU that is running a task
@@ -1805,7 +1800,7 @@ static int push_rt_task(struct rq *rq)
if (!rq->rt.overloaded)
return 0;
- next_task = pick_next_pushable_task(rq);
+ next_task = first_pushable_task(rq);
if (!next_task)
return 0;
@@ -1840,7 +1835,7 @@ static int push_rt_task(struct rq *rq)
* run-queue and is also still the next task eligible for
* pushing.
*/
- task = pick_next_pushable_task(rq);
+ task = first_pushable_task(rq);
if (task == next_task) {
/*
* The task hasn't migrated, and is still the next
