Re: [PATCH 32/35] clockevents: Fix cpu down race for hrtimer based broadcasting

From: Peter Zijlstra <peterz@infradead.org>
To: Nicolas Pitre <nico@linaro.org>
Cc: linux-kernel@vger.kernel.org, mingo@kernel.org,
	rjw@rjwysocki.net, tglx@linutronix.de,
	Preeti U Murthy <preeti@linux.vnet.ibm.com>
Subject: Re: [PATCH 32/35] clockevents: Fix cpu down race for hrtimer based broadcasting
Date: Mon, 23 Feb 2015 17:14:57 +0100	[thread overview]
Message-ID: <20150223161457.GA5029@twins.programming.kicks-ass.net> (raw)
In-Reply-To: <alpine.LFD.2.11.1502211135230.25484@knanqh.ubzr>

On Sat, Feb 21, 2015 at 12:45:51PM -0500, Nicolas Pitre wrote:
> > I'm having a very hard time trying to follow wth this thing all is
> > doing; its using hotplug but its also doing magic with cpu_suspend().
> 
> You're fully aware of the on-going work on the scheduler to better 
> support the big.LITTLE architecture amongst other things.  The switcher 
> is an interim solution where one big CPU is paired with one little CPU, 
> and that pair is conceptually used as one logical CPU where only one of 
> the big or little physical CPU runs at a time. Those logical CPUs have 
> identical capacities therefore the current scheduler may works well with 
> them.

Yeah, I'm aware of roughly what and why its doing things.

But I was trying to piece together how exactly stuff gets done so I
could figure out why that patch broke stuff.

So while it appears to use the regular hotplug code for some of the
work, it also has some extra magic on top to affect the actual
switcheroo of the cpu.

In any case, having had a second look I think I might have some ideas:

 - bL_switcher_enable() -- enables the whole switcher thing and
   disables half the cpus with hot-un-plug, creates a mapping etc..

 - bL_switcher_disable() -- disabled the whole switcher thing and
   gives us back all our cpus with hot-plug.

When the switcher is enabled; we switch by this magic cpu_suspend() call
that saves the entire cpu state and allows you to restore it on another
cpu.

You muck about with the tick; you disable it before cpu_suspend() and
re-enable it after on the target cpu. You further reprogram the
interrupt routing from the old to the new cpu.

But that appears to be it, no more. I suppose the tick is special
because its the only per-cpu device?

The reported function that fails: bL_switcher_restore_cpus() is called
in the error paths of the former and the main path in the latter to make
the 'stolen' cpus re-appear.

The patch in question somehow makes that go boom.

Now what all do you need to do to make it go boom? Just enable/disable
the switcher once and it'll explode? Or does it need to do actual
switches while it is enabled?

The place where it explodes is a bit surprising, it thinks hrtimers are
not enabled even though its calling into hrtimer code on that cpu...