diff mbox series

x86/mce: Clean up TP_printk() output line of the mce_record tracepoint

Message ID ZgZpn/zbCJWYdL5y@gmail.com (mailing list archive)
State Handled Elsewhere
Headers show
Series x86/mce: Clean up TP_printk() output line of the mce_record tracepoint | expand

Commit Message

Ingo Molnar March 29, 2024, 7:11 a.m. UTC 
* Avadhut Naik <avadhut.naik@amd.com> wrote:

>  
> +/*
> + * MCE Event Record.
> + *
> + * Only very relevant and transient information which cannot be
> + * gathered from a system by any other means or which can only be
> + * acquired arduously should be added to this record.
> + */
> +
>  TRACE_EVENT(mce_record,
>  
>  	TP_PROTO(struct mce *m),
> @@ -25,6 +33,7 @@ TRACE_EVENT(mce_record,
>  		__field(	u64,		ipid		)
>  		__field(	u64,		ip		)
>  		__field(	u64,		tsc		)
> +		__field(	u64,		ppin		)
>  		__field(	u64,		walltime	)
>  		__field(	u32,		cpu		)
>  		__field(	u32,		cpuid		)
> @@ -45,6 +54,7 @@ TRACE_EVENT(mce_record,
>  		__entry->ipid		= m->ipid;
>  		__entry->ip		= m->ip;
>  		__entry->tsc		= m->tsc;
> +		__entry->ppin		= m->ppin;
>  		__entry->walltime	= m->time;
>  		__entry->cpu		= m->extcpu;
>  		__entry->cpuid		= m->cpuid;
> @@ -55,7 +65,7 @@ TRACE_EVENT(mce_record,
>  		__entry->cpuvendor	= m->cpuvendor;
>  	),
>  
> -	TP_printk("CPU: %d, MCGc/s: %llx/%llx, MC%d: %016Lx, IPID: %016Lx, ADDR/MISC/SYND: %016Lx/%016Lx/%016Lx, RIP: %02x:<%016Lx>, TSC: %llx, PROCESSOR: %u:%x, TIME: %llu, SOCKET: %u, APIC: %x",
> +	TP_printk("CPU: %d, MCGc/s: %llx/%llx, MC%d: %016Lx, IPID: %016Lx, ADDR/MISC/SYND: %016Lx/%016Lx/%016Lx, RIP: %02x:<%016Lx>, TSC: %llx, PPIN: %llx, processor: %u:%x, time: %llu, socket: %u, APIC: %x",

Please split out the other (capitalization) changes to the output into 
a separate patch.

- While at it, don't forget to:

   s/ADDR/MISC/SYND
    /addr/misc/synd

- Also, it's a bit weird that we have 'CPU' but also 'processor' 
  fields, why isn't it 'vendor' and 'CPUID'?

- Finally, why are some fields 'merged' as per ADDR/MISC/SYND, while 
  others are listed separately? All that have separate names should be 
  listed separately.

Ie. something like the patch below?

Thanks,

	Ingo

============>
From: Ingo Molnar <mingo@kernel.org>
Date: Fri, 29 Mar 2024 08:09:23 +0100
Subject: [PATCH] x86/mce: Clean up TP_printk() output line of the mce_record tracepoint

 - Only capitalize entries where that makes sense
 - Print separate values separately
 - Rename 'PROCESSOR' to vendor & CPUID

Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/trace/events/mce.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Naik, Avadhut March 29, 2024, 4:22 p.m. UTC | #1 
On 3/29/2024 02:11, Ingo Molnar wrote:
> 
> Please split out the other (capitalization) changes to the output into 
> a separate patch.
> 
Okay. Will put the capitalization changes into a separate patch.

> - While at it, don't forget to:
> 
>    s/ADDR/MISC/SYND
>     /addr/misc/synd
>
These are actually acronyms for Address, Miscellaneous and Syndrome registers.

It was recommended to keep the acronyms in ALL CAPS. Hence, didn't change them.

https://lore.kernel.org/linux-edac/20240327205435.3667588-1-avadhut.naik@amd.com/T/#m0c04f1c0deaa0347af66653a5950aad5f6b320e7
 
> - Also, it's a bit weird that we have 'CPU' but also 'processor' 
>   fields, why isn't it 'vendor' and 'CPUID'?
> 
Think it has been this way since the tracepoint was created back
in 2009. Will modify the field per your suggestion.

> - Finally, why are some fields 'merged' as per ADDR/MISC/SYND, while 
>   others are listed separately? All that have separate names should be 
>   listed separately.
> 
Will separate the fields so that each is listed individually.
> Ie. something like the patch below?
> 
> Thanks,
> 
> 	Ingo
> 
> ============>
> From: Ingo Molnar <mingo@kernel.org>
> Date: Fri, 29 Mar 2024 08:09:23 +0100
> Subject: [PATCH] x86/mce: Clean up TP_printk() output line of the mce_record tracepoint
> 
>  - Only capitalize entries where that makes sense
>  - Print separate values separately
>  - Rename 'PROCESSOR' to vendor & CPUID
> 
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  include/trace/events/mce.h | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/include/trace/events/mce.h b/include/trace/events/mce.h
> index 1391ada0da3b..c5b0523f25ee 100644
> --- a/include/trace/events/mce.h
> +++ b/include/trace/events/mce.h
> @@ -55,15 +55,18 @@ TRACE_EVENT(mce_record,
>  		__entry->cpuvendor	= m->cpuvendor;
>  	),
>  
> -	TP_printk("CPU: %d, MCGc/s: %llx/%llx, MC%d: %016Lx, IPID: %016Lx, ADDR/MISC/SYND: %016Lx/%016Lx/%016Lx, RIP: %02x:<%016Lx>, TSC: %llx, PROCESSOR: %u:%x, TIME: %llu, SOCKET: %u, APIC: %x",
> +	TP_printk("CPU: %d, MCGc/s: %llx/%llx, MC%d: %016Lx, IPID: %016Lx, addr: %016Lx, misc: %016Lx, SYND: %016Lx, RIP: %02x:<%016Lx>, TSC: %llx, vendor: %u, CPUID: %x, time: %llu, socket: %u, APIC: %x",
>  		__entry->cpu,
>  		__entry->mcgcap, __entry->mcgstatus,
>  		__entry->bank, __entry->status,
>  		__entry->ipid,
> -		__entry->addr, __entry->misc, __entry->synd,
> +		__entry->addr,
> +		__entry->misc,
> +		__entry->synd,
>  		__entry->cs, __entry->ip,
>  		__entry->tsc,
> -		__entry->cpuvendor, __entry->cpuid,
> +		__entry->cpuvendor,
> +		__entry->cpuid,
>  		__entry->walltime,
>  		__entry->socketid,
>  		__entry->apicid)
Tony Luck March 29, 2024, 4:50 p.m. UTC | #2 
>> - While at it, don't forget to:
>> 
>>    s/ADDR/MISC/SYND
>>     /addr/misc/synd
>>
> These are actually acronyms for Address, Miscellaneous and Syndrome registers.

They look like abbreviations, not acronyms to me.

-Tony
Naik, Avadhut March 29, 2024, 6:23 p.m. UTC | #3 
On 3/29/2024 11:50, Luck, Tony wrote:
>>> - While at it, don't forget to:
>>>
>>>    s/ADDR/MISC/SYND
>>>     /addr/misc/synd
>>>
>> These are actually acronyms for Address, Miscellaneous and Syndrome registers.
> 
> They look like abbreviations, not acronyms to me.
> 
> -Tony

Yes, they are actually abbreviations. Wrong choice of words on my part.
Was under the impression that Boris' recommendation also applied to
abbreviations. Will change them though and resubmit.
Ingo Molnar March 30, 2024, 10:38 a.m. UTC | #4 
* Naik, Avadhut <avadnaik@amd.com> wrote:

> On 3/29/2024 02:11, Ingo Molnar wrote:
> > 
> > Please split out the other (capitalization) changes to the output into 
> > a separate patch.
> > 
> Okay. Will put the capitalization changes into a separate patch.
> 
> > - While at it, don't forget to:
> > 
> >    s/ADDR/MISC/SYND
> >     /addr/misc/synd
> >
> These are actually acronyms for Address, Miscellaneous and Syndrome registers.

I kept SYND capitalized in the patch:

> > +	TP_printk("CPU: %d, MCGc/s: %llx/%llx, MC%d: %016Lx, IPID: %016Lx, addr: %016Lx, misc: %016Lx, SYND: %016Lx, RIP: %02x:<%016Lx>, TSC: %llx, vendor: %u, CPUID: %x, time: %llu, socket: %u, APIC: %x",

But I guess ADDR and MISC are fine too.

To move forward with this I've committed the cleanup in tip:x86/cpu:

  ac5e80e94f5c x86/mce: Clean up TP_printk() output line of the 'mce_record' tracepoint

... and please post extensions of the tracepoint on top of that sane(er) code.

Thanks,

	Ingo
diff mbox series

Patch

diff --git a/include/trace/events/mce.h b/include/trace/events/mce.h
index 1391ada0da3b..c5b0523f25ee 100644
--- a/include/trace/events/mce.h
+++ b/include/trace/events/mce.h
@@ -55,15 +55,18 @@  TRACE_EVENT(mce_record,
 		__entry->cpuvendor	= m->cpuvendor;
 	),
 
-	TP_printk("CPU: %d, MCGc/s: %llx/%llx, MC%d: %016Lx, IPID: %016Lx, ADDR/MISC/SYND: %016Lx/%016Lx/%016Lx, RIP: %02x:<%016Lx>, TSC: %llx, PROCESSOR: %u:%x, TIME: %llu, SOCKET: %u, APIC: %x",
+	TP_printk("CPU: %d, MCGc/s: %llx/%llx, MC%d: %016Lx, IPID: %016Lx, addr: %016Lx, misc: %016Lx, SYND: %016Lx, RIP: %02x:<%016Lx>, TSC: %llx, vendor: %u, CPUID: %x, time: %llu, socket: %u, APIC: %x",
 		__entry->cpu,
 		__entry->mcgcap, __entry->mcgstatus,
 		__entry->bank, __entry->status,
 		__entry->ipid,
-		__entry->addr, __entry->misc, __entry->synd,
+		__entry->addr,
+		__entry->misc,
+		__entry->synd,
 		__entry->cs, __entry->ip,
 		__entry->tsc,
-		__entry->cpuvendor, __entry->cpuid,
+		__entry->cpuvendor,
+		__entry->cpuid,
 		__entry->walltime,
 		__entry->socketid,
 		__entry->apicid)