Re: [RFC] powerpc/pseries: Increase busy loop in pseries_cpu_die

Thiago Jung Bauermann [off-list ref] writes:

Am Dienstag, 7. Februar 2017, 08:26:45 BRST schrieb Balbir Singh:

quoted

On Mon, Feb 06, 2017 at 04:58:16PM -0200, Thiago Jung Bauermann wrote:

quoted

[  447.714064] Querying DEAD? cpu 134 (134) shows 2
cpu 0x86: Vector: 300 (Data Access) at [c000000007b0fd40]

    pc: 000000001ec3072c
    lr: 000000001ec2fee0
    sp: 1faf6bd0
   
   msr: 8000000102801000
   dar: 212d6c1a2a20c

This looks like we accessed a bad address, but why?

Am Dienstag, 7. Februar 2017, 13:10:22 BRST schrieb Michael Ellerman:

quoted

We shouldn't be crashing.

So we need to fix that.

We may also need to increase the timeout, though it's pretty gross TBH.

But step one is make sure we don't crash.

I didn't analyze exactly what is causing the CPU to crash because the root 
cause is the inconsistency between what the kernel thinks the CPU state is and 
reality. But if we have to be able to handle that inconsistency I will keep 
digging and try to fix that.

Please do.

It's obviously not a good situation, but it shouldn't crash the kernel.

I realise that may be easier said than done, but that should be the
goal. I see smp_ops->cpu_die() can't fail (returns void), which may be
part of the problem.

quoted

quoted

--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c

@@ -206,7 +206,7 @@ static void pseries_cpu_die(unsigned int cpu)

 		}
 	
 	} else if (get_preferred_offline_state(cpu) == CPU_STATE_OFFLINE) {

-		for (tries = 0; tries < 25; tries++) {
+		for (tries = 0; tries < 5000; tries++) {

This fixes some of the asymmetry between handling of CPU_STATE_INACTIVE
and CPU_STATE_OFFLINE, but I think we can probably move the cpu_relax()
to msleep(1).

I didn't change it to msleep() because I thought it would introduce a 
regression. commit b906cfa397fd ("powerpc/pseries: Fix cpu hotplug") changed a 
msleep(200) that was there to a cpu_relax() with this explanation:

    Currently, pseries_cpu_die() calls msleep() while polling RTAS for 
    the status of the dying cpu.

    However, if the cpu that is going down also happens to be the one 
    doing the tick then we're hosed as the tick_do_timer_cpu 'baton' is
    only passed later on in tick_shutdown() when _cpu_down() does the 
    CPU_DEAD notification.  Therefore jiffies won't be updated anymore.

    This replaces that msleep() with a cpu_relax() to make sure we're not 
    going to schedule at that point.

    With this patch my test box survives a 100k iterations hotplug stress
    test on _all_ cpus, whereas without it, it quickly dies after ~50 
    iterations.

I can try to add it back and see what happens. Perhaps that situation won't 
happen anymore with today's kernel.

The relative timing of the events may have changed. But the fundamental
problem still exists AFAIK.

So we'd need a pretty solid guarantee that the problem can't happy
anymore before I'd change it to msleep().

However it could/should at least use udelay(), which AFAIK is not
reliant on jiffies. That way the loop will actually take a set amount of
wall time, rather than just cycles.

cheers