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

[Ingo Molnar <mingo@kernel.org>]

* Mike Travis <travis@sgi.com> wrote:

> 
> 
> On 5/1/2015 12:27 AM, Ingo Molnar wrote:
> > 
> > * 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?
> 
> Yes.
> > 
> >> 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.
> 
> The IP or Instruction Pointer register.  The "show ips" is sort of a
> simplified ps showing the processes on non-idle CPUs.  We'd need to
> blame Intel for that name... :)

Yes, but this is 64-bit code, why not call it RIP? :-)

that's kind of not unambiguous either, but at least in technical 
discussions it should be ;-)

So what I found confusing is the ip_hdr - that sounds very network-ish 
...

> Currently you can have either the IPs or the stack dump, but both 
> contain useful info.  So George's idea was if you asked for the dump 
> you'd get both, if you asked only for IPs, you'd just get them.

Yeah, I'm not against the idea at all. The patch needs a bit of a face 
lift and then it looks good to me.

Thanks,

	Ingo