sched/rt: Do not pull from current CPU if only one cpu to pull
Steven Rostedt
reviewerPATCH 1/1 sched/rt: Do not pull from current CPU if only one cpu to pull
Daniel Wagner reported a crash on the beaglebone black. This is a single CPU architecture, and does not have a functional:arch_send_call_function_single_ipi() and can crash if that is called.
As it only has one CPU, it shouldn't be called, but if the kernel is compiled for SMP, the push/pull RT scheduling logic now calls it for irq_work if the one CPU is overloaded, it can use that function to call itself and crash the kernel.
Ideally, we should disable the SCHED_FEAT(RT_PUSH_IPI) if the system only has a single CPU. But SCHED_FEAT is a constant if sched debugging is turned off. Another fix can also be used, and this should also help with normal SMP machines. That is, do not initiate the pull code if there's only one RT overloaded CPU, and that CPU happens to be the current CPU that is scheduling in a lower priority task.
Even on a system with many CPUs, if there's many RT tasks waiting to run on a single CPU, and that CPU schedules in another RT task of lower priority, it will initiate the PULL logic in case there's a higher priority RT task on another CPU that is waiting to run. But if there is no other CPU with waiting RT tasks, it will initiate the RT pull logic on itself (as it still has RT tasks waiting to run). This is a wasted effort.
Not only does this help with SMP code where the current CPU is the only one with RT overloaded tasks, it should also solve the issue that Daniel encountered, because it will prevent the PULL logic from executing, as there's only one CPU on the system, and the check added here will cause it to exit the RT pull code.
---
kernel/sched/rt.c +7 -1
@@ -2034,8 +2034,9 @@ static void pull_rt_task(struct rq *this_rq)bool resched = false;struct task_struct *p;struct rq *src_rq; +int rt_overload_count =rt_overloaded (this_rq); -if (likely (!rt_overloaded (this_rq))) +if (likely (!rt_overload_count))return ;/*
@@ -2044,6 +2045,11 @@ static void pull_rt_task(struct rq *this_rq)*/ smp_rmb (); +/* If we are the only overloaded CPU do nothing */ +if (rt_overload_count ==1 && +cpumask_test_cpu (this_rq->cpu, this_rq->rd->rto_mask)) +return ; +#ifdef HAVE_RT_PUSH_IPI if (sched_feat (RT_PUSH_IPI )) {tell_cpu_to_push (this_rq);
Juri Lelli
maintainerHi Steve,
Quoted 51 lines · Steven Rostedt
Daniel Wagner reported a crash on the beaglebone black. This is a single CPU architecture, and does not have a functional:
arch_send_call_function_single_ipi()and can crash if that is called.As it only has one CPU, it shouldn't be called, but if the kernel is compiled for SMP, the push/pull RT scheduling logic now calls it for
irq_workif the one CPU is overloaded, it can use that function to call itself and crash the kernel.Ideally, we should disable the
SCHED_FEAT(RT_PUSH_IPI)if the system only has a single CPU. ButSCHED_FEATis a constant if sched debugging is turned off. Another fix can also be used, and this should also help with normal SMP machines. That is, do not initiate the pull code if there's only one RT overloaded CPU, and that CPU happens to be the current CPU that is scheduling in a lower priority task.Even on a system with many CPUs, if there's many RT tasks waiting to run on a single CPU, and that CPU schedules in another RT task of lower priority, it will initiate the PULL logic in case there's a higher priority RT task on another CPU that is waiting to run. But if there is no other CPU with waiting RT tasks, it will initiate the RT pull logic on itself (as it still has RT tasks waiting to run). This is a wasted effort.
Not only does this help with SMP code where the current CPU is the only one with RT overloaded tasks, it should also solve the issue that Daniel encountered, because it will prevent the PULL logic from executing, as there's only one CPU on the system, and the check added here will cause it to exit the RT pull code.
Fixes 4bdced5c9 ("sched/rt: Simplify the IPI based RT balancing logic")Reported-by Daniel Wagner <[email protected]>---
kernel/sched/rt.c +2
-1@@ -2034,8 +2034,9 @@ static void pull_rt_task(struct rq *this_rq)bool resched = false;struct task_struct *p;struct rq *src_rq; +int rt_overload_count =rt_overloaded (this_rq); -if (likely (!rt_overloaded (this_rq))) +if (likely (!rt_overload_count))return ;/*
kernel/sched/rt.c static void pull_rt_task(struct rq *this_rq) · line 2052
Quoted 8 lines
kernel/sched/rt.c +5
-0@@ -2044,6 +2045,11 @@ static void pull_rt_task(struct rq *this_rq)*/ smp_rmb (); +/* If we are the only overloaded CPU do nothing */ +if (rt_overload_count ==1 && +cpumask_test_cpu (this_rq->cpu, this_rq->rd->rto_mask)) +return ; +
Right. I was wondering however if for the truly UP case we shouldn't be initiating/queueing callbacks (pull/push) at all?
DEADLINE doesn't use (yet?) the PUSH_IPI, but we will need a similar patch to keep logics aligned.
Best,
Juri
Steven Rostedt
reviewerQuoted 2 lines · On Mon, 4 Dec 2017 08:45:17 +0100 Juri Lelli <[email protected]> wrote:
Right. I was wondering however if for the truly UP case we shouldn't be initiating/queueing callbacks (pull/push) at all?
If !CONFIG_SMP then it's not compiled in. The issue came up when Daniel ran a CONFIG_SMP kernel on an arch that only supports UP.
Quoted 3 lines
DEADLINE doesn't use (yet?) the
PUSH_IPI, but we will need a similar patch to keep logics aligned.
Maybe.
-- Steve
Juri Lelli
maintainerQuoted 9 lines · Steven Rostedt
Quoted 2 lines · On Mon, 4 Dec 2017 08:45:17 +0100 Juri Lelli <[email protected]> wrote:
Right. I was wondering however if for the truly UP case we shouldn't be initiating/queueing callbacks (pull/push) at all?
If !CONFIG_SMP then it's not compiled in. The issue came up when Daniel ran a
CONFIG_SMPkernel on an arch that only supports UP.
Right, sorry. I meant num_online_cpus() == 1.
Best,
Juri
Steven Rostedt
reviewerQuoted 4 lines · On Mon, 4 Dec 2017 10:07:57 +0100 Juri Lelli <[email protected]> wrote:
Quoted 3 lines · Steven Rostedt
On Mon, 4 Dec 2017 08:45:17 +0100
Juri Lelli <[email protected]> wrote:
Quoted 9 lines
Quoted 6 lines
Quoted 2 lines
Right. I was wondering however if for the truly UP case we shouldn't be initiating/queueing callbacks (pull/push) at all?
If !CONFIG_SMP then it's not compiled in. The issue came up when Daniel ran a
CONFIG_SMPkernel on an arch that only supports UP.Right, sorry. I meant
num_online_cpus()== 1.
Correct. But we need to disable the push/pull when CPUs go down to 1, or if we see "num_possible_cpus() == 1" at boot up. It woulld need to be re-enabled when CPUs are onlined and count goes greater than one. Which we could also add, and I started going that route first. My first patch had that check at each push/pull, but num_online_cpus() is a weight of the cpumask, and for machines with more than 64 CPUs, calculating that number becomes a bigger task and we want to keep that out of the scheduler fast path, which push/pull logic happens to be in.
When looking at changing this code, I realized that rt_overloaded() returns the count of overloaded CPUs, and the check to see if the current CPU is overloaded is a single bit check of a cpumask (all very quick). This not only fixes the issue with what Daniel found, but also can help in certain cases on large CPU count machines.
-- Steve
Hi Steven,
Quoted 30 lines · Steven Rostedt
Daniel Wagner reported a crash on the beaglebone black. This is a single CPU architecture, and does not have a functional:
arch_send_call_function_single_ipi()and can crash if that is called.As it only has one CPU, it shouldn't be called, but if the kernel is compiled for SMP, the push/pull RT scheduling logic now calls it for
irq_workif the one CPU is overloaded, it can use that function to call itself and crash the kernel.Ideally, we should disable the
SCHED_FEAT(RT_PUSH_IPI)if the system only has a single CPU. ButSCHED_FEATis a constant if sched debugging is turned off. Another fix can also be used, and this should also help with normal SMP machines. That is, do not initiate the pull code if there's only one RT overloaded CPU, and that CPU happens to be the current CPU that is scheduling in a lower priority task.Even on a system with many CPUs, if there's many RT tasks waiting to run on a single CPU, and that CPU schedules in another RT task of lower priority, it will initiate the PULL logic in case there's a higher priority RT task on another CPU that is waiting to run. But if there is no other CPU with waiting RT tasks, it will initiate the RT pull logic on itself (as it still has RT tasks waiting to run). This is a wasted effort.
Not only does this help with SMP code where the current CPU is the only one with RT overloaded tasks, it should also solve the issue that Daniel encountered, because it will prevent the PULL logic from executing, as there's only one CPU on the system, and the check added here will cause it to exit the RT pull code.
Quoted 4 lines
Fixes 4bdced5c9 ("sched/rt: Simplify the IPI based RT balancing logic")Reported-by Daniel Wagner <[email protected]>
Thanks,
Daniel
Quoted 30 lines · Steven Rostedt
Daniel Wagner reported a crash on the beaglebone black. This is a single CPU architecture, and does not have a functional:
arch_send_call_function_single_ipi()and can crash if that is called.As it only has one CPU, it shouldn't be called, but if the kernel is compiled for SMP, the push/pull RT scheduling logic now calls it for
irq_workif the one CPU is overloaded, it can use that function to call itself and crash the kernel.Ideally, we should disable the
SCHED_FEAT(RT_PUSH_IPI)if the system only has a single CPU. ButSCHED_FEATis a constant if sched debugging is turned off. Another fix can also be used, and this should also help with normal SMP machines. That is, do not initiate the pull code if there's only one RT overloaded CPU, and that CPU happens to be the current CPU that is scheduling in a lower priority task.Even on a system with many CPUs, if there's many RT tasks waiting to run on a single CPU, and that CPU schedules in another RT task of lower priority, it will initiate the PULL logic in case there's a higher priority RT task on another CPU that is waiting to run. But if there is no other CPU with waiting RT tasks, it will initiate the RT pull logic on itself (as it still has RT tasks waiting to run). This is a wasted effort.
Not only does this help with SMP code where the current CPU is the only one with RT overloaded tasks, it should also solve the issue that Daniel encountered, because it will prevent the PULL logic from executing, as there's only one CPU on the system, and the check added here will cause it to exit the RT pull code.
Quoted 4 lines
Fixes 4bdced5c9 ("sched/rt: Simplify the IPI based RT balancing logic")Reported-by Daniel Wagner <[email protected]>
I've added:
Which I suspect you just forgot to add?
Thanks,
Ingo
Steven Rostedt
reviewerQuoted 10 lines · On Mon, 11 Dec 2017 17:47:32 +0100 Ingo Molnar <[email protected]> wrote:
Quoted 4 lines
Fixes 4bdced5c9 ("sched/rt: Simplify the IPI based RT balancing logic")Reported-by Daniel Wagner <[email protected]>I've added:
Signed-off-by Steven Rostedt (VMware) <[email protected]>Which I suspect you just forgot to add?
Ug, not sure how that happened, but yes, I simply forgot to add that (probably did it using quilt and expected to use git commit -s)
Thanks Ingo!
-- Steve
tip-bot for Steven Rostedt
merge botsched/rt: Do not pull from current CPU if only one cpu to pull
Commit-ID: f80a6308b1100d431e27afb61aa90d752df20aae Gitweb: https://git.kernel.org/tip/f80a6308b1100d431e27afb61aa90d752df20aae Author: Steven Rostedt <[email protected]> AuthorDate: Sat, 2 Dec 2017 13:04:54 -0500 Committer: Ingo Molnar <[email protected]> CommitDate: Mon, 11 Dec 2017 17:46:58 +0100
sched/rt: Do not pull from current CPU if only one cpu to pull
Daniel Wagner reported a crash on the beaglebone black. This is a single CPU architecture, and does not have a functional:arch_send_call_function_single_ipi() and can crash if that is called.
As it only has one CPU, it shouldn't be called, but if the kernel is compiled for SMP, the push/pull RT scheduling logic now calls it for irq_work if the one CPU is overloaded, it can use that function to call itself and crash the kernel.
Ideally, we should disable the SCHED_FEAT(RT_PUSH_IPI) if the system only has a single CPU. But SCHED_FEAT is a constant if sched debugging is turned off. Another fix can also be used, and this should also help with normal SMP machines. That is, do not initiate the pull code if there's only one RT overloaded CPU, and that CPU happens to be the current CPU that is scheduling in a lower priority task.
Even on a system with many CPUs, if there's many RT tasks waiting to run on a single CPU, and that CPU schedules in another RT task of lower priority, it will initiate the PULL logic in case there's a higher priority RT task on another CPU that is waiting to run. But if there is no other CPU with waiting RT tasks, it will initiate the RT pull logic on itself (as it still has RT tasks waiting to run). This is a wasted effort.
Not only does this help with SMP code where the current CPU is the only one with RT overloaded tasks, it should also solve the issue that Daniel encountered, because it will prevent the PULL logic from executing, as there's only one CPU on the system, and the check added here will cause it to exit the RT pull code.
--- kernel/sched/rt.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
kernel/sched/rt.c +7 -1
@@ -2034,8 +2034,9 @@ static void pull_rt_task(struct rq *this_rq)bool resched = false;struct task_struct *p;struct rq *src_rq; +int rt_overload_count =rt_overloaded (this_rq); -if (likely (!rt_overloaded (this_rq))) +if (likely (!rt_overload_count))return ;/*
@@ -2044,6 +2045,11 @@ static void pull_rt_task(struct rq *this_rq)*/ smp_rmb (); +/* If we are the only overloaded CPU do nothing */ +if (rt_overload_count ==1 && +cpumask_test_cpu (this_rq->cpu, this_rq->rd->rto_mask)) +return ; +#ifdef HAVE_RT_PUSH_IPI if (sched_feat (RT_PUSH_IPI )) {tell_cpu_to_push (this_rq);
tip-bot for Steven Rostedt
merge bot[tip:sched/urgent] sched/rt: Do not pull from current CPU if only one CPU to pull
In reply to Steven Rostedtsched/rt: Do not pull from current CPU if only one CPU to pull
Commit-ID: f73c52a5bcd1710994e53fbccc378c42b97a06b6 Gitweb: https://git.kernel.org/tip/f73c52a5bcd1710994e53fbccc378c42b97a06b6 Author: Steven Rostedt <[email protected]> AuthorDate: Sat, 2 Dec 2017 13:04:54 -0500 Committer: Ingo Molnar <[email protected]> CommitDate: Fri, 15 Dec 2017 16:28:02 +0100
sched/rt: Do not pull from current CPU if only one CPU to pull
Daniel Wagner reported a crash on the BeagleBone Black SoC.
This is a single CPU architecture, and does not have a functional arch_send_call_function_single_ipi() implementation which can crash the kernel if that is called.
As it only has one CPU, it shouldn't be called, but if the kernel is compiled for SMP, the push/pull RT scheduling logic now calls it for irq_work if the one CPU is overloaded, it can use that function to call itself and crash the kernel.
Ideally, we should disable the SCHED_FEAT(RT_PUSH_IPI) if the system only has a single CPU. But SCHED_FEAT is a constant if sched debugging is turned off. Another fix can also be used, and this should also help with normal SMP machines. That is, do not initiate the pull code if there's only one RT overloaded CPU, and that CPU happens to be the current CPU that is scheduling in a lower priority task.
Even on a system with many CPUs, if there's many RT tasks waiting to run on a single CPU, and that CPU schedules in another RT task of lower priority, it will initiate the PULL logic in case there's a higher priority RT task on another CPU that is waiting to run. But if there is no other CPU with waiting RT tasks, it will initiate the RT pull logic on itself (as it still has RT tasks waiting to run). This is a wasted effort.
Not only does this help with SMP code where the current CPU is the only one with RT overloaded tasks, it should also solve the issue that Daniel encountered, because it will prevent the PULL logic from executing, as there's only one CPU on the system, and the check added here will cause it to exit the RT pull code.
--- kernel/sched/rt.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
kernel/sched/rt.c +7 -1
@@ -2034,8 +2034,9 @@ static void pull_rt_task(struct rq *this_rq)bool resched = false;struct task_struct *p;struct rq *src_rq; +int rt_overload_count =rt_overloaded (this_rq); -if (likely (!rt_overloaded (this_rq))) +if (likely (!rt_overload_count))return ;/*
@@ -2044,6 +2045,11 @@ static void pull_rt_task(struct rq *this_rq)*/ smp_rmb (); +/* If we are the only overloaded CPU do nothing */ +if (rt_overload_count ==1 && +cpumask_test_cpu (this_rq->cpu, this_rq->rd->rto_mask)) +return ; +#ifdef HAVE_RT_PUSH_IPI if (sched_feat (RT_PUSH_IPI )) {tell_cpu_to_push (this_rq);