Re: [RFC PATCH 1/3] powernv/mce: reduce mce console logs to lesser lines.

[Michael Ellerman <mpe@ellerman.id.au>]

Hi Mahesh,

Thanks for doing this series.

Mahesh J Salgaonkar <mahesh@linux.vnet.ibm.com> writes:
> From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>
> Also add cpu number while displaying mce log. This will help cleaner logs
> when mce hits on multiple cpus simultaneously.

Can you include some examples of the output before and after, so it's
easier to compare what the changes are.

I think you have an example in patch 3, but it would be good to have it here.

> diff --git a/arch/powerpc/kernel/mce.c b/arch/powerpc/kernel/mce.c
> index b5fec1f9751a..44614462cb34 100644
> --- a/arch/powerpc/kernel/mce.c
> +++ b/arch/powerpc/kernel/mce.c
> @@ -384,101 +387,100 @@ void machine_check_print_event_info(struct machine_check_event *evt,
>  		break;
>  	}
>  
> -	printk("%s%s Machine check interrupt [%s]\n", level, sevstr,

I think I'd still like the first line at least to include "machine
check" somewhere, I'm not sure everyone will understand what "MCE" means.

...
> +
> +	if (ea && evt->srr0 != ea)
> +		sprintf(dar_str, "DAR: %016llx ", ea);
> +	else
> +		memset(dar_str, 0, sizeof(dar_str));

Just dar_str[0] = '\0' would work wouldn't it?

> +	if (in_guest || user_mode) {
> +		printk("%sMCE: CPU%d: (%s) %s %s %s at %016llx %s[%s]\n",
> +			level, evt->cpu, sevstr,
> +			in_guest ? "Guest" : "Host",
> +			err_type, subtype, evt->srr0, dar_str,
> +			evt->disposition == MCE_DISPOSITION_RECOVERED ?
> +			"Recovered" : "Not recovered");
> +		printk("%sMCE: CPU%d: PID: %d Comm: %s\n",
> +			level, evt->cpu, current->pid, current->comm);
> +	} else {
> +		printk("%sMCE: CPU%d: (%s) Host %s %s at %016llx %s[%s]\n",
> +			level, evt->cpu, sevstr, err_type, subtype, evt->srr0,
> +			dar_str,
> +			evt->disposition == MCE_DISPOSITION_RECOVERED ?
> +			"Recovered" : "Not recovered");
> +		printk("%sMCE: CPU%d: NIP: [%016llx] %pS\n",
> +			level, evt->cpu, evt->srr0, (void *)evt->srr0);
> +	}

The first printf in the two cases is quite similar, seems like they
could be consolidated.

I also think it'd be clearer to print the NIP on the 2nd line in all
cases, rather than the first.

What about (untested) ?

  printk("%sMCE: CPU%d: (%s) %s %s %s %s[%s]\n",
	 level, evt->cpu, sevstr,
	 in_guest ? "Guest" : "Host",
	 err_type, subtype, dar_str,
	 evt->disposition == MCE_DISPOSITION_RECOVERED ?
	 "Recovered" : "Not recovered");
  
  if (in_guest || user_mode) {
	printk("%sMCE: CPU%d: PID: %d Comm: %s %sNIP: [%016llx]\n",
	       level, evt->cpu, current->pid, current->comm,
	       in_guest ? "Guest " : "", evt->srr0);
  } else {
	printk("%sMCE: CPU%d: NIP: [%016llx] %pS\n",
		level, evt->cpu, evt->srr0, (void *)evt->srr0);
  }


cheers