Re: [PATCH] x86/mce: Clean up TP_printk() output line of the mce_record tracepoint
* Naik, Avadhut 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
[PATCH] x86/mce: Clean up TP_printk() output line of the mce_record tracepoint
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. -- Thanks, Avadhut Naik
RE: [PATCH] x86/mce: Clean up TP_printk() output line of the mce_record tracepoint
>> - 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
[PATCH] x86/mce: Clean up TP_printk() output line of the mce_record tracepoint
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.n...@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
> 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
> ---
> 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)
--
Thanks,
Avadhut Naik
[PATCH] x86/mce: Clean up TP_printk() output line of the mce_record tracepoint
* Avadhut Naik 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
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
---
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)
