Re: [PATCH 2/2] UV: NMI: simple dump failover if kdump fails

[Ingo Molnar <mingo@kernel.org>]

* George Beshers <gbeshers@sgi.com> wrote:

> UV: NMI: simple dump failover if kdump fails
>     
> The ability to trigger a kdump using the system NMI command
> was added by
> 
>     commit 12ba6c990fab50fe568f3ad8715e81e356552428
>     Author: Mike Travis <travis@sgi.com>
>     Date:   Mon Sep 23 16:25:03 2013 -0500
> 
> When kdump is works it is preferable to the set of backtraces

(spelling error)

> that dump provides; however a number of things can go wrong and
> the backtraces are much more useful than nothing.
> 
> The two most common reason for kdump not to be available are

(spelling error)

> a problem during boot or the kdump daemon fails to start.

(spelling error)

> In either case the call to crash_kexec() returns unexpectedly;
> when this happens uv_nmi_kdump() also returns with the
> uv_nmi_kexec_failed flag set.  This condition now causes a
> standard dump.

'standard dump' == printing an NMI backtrace on all CPUs?

> One other minor change is that dump now generates both the
> show_regs() stack trace and the uv_nmi_dump_ip{,_hdr} information
> that is generated by the "ips" action; the additional information
> has proved to be useful.

Looks like a useful change.

> -/* Dump this cpu's state */
> +/*
> + * Dump this cpu's state.  Note that "kdump" only happens

s/CPU's

> + * when crash_kexec() has failed and we are providing the user
> + * a standard dump instead.

So this sentence does not parse for me: kdump only happens if kdump 
fails??

> + */
>  static void uv_nmi_dump_state_cpu(int cpu, struct pt_regs *regs)
>  {
>  	const char *dots = " ................................. ";
>  
> -	if (uv_nmi_action_is("ips")) {
> -		if (cpu == 0)
> -			uv_nmi_dump_cpu_ip_hdr();
> -
> -		if (current->pid != 0)
> -			uv_nmi_dump_cpu_ip(cpu, regs);
> -
> -	} else if (uv_nmi_action_is("dump")) {
> +	if (uv_nmi_action_is("dump") || uv_nmi_action_is("kdump")) {
>  		printk(KERN_DEFAULT
>  			"UV:%sNMI process trace for CPU %d\n", dots, cpu);

pr_info().

>  		show_regs(regs);
>  	}
> +
> +	if (cpu == 0)
> +		uv_nmi_dump_cpu_ip_hdr();
> +
> +	if (current->pid != 0)
> +		uv_nmi_dump_cpu_ip(cpu, regs);

What is an 'ip header'? If it's not an Internet IP address then it's 
probably horribly named.

> +
> +#if defined(CONFIG_KEXEC)

#ifdef

> @@ -502,9 +507,9 @@ static void uv_nmi_kdump(int cpu, int master, struct pt_regs *regs)
>  		crash_kexec(regs);
>  
>  		pr_emerg("UV: crash_kexec unexpectedly returned, ");
> +		atomic_set(&uv_nmi_kexec_failed, 1);

Why is this flag an atomic variable?

Thanks,

	Ingo