lkml.org 
[lkml]   [2012]   [Jun]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] sched/cpu hotplug: Don't traverse sched domain tree of cpus going offline
On 06/03/2012 12:27 AM, Srivatsa S. Bhat wrote:

> The cpu_active_mask is used to help the scheduler make the right decisions
> during CPU hotplug transitions, and thereby enable CPU hotplug operations to
> run smoothly.
>
> However there are a few places where the scheduler doesn't consult the
> cpu_active_mask and hence can trip over during CPU hotplug operations, by
> making decisions based on stale data structures.
>
> In particular, the call to partition_sched_domains() in the CPU offline path
> sets cpu_rq(dying_cpu)->sd (sched domain pointer) to NULL. However, until
> we get to that point, it remains non-NULL. And during that time period,
> the scheduler could end up traversing that non-NULL sched domain tree and
> making decisions based on that, which could lead to undesirable consequences.
>
> For example, commit 8f2f748 (CPU hotplug, cpusets, suspend: Don't touch
> cpusets during suspend/resume) caused suspend hangs at the CPU hotplug stage,
> in some configurations. And the root-cause of that hang was that the
> scheduler was traversing the sched domain trees of cpus going offline, thereby
> messing up its decisions. The analysis of that hang can be found in:
> http://marc.info/?l=linux-kernel&m=133518704022532&w=4
>
> In short, our assumption that we are good to go (even with stale sched
> domains) as long as the dying cpu was not in the cpu_active_mask, was wrong.
> Because, there are several places where the scheduler code doesn't really
> check the cpu_active_mask, but instead traverses the sched domains directly
> to do stuff.
>
> Anyway, that commit got reverted. However, that same problem could very well
> occur during regular CPU hotplug too, even now. Ideally, we would want the
> scheduler to make all its decisions during the CPU hotplug transition, by
> looking up the cpu_active_mask (which is updated very early during CPU
> Hotplug) to avoid such disasters. So, strengthen the scheduler by making it
> consult the cpu_active_mask as a validity check before traversing the sched
> domain tree.
>
> Here we are specifically fixing the 2 known instances: idle_balance() and
> find_lowest_rq().
>
> 1. kernel/sched/core.c and fair.c:
> schedule()
> __schedule()
> idle_balance()
>
> idle_balance() can end up doing:
>
> for_each_domain(dying_cpu, sd) {
> ...
> }
>
> 2. kernel/sched/core.c and rt.c:
>
> migration_call()
> sched_ttwu_pending()
> ttwu_do_activate()
> ttwu_do_wakeup()
> task_woken()
> task_woken_rt()
> push_rt_tasks()
> find_lowest_rq()
>
> find_lowest_rq() can end up doing:
>
> for_each_domain(dying_cpu, sd) {
> ...
> }

>

> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>


Any thoughts on this patch?

Regards,
Srivatsa S. Bhat

> ---
>
> kernel/sched/fair.c | 5 +++++
> kernel/sched/rt.c | 4 ++++
> 2 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 940e6d1..72cc7c6 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4402,6 +4402,10 @@ void idle_balance(int this_cpu, struct rq *this_rq)
> raw_spin_unlock(&this_rq->lock);
>
> update_shares(this_cpu);
> +
> + if (!cpu_active(this_cpu))
> + goto out;
> +
> rcu_read_lock();
> for_each_domain(this_cpu, sd) {
> unsigned long interval;
> @@ -4426,6 +4430,7 @@ void idle_balance(int this_cpu, struct rq *this_rq)
> }
> rcu_read_unlock();
>
> + out:
> raw_spin_lock(&this_rq->lock);
>
> if (pulled_task || time_after(jiffies, this_rq->next_balance)) {
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index c5565c3..8cbafcd 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -1488,6 +1488,9 @@ static int find_lowest_rq(struct task_struct *task)
> if (!cpumask_test_cpu(this_cpu, lowest_mask))
> this_cpu = -1; /* Skip this_cpu opt if not among lowest */
>
> + if (!cpu_active(cpu))
> + goto out;
> +
> rcu_read_lock();
> for_each_domain(cpu, sd) {
> if (sd->flags & SD_WAKE_AFFINE) {
> @@ -1513,6 +1516,7 @@ static int find_lowest_rq(struct task_struct *task)
> }
> rcu_read_unlock();
>
> + out:
> /*
> * And finally, if there were no matches within the domains
> * just give the caller *something* to work with from the compatible
>




\
 
 \ /
  Last update: 2012-06-20 12:41    [W:0.055 / U:0.076 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site