Re: [PATCH v3 1/4] sched/rt: Check to push the task away after its affinity was changed
On Sat, 30 May 2015 12:54:23 +0200 Peter Zijlstra wrote: > On Sat, 2015-05-30 at 10:20 +0200, Peter Zijlstra wrote: > > Makes me like the thing even less though.. > > Steven, why do we normally push on schedule()? Would not the natural > location be where we add to pushable_tasks? > > Which would be here in set_cpus_allowed() and wakeups. schedule() seems > like a second best location. What about when an RT task gets preempted by a higher prio task. We may need to push the preempted one. You can't do that at wakeup. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/4] sched/rt: Check to push the task away after its affinity was changed
On Sat, 2015-05-30 at 10:20 +0200, Peter Zijlstra wrote: > Makes me like the thing even less though.. Steven, why do we normally push on schedule()? Would not the natural location be where we add to pushable_tasks? Which would be here in set_cpus_allowed() and wakeups. schedule() seems like a second best location. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/4] sched/rt: Check to push the task away after its affinity was changed
On Fri, May 29, 2015 at 10:04:36PM +0800, pang.xun...@zte.com.cn wrote:
> Hi Peter,
>
> Peter Zijlstra wrote 2015-05-29 PM 09:16:26:
> >
> > Re: [PATCH v3 1/4] sched/rt: Check to push the task away after its
> > affinity was changed
> >
> > On Tue, May 12, 2015 at 10:46:41PM +0800, Xunlei Pang wrote:
> > > @@ -2278,6 +2279,20 @@ static void set_cpus_allowed_rt(struct
> > task_struct *p,
> > > }
> > >
> > > update_rt_migration(>rt);
> > > +
> > > +check_push:
> > > + if (weight > 1 &&
> > > + !task_running(rq, p) &&
> > > + !test_tsk_need_resched(rq->curr) &&
> > > + !cpumask_subset(new_mask, >cpus_allowed)) {
> > > + /* Update new affinity and try to push. */
> > > + cpumask_copy(>cpus_allowed, new_mask);
> > > + p->nr_cpus_allowed = weight;
> > > + push_rt_tasks(rq);
> > > + return true;
> > > + }
> > > +
> > > + return false;
> > > }
> >
> > I think this is broken; push_rt_tasks() will do double_rq_lock() which
> > will drop rq->lock.
> >
> > This means load-balancing can come in and move our task p; in fact,
> > push_rt_task() can do exactly that -- after all that was the point of
> > this patch.
> >
> > _However_ this means that after calling ->set_cpus_allowed() we must not
> > assume @p is on @rt, yet we do. Look at __set_cpus_allowed_ptr(), we'll
> > call move_queued_task() if (!running || waking) && on_rq, and
> > move_queued_task() happily calls dequeue_task(rq, p), which will go
> > *boom*.
>
> I can't see why this can happen?
>
> After finishing set_cpus_allowed_rt(), if there happens a successful
> load-balancing (pull or push) action, new task_cpu(@p) will be set,
> so we will definitely get the following true condition:
>
> /* Can the task run on the task's current CPU? If so, we're done
> */
> if (cpumask_test_cpu(task_cpu(p), new_mask))
> goto out;
>
> So I think the whole function will simply go out and return normally.
Humm, yes. Missed that. That makes it work by accident; because you
didn't document/Changelog any of this.
Makes me like the thing even less though..
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/4] sched/rt: Check to push the task away after its affinity was changed
On Sat, 30 May 2015 12:54:23 +0200 Peter Zijlstra pet...@infradead.org wrote: On Sat, 2015-05-30 at 10:20 +0200, Peter Zijlstra wrote: Makes me like the thing even less though.. Steven, why do we normally push on schedule()? Would not the natural location be where we add to pushable_tasks? Which would be here in set_cpus_allowed() and wakeups. schedule() seems like a second best location. What about when an RT task gets preempted by a higher prio task. We may need to push the preempted one. You can't do that at wakeup. -- Steve -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/4] sched/rt: Check to push the task away after its affinity was changed
On Fri, May 29, 2015 at 10:04:36PM +0800, pang.xun...@zte.com.cn wrote:
Hi Peter,
Peter Zijlstra pet...@infradead.org wrote 2015-05-29 PM 09:16:26:
Re: [PATCH v3 1/4] sched/rt: Check to push the task away after its
affinity was changed
On Tue, May 12, 2015 at 10:46:41PM +0800, Xunlei Pang wrote:
@@ -2278,6 +2279,20 @@ static void set_cpus_allowed_rt(struct
task_struct *p,
}
update_rt_migration(rq-rt);
+
+check_push:
+ if (weight 1
+ !task_running(rq, p)
+ !test_tsk_need_resched(rq-curr)
+ !cpumask_subset(new_mask, p-cpus_allowed)) {
+ /* Update new affinity and try to push. */
+ cpumask_copy(p-cpus_allowed, new_mask);
+ p-nr_cpus_allowed = weight;
+ push_rt_tasks(rq);
+ return true;
+ }
+
+ return false;
}
I think this is broken; push_rt_tasks() will do double_rq_lock() which
will drop rq-lock.
This means load-balancing can come in and move our task p; in fact,
push_rt_task() can do exactly that -- after all that was the point of
this patch.
_However_ this means that after calling -set_cpus_allowed() we must not
assume @p is on @rt, yet we do. Look at __set_cpus_allowed_ptr(), we'll
call move_queued_task() if (!running || waking) on_rq, and
move_queued_task() happily calls dequeue_task(rq, p), which will go
*boom*.
I can't see why this can happen?
After finishing set_cpus_allowed_rt(), if there happens a successful
load-balancing (pull or push) action, new task_cpu(@p) will be set,
so we will definitely get the following true condition:
/* Can the task run on the task's current CPU? If so, we're done
*/
if (cpumask_test_cpu(task_cpu(p), new_mask))
goto out;
So I think the whole function will simply go out and return normally.
Humm, yes. Missed that. That makes it work by accident; because you
didn't document/Changelog any of this.
Makes me like the thing even less though..
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/4] sched/rt: Check to push the task away after its affinity was changed
On Sat, 2015-05-30 at 10:20 +0200, Peter Zijlstra wrote: Makes me like the thing even less though.. Steven, why do we normally push on schedule()? Would not the natural location be where we add to pushable_tasks? Which would be here in set_cpus_allowed() and wakeups. schedule() seems like a second best location. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/4] sched/rt: Check to push the task away after its affinity was changed
On Tue, May 12, 2015 at 10:46:41PM +0800, Xunlei Pang wrote:
> @@ -2278,6 +2279,20 @@ static void set_cpus_allowed_rt(struct task_struct *p,
> }
>
> update_rt_migration(>rt);
> +
> +check_push:
> + if (weight > 1 &&
> + !task_running(rq, p) &&
> + !test_tsk_need_resched(rq->curr) &&
> + !cpumask_subset(new_mask, >cpus_allowed)) {
> + /* Update new affinity and try to push. */
> + cpumask_copy(>cpus_allowed, new_mask);
> + p->nr_cpus_allowed = weight;
> + push_rt_tasks(rq);
> + return true;
> + }
> +
> + return false;
> }
I think this is broken; push_rt_tasks() will do double_rq_lock() which
will drop rq->lock.
This means load-balancing can come in and move our task p; in fact,
push_rt_task() can do exactly that -- after all that was the point of
this patch.
_However_ this means that after calling ->set_cpus_allowed() we must not
assume @p is on @rt, yet we do. Look at __set_cpus_allowed_ptr(), we'll
call move_queued_task() if (!running || waking) && on_rq, and
move_queued_task() happily calls dequeue_task(rq, p), which will go
*boom*.
I currently do not have a better idea than to repurpose the PUSH_IPI
stuff, that is, send a self IPI to go do the push or somesuch. Lemme
stare at this a little more.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/4] sched/rt: Check to push the task away after its affinity was changed
On Tue, May 12, 2015 at 10:46:41PM +0800, Xunlei Pang wrote:
@@ -2278,6 +2279,20 @@ static void set_cpus_allowed_rt(struct task_struct *p,
}
update_rt_migration(rq-rt);
+
+check_push:
+ if (weight 1
+ !task_running(rq, p)
+ !test_tsk_need_resched(rq-curr)
+ !cpumask_subset(new_mask, p-cpus_allowed)) {
+ /* Update new affinity and try to push. */
+ cpumask_copy(p-cpus_allowed, new_mask);
+ p-nr_cpus_allowed = weight;
+ push_rt_tasks(rq);
+ return true;
+ }
+
+ return false;
}
I think this is broken; push_rt_tasks() will do double_rq_lock() which
will drop rq-lock.
This means load-balancing can come in and move our task p; in fact,
push_rt_task() can do exactly that -- after all that was the point of
this patch.
_However_ this means that after calling -set_cpus_allowed() we must not
assume @p is on @rt, yet we do. Look at __set_cpus_allowed_ptr(), we'll
call move_queued_task() if (!running || waking) on_rq, and
move_queued_task() happily calls dequeue_task(rq, p), which will go
*boom*.
I currently do not have a better idea than to repurpose the PUSH_IPI
stuff, that is, send a self IPI to go do the push or somesuch. Lemme
stare at this a little more.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3 1/4] sched/rt: Check to push the task away after its affinity was changed
From: Xunlei Pang
We may suffer from extra rt overload rq due to the affinity,
so when the affinity of any runnable rt task is changed, we
should check to trigger balancing, otherwise it will cause
some unnecessary delayed real-time response. Unfortunately,
current RT global scheduler does nothing about this.
For example: a 2-cpu system with two runnable FIFO tasks(same
rt_priority) bound on CPU0, let's name them rt1(running) and
rt2(runnable) respectively; CPU1 has no RTs. Then, someone sets
the affinity of rt2 to 0x3(i.e. CPU0 and CPU1), but after this,
rt2 still can't be scheduled enters schedule(), this
definitely causes some/big response latency for rt2.
This patch modified set_cpus_allowed_rt(), if the target task
is runnable but not running, it tries to push it away once it
got migratable.
The patch also solves a problem about move_queued_task() called
in set_cpus_allowed_ptr():
When a lower priorioty rt task got migrated due to its curr cpu
isn't in the new affinity mask, after move_queued_task() it will
miss the chance of pushing away, because check_preempt_curr()
called by move_queued_task() doens't set the "need resched flag"
for lower priority tasks.
Signed-off-by: Xunlei Pang
---
kernel/sched/core.c | 10 +++---
kernel/sched/deadline.c | 8 +---
kernel/sched/rt.c | 29 ++---
kernel/sched/sched.h| 3 ++-
4 files changed, 36 insertions(+), 14 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index d13fc13..c995a02 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4768,11 +4768,15 @@ static struct rq *move_queued_task(struct task_struct
*p, int new_cpu)
void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new_mask)
{
+ bool updated = false;
+
if (p->sched_class->set_cpus_allowed)
- p->sched_class->set_cpus_allowed(p, new_mask);
+ updated = p->sched_class->set_cpus_allowed(p, new_mask);
- cpumask_copy(>cpus_allowed, new_mask);
- p->nr_cpus_allowed = cpumask_weight(new_mask);
+ if (!updated) {
+ cpumask_copy(>cpus_allowed, new_mask);
+ p->nr_cpus_allowed = cpumask_weight(new_mask);
+ }
}
/*
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 5e95145..3baffb2 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -1574,7 +1574,7 @@ static void task_woken_dl(struct rq *rq, struct
task_struct *p)
}
}
-static void set_cpus_allowed_dl(struct task_struct *p,
+static bool set_cpus_allowed_dl(struct task_struct *p,
const struct cpumask *new_mask)
{
struct rq *rq;
@@ -1610,7 +1610,7 @@ static void set_cpus_allowed_dl(struct task_struct *p,
* it is on the rq AND it is not throttled).
*/
if (!on_dl_rq(>dl))
- return;
+ return false;
weight = cpumask_weight(new_mask);
@@ -1619,7 +1619,7 @@ static void set_cpus_allowed_dl(struct task_struct *p,
* can migrate or not.
*/
if ((p->nr_cpus_allowed > 1) == (weight > 1))
- return;
+ return false;
/*
* The process used to be able to migrate OR it can now migrate
@@ -1636,6 +1636,8 @@ static void set_cpus_allowed_dl(struct task_struct *p,
}
update_dl_migration(>dl);
+
+ return false;
}
/* Assumes rq->lock is held */
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 8885b65..4a49c6a 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -2241,7 +2241,7 @@ static void task_woken_rt(struct rq *rq, struct
task_struct *p)
push_rt_tasks(rq);
}
-static void set_cpus_allowed_rt(struct task_struct *p,
+static bool set_cpus_allowed_rt(struct task_struct *p,
const struct cpumask *new_mask)
{
struct rq *rq;
@@ -2250,18 +2250,19 @@ static void set_cpus_allowed_rt(struct task_struct *p,
BUG_ON(!rt_task(p));
if (!task_on_rq_queued(p))
- return;
+ return false;
weight = cpumask_weight(new_mask);
+ rq = task_rq(p);
+
/*
-* Only update if the process changes its state from whether it
-* can migrate or not.
+* Skip updating the migration stuff if the process doesn't change
+* its migrate state, but still need to check if it can be pushed
+* away due to its new affinity.
*/
if ((p->nr_cpus_allowed > 1) == (weight > 1))
- return;
-
- rq = task_rq(p);
+ goto check_push;
/*
* The process used to be able to migrate OR it can now migrate
@@ -2278,6 +2279,20 @@ static void set_cpus_allowed_rt(struct task_struct *p,
}
update_rt_migration(>rt);
+
+check_push:
+ if (weight > 1 &&
+ !task_running(rq, p) &&
+ !test_tsk_need_resched(rq->curr) &&
+
[PATCH v3 1/4] sched/rt: Check to push the task away after its affinity was changed
From: Xunlei Pang pang.xun...@linaro.org
We may suffer from extra rt overload rq due to the affinity,
so when the affinity of any runnable rt task is changed, we
should check to trigger balancing, otherwise it will cause
some unnecessary delayed real-time response. Unfortunately,
current RT global scheduler does nothing about this.
For example: a 2-cpu system with two runnable FIFO tasks(same
rt_priority) bound on CPU0, let's name them rt1(running) and
rt2(runnable) respectively; CPU1 has no RTs. Then, someone sets
the affinity of rt2 to 0x3(i.e. CPU0 and CPU1), but after this,
rt2 still can't be scheduled enters schedule(), this
definitely causes some/big response latency for rt2.
This patch modified set_cpus_allowed_rt(), if the target task
is runnable but not running, it tries to push it away once it
got migratable.
The patch also solves a problem about move_queued_task() called
in set_cpus_allowed_ptr():
When a lower priorioty rt task got migrated due to its curr cpu
isn't in the new affinity mask, after move_queued_task() it will
miss the chance of pushing away, because check_preempt_curr()
called by move_queued_task() doens't set the need resched flag
for lower priority tasks.
Signed-off-by: Xunlei Pang pang.xun...@linaro.org
---
kernel/sched/core.c | 10 +++---
kernel/sched/deadline.c | 8 +---
kernel/sched/rt.c | 29 ++---
kernel/sched/sched.h| 3 ++-
4 files changed, 36 insertions(+), 14 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index d13fc13..c995a02 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4768,11 +4768,15 @@ static struct rq *move_queued_task(struct task_struct
*p, int new_cpu)
void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new_mask)
{
+ bool updated = false;
+
if (p-sched_class-set_cpus_allowed)
- p-sched_class-set_cpus_allowed(p, new_mask);
+ updated = p-sched_class-set_cpus_allowed(p, new_mask);
- cpumask_copy(p-cpus_allowed, new_mask);
- p-nr_cpus_allowed = cpumask_weight(new_mask);
+ if (!updated) {
+ cpumask_copy(p-cpus_allowed, new_mask);
+ p-nr_cpus_allowed = cpumask_weight(new_mask);
+ }
}
/*
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 5e95145..3baffb2 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -1574,7 +1574,7 @@ static void task_woken_dl(struct rq *rq, struct
task_struct *p)
}
}
-static void set_cpus_allowed_dl(struct task_struct *p,
+static bool set_cpus_allowed_dl(struct task_struct *p,
const struct cpumask *new_mask)
{
struct rq *rq;
@@ -1610,7 +1610,7 @@ static void set_cpus_allowed_dl(struct task_struct *p,
* it is on the rq AND it is not throttled).
*/
if (!on_dl_rq(p-dl))
- return;
+ return false;
weight = cpumask_weight(new_mask);
@@ -1619,7 +1619,7 @@ static void set_cpus_allowed_dl(struct task_struct *p,
* can migrate or not.
*/
if ((p-nr_cpus_allowed 1) == (weight 1))
- return;
+ return false;
/*
* The process used to be able to migrate OR it can now migrate
@@ -1636,6 +1636,8 @@ static void set_cpus_allowed_dl(struct task_struct *p,
}
update_dl_migration(rq-dl);
+
+ return false;
}
/* Assumes rq-lock is held */
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 8885b65..4a49c6a 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -2241,7 +2241,7 @@ static void task_woken_rt(struct rq *rq, struct
task_struct *p)
push_rt_tasks(rq);
}
-static void set_cpus_allowed_rt(struct task_struct *p,
+static bool set_cpus_allowed_rt(struct task_struct *p,
const struct cpumask *new_mask)
{
struct rq *rq;
@@ -2250,18 +2250,19 @@ static void set_cpus_allowed_rt(struct task_struct *p,
BUG_ON(!rt_task(p));
if (!task_on_rq_queued(p))
- return;
+ return false;
weight = cpumask_weight(new_mask);
+ rq = task_rq(p);
+
/*
-* Only update if the process changes its state from whether it
-* can migrate or not.
+* Skip updating the migration stuff if the process doesn't change
+* its migrate state, but still need to check if it can be pushed
+* away due to its new affinity.
*/
if ((p-nr_cpus_allowed 1) == (weight 1))
- return;
-
- rq = task_rq(p);
+ goto check_push;
/*
* The process used to be able to migrate OR it can now migrate
@@ -2278,6 +2279,20 @@ static void set_cpus_allowed_rt(struct task_struct *p,
}
update_rt_migration(rq-rt);
+
+check_push:
+ if (weight 1
+ !task_running(rq, p)
+
