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