Message-ID: <4B18FB2E.1070200@in.ibm.com>
Date:	Fri, 04 Dec 2009 17:36:06 +0530
From:	Sachin Sant <sachinp@...ibm.com>
To:	Peter Zijlstra <peterz@...radead.org>
CC:	ego@...ibm.com, LKML <linux-kernel@...r.kernel.org>,
	Stephen Rothwell <sfr@...b.auug.org.au>,
	linux-next@...r.kernel.org, Ingo Molnar <mingo@...e.hu>,
	Mike Galbraith <efault@....de>,
	Gregory Haskins <ghaskins@...ell.com>, maxk <maxk@...lcomm.com>
Subject: Re: -next: Nov 12 - kernel BUG at kernel/sched.c:7359!

Peter Zijlstra wrote:
> On Mon, 2009-11-23 at 15:23 +0530, Sachin Sant wrote:
>   
>> Peter Zijlstra wrote:
>>     
>>> Well, it boots for me, but then, I've not been able to reproduce any
>>> issues anyway :/
>>>
>>> /me goes try a PREEMPT=n kernel, since that is what Mike reports boot
>>> funnies with..
>>>
>>> Full running diff against -tip:
>>>   
>>>       
>> Peter i still can recreate this issue with today's next(20091123).
>> Looks like the following patch haven't been merged yet.
>>     
>
> Correct, Ingo objected to the fastpath overhead.
>
> Could you please try the below patch which tries to address the issue
> differently.
>
>   
Peter,

Ping on this patch. Still missing from linux-next.

thanks
-Sachin

