Re: [PATCH] powerpc/stacktrace: fix raise_backtrace_ipi() logic

[Nathan Lynch <nathanl@linux.ibm.com>]

Hi Michael,

Michael Ellerman <mpe@ellerman.id.au> writes:
> Nathan Lynch <nathanl@linux.ibm.com> writes:
>> When smp_send_safe_nmi_ipi() indicates that the target CPU has
>> responded to the IPI, skip the remote paca inspection
>> fallback. Otherwise both the sending and target CPUs attempt the
>> backtrace, usually creating a misleading ("didn't respond to backtrace
>> IPI" is wrong) and interleaved mess:
>
> Thanks for fixing my bugs for me :)
>

Thanks for your review! I was beginning to think I had missed some
subtletly here, thanks for illustrating it.

I'll run with your proposed change below for the problem I'm working.


> To solve it I think we want to avoid clearing a CPU from the mask unless
> we know that the IPI failed for that CPU. That way there's no risk of
> suppressing a trace from a CPU that successfully handles the IPI, and we
> know we've waited 5 seconds for CPUs that fail to handle the IPI.
>
> I don't think we want to allocate a whole new cpumask to track which
> CPUs have failed to respond, but I don't think we need to. We can just
> synchronously handle them.
>
> diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
> index 1deb1bf331dd..980e87f7ae7a 100644
> --- a/arch/powerpc/kernel/stacktrace.c
> +++ b/arch/powerpc/kernel/stacktrace.c
> @@ -172,17 +172,19 @@ static void handle_backtrace_ipi(struct pt_regs *regs)
>  
>  static void raise_backtrace_ipi(cpumask_t *mask)
>  {
> +	struct paca_struct *p;
>  	unsigned int cpu;
>  
>  	for_each_cpu(cpu, mask) {
> -		if (cpu == smp_processor_id())
> +		if (cpu == smp_processor_id()) {
>  			handle_backtrace_ipi(NULL);
> -		else
> -			smp_send_safe_nmi_ipi(cpu, handle_backtrace_ipi, 5 * USEC_PER_SEC);
> -	}
> +			continue;
> +		}
>  
> -	for_each_cpu(cpu, mask) {
> -		struct paca_struct *p = paca_ptrs[cpu];
> +		if (smp_send_safe_nmi_ipi(cpu, handle_backtrace_ipi, 5 * USEC_PER_SEC))
> +			continue;
> +
> +		p = paca_ptrs[cpu];
>  
>  		cpumask_clear_cpu(cpu, mask);
>