Re: NOHZ: WARNING: at arch/x86/kernel/smp.c:123 native_smp_send_reschedule, round 2

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

On 05/20/2013 01:40 PM, Frederic Weisbecker wrote:
> 2013/5/20 Borislav Petkov <bp@alien8.de>:
>> On Mon, May 20, 2013 at 11:16:33AM +0800, Michael Wang wrote:
>>> I suppose the reason is that the cpu we passed to
>>> mod_delayed_work_on() has a chance to become offline before we
>>> disabled irq, what about check it before send resched ipi? like:
>>
>> I think this is only addressing the symptoms - what we should be doing
>> instead is asking ourselves why are we even scheduling work on a cpu if
>> the machine goes offline?
>>
>> I don't know though who should be responsible for killing all that
>> work - the workqueue itself or the guy who created it, i.e. cpufreq
>> governor...
>>
>> Hmmm.
> 
> Let's look at this portion of cpu_down():
> 
> 	err = __stop_machine(take_cpu_down, &tcd_param, cpumask_of(cpu));
> 	if (err) {
> 		/* CPU didn't die: tell everyone.  Can't complain. */
> 		smpboot_unpark_threads(cpu);
> 		cpu_notify_nofail(CPU_DOWN_FAILED | mod, hcpu);
> 		goto out_release;
> 	}
> 	BUG_ON(cpu_online(cpu));
> 
> 	/*
> 	 * The migration_call() CPU_DYING callback will have removed all
> 	 * runnable tasks from the cpu, there's only the idle task left now
> 	 * that the migration thread is done doing the stop_machine thing.
> 	 *
> 	 * Wait for the stop thread to go away.
> 	 */
> 	while (!idle_cpu(cpu))
> 		cpu_relax();
> 	/* This actually kills the CPU. */
> 	__cpu_die(cpu);
> 
> 	/* CPU is completely dead: tell everyone.  Too late to complain. */
> 	cpu_notify_nofail(CPU_DEAD | mod, hcpu);
> 
> 	check_for_tasks(cpu);
> 
> The CPU is considered offline after the take_cpu_down stop machine job
> completes. But the struct timer_list timers are migrated later through
> CPU_DEAD notification. Only once that's completed we check for illegal
> residual tasks in the CPU.  So there is a little window between the
> stop machine thing and __cpu_die() where a timer can fire with
> cpu_online(cpu) == 1.
> 

Nope, the dying CPU is removed from the cpu_online_mask in the very first
stages of stop_machine(), specifically in the __cpu_disable() function.
__cpu_die() is just a dummy.

> Now concerning the workqueue I don't know. I guess the per cpu ones
> are not migrated due to their affinity. Apparently they can still wake
> up and execute works due to the timers...

The interesting thing is that the cpufreq governor actually _cancels_ the
queued work in CPU_DOWN_PREPARE stage, as far as I understand.

cpufreq_cpu_callback()
  -> __cpufreq_remove_dev()
    -> __cpufreq_governor(data, CPUFREQ_GOV_STOP);
      -> od_cpufreq_governor_dbs()
	 -> cpufreq_governor_dbs(), which has the following case statement:


        case CPUFREQ_GOV_STOP:
                if (dbs_data->cdata->governor == GOV_CONSERVATIVE)
                        cs_dbs_info->enable = 0;

                gov_cancel_work(dbs_data, policy);

                mutex_lock(&dbs_data->mutex);
                mutex_destroy(&cpu_cdbs->timer_mutex);

                mutex_unlock(&dbs_data->mutex);

                break;


But recently I removed the call to __cpufreq_remove_dev() in the suspend/resume
path (tasks frozen), in commit a66b2e503 (cpufreq: Preserve sysfs files across
suspend/resume). So I'm curious to know if this is affecting in any way.

So Boris, do you see the warnings during regular hotplug also (via sysfs) or
only during suspend/shutdown? [Actually shutdown doesn't freeze tasks, so that is
already a hint that this warning can be triggered via sysfs also, but it would
be good to get a confirmation.]

And Viresh, in the regular hotplug paths, the call to gov_cancel_work() is
supposed to kill any pending workqueue functions pertaining to offline CPUs
right? Could there be a synchronization bug somewhere due to which this
might not be happening properly?

Regards,
Srivatsa S. Bhat