> ---
> Subject: sched: Fix balance vs hotplug race
> From: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> Date: Wed Nov 25 13:31:39 CET 2009
>
> Since (e761b77: cpu hotplug, sched: Introduce cpu_active_map and redo
> sched domain managment) we have cpu_active_mask which is suppose to
> rule scheduler migration and load-balancing, except it never did.
>
> The particular problem being solved here is a crash in
> try_to_wake_up() where select_task_rq() ends up selecting an offline
> cpu because select_task_rq_fair() trusts the sched_domain tree to reflect
> the current state of affairs, similarly select_task_rq_rt() trusts the
> root_domain.
>
> However, the sched_domains are updated from CPU_DEAD, which is after
> the cpu is taken offline and after stop_machine is done. Therefore it
> can race perfectly well with code assuming the domains are right.
>
> Cure this by building the domains from cpu_active_mask on
> CPU_DOWN_PREPARE.
>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> ---
>  include/linux/cpumask.h |    2 ++
>  kernel/cpu.c            |   18 +++++++++++++-----
>  kernel/cpuset.c         |   16 +++++++++-------
>  kernel/sched.c          |   44 ++++++++++++++++++++++++++------------------
>  4 files changed, 50 insertions(+), 30 deletions(-)
>
> Index: linux-2.6/include/linux/cpumask.h
> ===================================================================
> --- linux-2.6.orig/include/linux/cpumask.h
> +++ linux-2.6/include/linux/cpumask.h
> @@ -84,6 +84,7 @@ extern const struct cpumask *const cpu_a
>  #define num_online_cpus()	cpumask_weight(cpu_online_mask)
>  #define num_possible_cpus()	cpumask_weight(cpu_possible_mask)
>  #define num_present_cpus()	cpumask_weight(cpu_present_mask)
> +#define num_active_cpus()	cpumask_weight(cpu_active_mask)
>  #define cpu_online(cpu)		cpumask_test_cpu((cpu), cpu_online_mask)
>  #define cpu_possible(cpu)	cpumask_test_cpu((cpu), cpu_possible_mask)
>  #define cpu_present(cpu)	cpumask_test_cpu((cpu), cpu_present_mask)
> @@ -92,6 +93,7 @@ extern const struct cpumask *const cpu_a
>  #define num_online_cpus()	1
>  #define num_possible_cpus()	1
>  #define num_present_cpus()	1
> +#define num_active_cpus()	1
>  #define cpu_online(cpu)		((cpu) == 0)
>  #define cpu_possible(cpu)	((cpu) == 0)
>  #define cpu_present(cpu)	((cpu) == 0)
> Index: linux-2.6/kernel/cpuset.c
> ===================================================================
> --- linux-2.6.orig/kernel/cpuset.c
> +++ linux-2.6/kernel/cpuset.c
> @@ -872,7 +872,7 @@ static int update_cpumask(struct cpuset 
>  		if (retval < 0)
>  			return retval;
>
> -		if (!cpumask_subset(trialcs->cpus_allowed, cpu_online_mask))
> +		if (!cpumask_subset(trialcs->cpus_allowed, cpu_active_mask))
>  			return -EINVAL;
>  	}
>  	retval = validate_change(cs, trialcs);
> @@ -2010,7 +2010,7 @@ static void scan_for_empty_cpusets(struc
>  		}
>
>  		/* Continue past cpusets with all cpus, mems online */
> -		if (cpumask_subset(cp->cpus_allowed, cpu_online_mask) &&
> +		if (cpumask_subset(cp->cpus_allowed, cpu_active_mask) &&
>  		    nodes_subset(cp->mems_allowed, node_states[N_HIGH_MEMORY]))
>  			continue;
>
> @@ -2019,7 +2019,7 @@ static void scan_for_empty_cpusets(struc
>  		/* Remove offline cpus and mems from this cpuset. */
>  		mutex_lock(&callback_mutex);
>  		cpumask_and(cp->cpus_allowed, cp->cpus_allowed,
> -			    cpu_online_mask);
> +			    cpu_active_mask);
>  		nodes_and(cp->mems_allowed, cp->mems_allowed,
>  						node_states[N_HIGH_MEMORY]);
>  		mutex_unlock(&callback_mutex);
> @@ -2057,8 +2057,10 @@ static int cpuset_track_online_cpus(stru
>  	switch (phase) {
>  	case CPU_ONLINE:
>  	case CPU_ONLINE_FROZEN:
> -	case CPU_DEAD:
> -	case CPU_DEAD_FROZEN:
> +	case CPU_DOWN_PREPARE:
> +	case CPU_DOWN_PREPARE_FROZEN:
> +	case CPU_DOWN_FAILED:
> +	case CPU_DOWN_FAILED_FROZEN:
>  		break;
>
>  	default:
> @@ -2067,7 +2069,7 @@ static int cpuset_track_online_cpus(stru
>
>  	cgroup_lock();
>  	mutex_lock(&callback_mutex);
> -	cpumask_copy(top_cpuset.cpus_allowed, cpu_online_mask);
> +	cpumask_copy(top_cpuset.cpus_allowed, cpu_active_mask);
>  	mutex_unlock(&callback_mutex);
>  	scan_for_empty_cpusets(&top_cpuset);
>  	ndoms = generate_sched_domains(&doms, &attr);
> @@ -2114,7 +2116,7 @@ static int cpuset_track_online_nodes(str
>
>  void __init cpuset_init_smp(void)
>  {
> -	cpumask_copy(top_cpuset.cpus_allowed, cpu_online_mask);
> +	cpumask_copy(top_cpuset.cpus_allowed, cpu_active_mask);
>  	top_cpuset.mems_allowed = node_states[N_HIGH_MEMORY];
>
>  	hotcpu_notifier(cpuset_track_online_cpus, 0);
> Index: linux-2.6/kernel/sched.c
> ===================================================================
> --- linux-2.6.orig/kernel/sched.c
> +++ linux-2.6/kernel/sched.c
> @@ -2323,6 +2323,12 @@ void task_oncpu_function_call(struct tas
>  	preempt_enable();
>  }
>
> +static inline
> +int select_task_rq(struct task_struct *p, int sd_flags, int wake_flags)
> +{
> +	return p->sched_class->select_task_rq(p, sd_flags, wake_flags);
> +}
> +
>  /***
>   * try_to_wake_up - wake up a thread
>   * @p: the to-be-woken-up thread
> @@ -2376,7 +2382,7 @@ static int try_to_wake_up(struct task_st
>  	p->state = TASK_WAKING;
>  	task_rq_unlock(rq, &flags);
>
> -	cpu = p->sched_class->select_task_rq(p, SD_BALANCE_WAKE, wake_flags);
> +	cpu = select_task_rq(p, SD_BALANCE_WAKE, wake_flags);
>  	if (cpu != orig_cpu) {
>  		local_irq_save(flags);
>  		rq = cpu_rq(cpu);
> @@ -2593,7 +2599,7 @@ void sched_fork(struct task_struct *p, i
>  		p->sched_class = &fair_sched_class;
>
>  #ifdef CONFIG_SMP
> -	cpu = p->sched_class->select_task_rq(p, SD_BALANCE_FORK, 0);
> +	cpu = select_task_rq(p, SD_BALANCE_FORK, 0);
>  #endif
>  	local_irq_save(flags);
>  	update_rq_clock(cpu_rq(cpu));
> @@ -3156,7 +3162,7 @@ out:
>  void sched_exec(void)
>  {
>  	int new_cpu, this_cpu = get_cpu();
> -	new_cpu = current->sched_class->select_task_rq(current, SD_BALANCE_EXEC, 0);
> +	new_cpu = select_task_rq(current, SD_BALANCE_EXEC, 0);
>  	put_cpu();
>  	if (new_cpu != this_cpu)
>  		sched_migrate_task(current, new_cpu);
> @@ -4134,7 +4140,7 @@ static int load_balance(int this_cpu, st
>  	unsigned long flags;
>  	struct cpumask *cpus = __get_cpu_var(load_balance_tmpmask);
>
> -	cpumask_copy(cpus, cpu_online_mask);
> +	cpumask_copy(cpus, cpu_active_mask);
>
>  	/*
>  	 * When power savings policy is enabled for the parent domain, idle
> @@ -4297,7 +4303,7 @@ load_balance_newidle(int this_cpu, struc
>  	int all_pinned = 0;
>  	struct cpumask *cpus = __get_cpu_var(load_balance_tmpmask);
>
> -	cpumask_copy(cpus, cpu_online_mask);
> +	cpumask_copy(cpus, cpu_active_mask);
>
>  	/*
>  	 * When power savings policy is enabled for the parent domain, idle
> @@ -4694,7 +4700,7 @@ int select_nohz_load_balancer(int stop_t
>  		cpumask_set_cpu(cpu, nohz.cpu_mask);
>
>  		/* time for ilb owner also to sleep */
> -		if (cpumask_weight(nohz.cpu_mask) == num_online_cpus()) {
> +		if (cpumask_weight(nohz.cpu_mask) == num_active_cpus()) {
>  			if (atomic_read(&nohz.load_balancer) == cpu)
>  				atomic_set(&nohz.load_balancer, -1);
>  			return 0;
> @@ -7071,7 +7077,7 @@ int set_cpus_allowed_ptr(struct task_str
>  	int ret = 0;
>
>  	rq = task_rq_lock(p, &flags);
> -	if (!cpumask_intersects(new_mask, cpu_online_mask)) {
> +	if (!cpumask_intersects(new_mask, cpu_active_mask)) {
>  		ret = -EINVAL;
>  		goto out;
>  	}
> @@ -7093,7 +7099,7 @@ int set_cpus_allowed_ptr(struct task_str
>  	if (cpumask_test_cpu(task_cpu(p), new_mask))
>  		goto out;
>
> -	if (migrate_task(p, cpumask_any_and(cpu_online_mask, new_mask), &req)) {
> +	if (migrate_task(p, cpumask_any_and(cpu_active_mask, new_mask), &req)) {
>  		/* Need help from migration thread: drop lock and wait. */
>  		struct task_struct *mt = rq->migration_thread;
>
> @@ -7247,19 +7253,19 @@ static void move_task_off_dead_cpu(int d
>
>  again:
>  	/* Look for allowed, online CPU in same node. */
> -	for_each_cpu_and(dest_cpu, nodemask, cpu_online_mask)
> +	for_each_cpu_and(dest_cpu, nodemask, cpu_active_mask)
>  		if (cpumask_test_cpu(dest_cpu, &p->cpus_allowed))
>  			goto move;
>
>  	/* Any allowed, online CPU? */
> -	dest_cpu = cpumask_any_and(&p->cpus_allowed, cpu_online_mask);
> +	dest_cpu = cpumask_any_and(&p->cpus_allowed, cpu_active_mask);
>  	if (dest_cpu < nr_cpu_ids)
>  		goto move;
>
>  	/* No more Mr. Nice Guy. */
>  	if (dest_cpu >= nr_cpu_ids) {
>  		cpuset_cpus_allowed_locked(p, &p->cpus_allowed);
> -		dest_cpu = cpumask_any_and(cpu_online_mask, &p->cpus_allowed);
> +		dest_cpu = cpumask_any_and(cpu_active_mask, &p->cpus_allowed);
>
>  		/*
>  		 * Don't tell them about moving exiting tasks or
> @@ -7288,7 +7294,7 @@ move:
>   */
>  static void migrate_nr_uninterruptible(struct rq *rq_src)
>  {
> -	struct rq *rq_dest = cpu_rq(cpumask_any(cpu_online_mask));
> +	struct rq *rq_dest = cpu_rq(cpumask_any(cpu_active_mask));
>  	unsigned long flags;
>
>  	local_irq_save(flags);
> @@ -7542,7 +7548,7 @@ static ctl_table *sd_alloc_ctl_cpu_table
>  static struct ctl_table_header *sd_sysctl_header;
>  static void register_sched_domain_sysctl(void)
>  {
> -	int i, cpu_num = num_online_cpus();
> +	int i, cpu_num = num_possible_cpus();
>  	struct ctl_table *entry = sd_alloc_ctl_entry(cpu_num + 1);
>  	char buf[32];
>
> @@ -7552,7 +7558,7 @@ static void register_sched_domain_sysctl
>  	if (entry == NULL)
>  		return;
>
> -	for_each_online_cpu(i) {
> +	for_each_possible_cpu(i) {
>  		snprintf(buf, 32, "cpu%d", i);
>  		entry->procname = kstrdup(buf, GFP_KERNEL);
>  		entry->mode = 0555;
> @@ -9064,7 +9070,7 @@ match1:
>  	if (doms_new == NULL) {
>  		ndoms_cur = 0;
>  		doms_new = &fallback_doms;
> -		cpumask_andnot(doms_new[0], cpu_online_mask, cpu_isolated_map);
> +		cpumask_andnot(doms_new[0], cpu_active_mask, cpu_isolated_map);
>  		WARN_ON_ONCE(dattr_new);
>  	}
>
> @@ -9195,8 +9201,10 @@ static int update_sched_domains(struct n
>  	switch (action) {
>  	case CPU_ONLINE:
>  	case CPU_ONLINE_FROZEN:
> -	case CPU_DEAD:
> -	case CPU_DEAD_FROZEN:
> +	case CPU_DOWN_PREPARE:
> +	case CPU_DOWN_PREPARE_FROZEN:
> +	case CPU_DOWN_FAILED:
> +	case CPU_DOWN_FAILED_FROZEN:
>  		partition_sched_domains(1, NULL, NULL);
>  		return NOTIFY_OK;
>
> @@ -9243,7 +9251,7 @@ void __init sched_init_smp(void)
>  #endif
>  	get_online_cpus();
>  	mutex_lock(&sched_domains_mutex);
> -	arch_init_sched_domains(cpu_online_mask);
> +	arch_init_sched_domains(cpu_active_mask);
>  	cpumask_andnot(non_isolated_cpus, cpu_possible_mask, cpu_isolated_map);
>  	if (cpumask_empty(non_isolated_cpus))
>  		cpumask_set_cpu(smp_processor_id(), non_isolated_cpus);
> Index: linux-2.6/kernel/cpu.c
> ===================================================================
> --- linux-2.6.orig/kernel/cpu.c
> +++ linux-2.6/kernel/cpu.c
> @@ -212,6 +212,8 @@ static int __ref _cpu_down(unsigned int 
>  	err = __raw_notifier_call_chain(&cpu_chain, CPU_DOWN_PREPARE | mod,
>  					hcpu, -1, &nr_calls);
>  	if (err == NOTIFY_BAD) {
> +		set_cpu_active(cpu, true);
> +
>  		nr_calls--;
>  		__raw_notifier_call_chain(&cpu_chain, CPU_DOWN_FAILED | mod,
>  					  hcpu, nr_calls, NULL);
> @@ -223,11 +225,11 @@ static int __ref _cpu_down(unsigned int 
>
>  	/* Ensure that we are not runnable on dying cpu */
>  	cpumask_copy(old_allowed, &current->cpus_allowed);
> -	set_cpus_allowed_ptr(current,
> -			     cpumask_of(cpumask_any_but(cpu_online_mask, cpu)));
> +	set_cpus_allowed_ptr(current, cpu_active_mask);
>
>  	err = __stop_machine(take_cpu_down, &tcd_param, cpumask_of(cpu));
>  	if (err) {
> +		set_cpu_active(cpu, true);
>  		/* CPU didn't die: tell everyone.  Can't complain. */
>  		if (raw_notifier_call_chain(&cpu_chain, CPU_DOWN_FAILED | mod,
>  					    hcpu) == NOTIFY_BAD)
> @@ -292,9 +294,6 @@ int __ref cpu_down(unsigned int cpu)
>
>  	err = _cpu_down(cpu, 0);
>
> -	if (cpu_online(cpu))
> -		set_cpu_active(cpu, true);
> -
>  out:
>  	cpu_maps_update_done();
>  	stop_machine_destroy();
> @@ -387,6 +386,15 @@ int disable_nonboot_cpus(void)
>  	 * with the userspace trying to use the CPU hotplug at the same time
>  	 */
>  	cpumask_clear(frozen_cpus);
> +
> +	for_each_online_cpu(cpu) {
> +		if (cpu == first_cpu)
> +			continue;
> +		set_cpu_active(cpu, false);
> +	}
> +
> +	synchronize_sched();
> +
>  	printk("Disabling non-boot CPUs ...\n");
>  	for_each_online_cpu(cpu) {
>  		if (cpu == first_cpu)
>
>
>   


-- 

---------------------------------
Sachin Sant
IBM Linux Technology Center
India Systems and Technology Labs
Bangalore, India
---------------------------------

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists