On Thu, 14 Dec 2017 11:12:13 +1100
Balbir Singh [off-list ref] wrote:

On Wed, 13 Dec 2017 20:51:01 +1000
Nicholas Piggin [off-list ref] wrote:

quoted

This is looking pretty nice now...

On Wed, 13 Dec 2017 19:08:28 +1100
Balbir Singh [off-list ref] wrote:
  

quoted

@@ -543,7 +543,25 @@ void smp_send_debugger_break(void)
 #ifdef CONFIG_KEXEC_CORE
 void crash_send_ipi(void (*crash_ipi_callback)(struct pt_regs *))
 {
+	int cpu;
+
 	smp_send_nmi_ipi(NMI_IPI_ALL_OTHERS, crash_ipi_callback, 1000000);
+	if (kdump_in_progress() && crash_wake_offline) {
+		for_each_present_cpu(cpu) {
+			if (cpu_online(cpu))
+				continue;
+			/*
+			 * crash_ipi_callback will wait for
+			 * all cpus, including offline CPUs.
+			 * We don't care about nmi_ipi_function.
+			 * Offline cpus will jump straight into
+			 * crash_ipi_callback, we can skip the
+			 * entire NMI dance and waiting for
+			 * cpus to clear pending mask, etc.
+			 */
+			do_smp_send_nmi_ipi(cpu);    

Still a little bit concerned about using NMI IPI for this.
 

OK -- for offline CPUs you mean?

Yes.

quoted

If you take an NMI IPI from stop, the idle code should do the
right thing and we would just return the system reset wakeup
reason in SRR1 here (which does not need to be cleared).

If you take the system reset anywhere else in the loop, it's
going to go out via system_reset_exception. I guess that
would end up doing the right thing, it probably gets to
crash_ipi_callback from crash_kexec_secondary?  

You mean like if we are online at the time of NMI'ing? If so
the original loop will NMI us back into crash_ipi_callback
anyway. We don't expect this to occur for offline CPUs

No, if the offline CPU is executing any instruction except for
stop when the crash occurs.

quoted

It's just going to be a very untested code path :( What we
gain I suppose is better ability to handle a CPU that's locked
up somewhere in the cpu offline path. Assuming the uncommon
case works...

Actually, if you *always* go via the system reset exception
handler, then code paths will be shared. That might be the
way to go. So I would check for system reset wakeup SRR1 reason
and call replay_system_reset() for it. What do you think?
  

We could do that, but that would call pnv_system_reset_exception
and try to call the NMI function, but we've not used that path
to initiate the NMI, so it should call the stale nmi_ipi_function
which is crash_ipi_callback and not go via the crash_kexec path.

It shouldn't, if the CPU is not set in the NMI bitmask, I think
it should go back out and do the rest of the system_reset_exception
handler.

Anyway we have to get this case right, because it can already hit
as I said if the offline CPU takes the NMI when it is not stopped.
This is why I want to try to use a unified code path.

I can't call smp_send_nmi_ipi due to the nmi_ipi_busy_count and
I'm worried about calling a stale nmi_ipi_function via the
system_reset_exception path, if we are OK with it, I can revisit
the code path

You shouldn't get a stale one, that would also be a bug -- we
have to cope with NMIs coming in at any time that are triggered
externally (not by smp_send_nmi_ipi), so if you see any bugs
there those need to be fixed separately.

Thanks,
Nick