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

[Michael Ellerman]

Hi Mahesh,

Thanks for doing this series.

Mahesh J Salgaonkar <mah...@linux.vnet.ibm.com> writes:
> From: Mahesh Salgaonkar <mah...@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