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

[Mahesh Jagannath Salgaonkar <mahesh@linux.vnet.ibm.com>]

On 3/29/19 5:50 AM, Michael Ellerman wrote:
> 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.

Sure will add that in next revision.

> 
> 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.

I reduced it to MCE so that I can pack in more stuff in 80 columns. But
you are right, let me see.

> 
> ...
>> +
>> +	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?

Yeah, that also should be enough.

> 
>> +	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.

Sure, and I will then put "machine check" on 1st line like below ?

printk("%sMCE: CPU%d: machine check (%s) %s %s %s %s[%s]\n",


> 
> 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);
>   }

Sure, will make these changes.

Thanks,
-Mahesh.