| Message ID | 20231220-cxl-cper-v5-4-1bb8a4ca2c7a@intel.com |
|---|---|
| State | Accepted |
| Commit | 207a1f82301de0b4123f00a8d26ea55bb2484757 |
| Headers | show |
| Series | efi/cxl-cper: Report CPER CXL component events through trace events | expand |
On Wed, 20 Dec 2023 16:17:31 -0800 Ira Weiny <ira.weiny@intel.com> wrote: > The UUID data is redundant in the known event trace types. The addition > of static defines allows the trace macros to create the UUID data inside > the trace thus removing unnecessary code. > > Have well known trace events use static data to set the uuid field based > on the event type. > > Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > TP_STRUCT__entry( > CXL_EVT_TP_entry > @@ -422,7 +424,8 @@ TRACE_EVENT(cxl_dram, > ), > > TP_fast_assign( > - CXL_EVT_TP_fast_assign(cxlmd, log, uuid, rec->hdr); > + CXL_EVT_TP_fast_assign(cxlmd, log, rec->hdr); > + memcpy(&__entry->hdr_uuid, &CXL_EVENT_DRAM_UUID, sizeof(uuid_t)); Hmm. Why not __entry->hdr_uuid = CXL_EVENT_DRAM_UUID; ? Compiler should be able to squish the stuff in the define down to data as as the UUID generation logic is pretty simple. I've not emulated the cper records for these yet, so not tested that works beyond compiling. J
Jonathan Cameron wrote: > On Wed, 20 Dec 2023 16:17:31 -0800 > Ira Weiny <ira.weiny@intel.com> wrote: > > > The UUID data is redundant in the known event trace types. The addition > > of static defines allows the trace macros to create the UUID data inside > > the trace thus removing unnecessary code. > > > > Have well known trace events use static data to set the uuid field based > > on the event type. > > > > Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > > > > TP_STRUCT__entry( > > CXL_EVT_TP_entry > > @@ -422,7 +424,8 @@ TRACE_EVENT(cxl_dram, > > ), > > > > TP_fast_assign( > > - CXL_EVT_TP_fast_assign(cxlmd, log, uuid, rec->hdr); > > + CXL_EVT_TP_fast_assign(cxlmd, log, rec->hdr); > > + memcpy(&__entry->hdr_uuid, &CXL_EVENT_DRAM_UUID, sizeof(uuid_t)); > > Hmm. Why not > > __entry->hdr_uuid = CXL_EVENT_DRAM_UUID; > ? > > Compiler should be able to squish the stuff in the define down to data as as the > UUID generation logic is pretty simple. > > I've not emulated the cper records for these yet, so not tested that works beyond > compiling. We can follow on with this conversion later as I see other usage of uuid copying in trace events (bcache for instance). Although I probably would not replace it with straight assignment and instead use the uuid_copy() helper. Otherwise, why do {uuid,guid}_copy() helpers exist?
On Tue, 9 Jan 2024 15:38:28 -0800 Dan Williams <dan.j.williams@intel.com> wrote: > Jonathan Cameron wrote: > > On Wed, 20 Dec 2023 16:17:31 -0800 > > Ira Weiny <ira.weiny@intel.com> wrote: > > > > > The UUID data is redundant in the known event trace types. The addition > > > of static defines allows the trace macros to create the UUID data inside > > > the trace thus removing unnecessary code. > > > > > > Have well known trace events use static data to set the uuid field based > > > on the event type. > > > > > > Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > > > > > > > TP_STRUCT__entry( > > > CXL_EVT_TP_entry > > > @@ -422,7 +424,8 @@ TRACE_EVENT(cxl_dram, > > > ), > > > > > > TP_fast_assign( > > > - CXL_EVT_TP_fast_assign(cxlmd, log, uuid, rec->hdr); > > > + CXL_EVT_TP_fast_assign(cxlmd, log, rec->hdr); > > > + memcpy(&__entry->hdr_uuid, &CXL_EVENT_DRAM_UUID, sizeof(uuid_t)); > > > > Hmm. Why not > > > > __entry->hdr_uuid = CXL_EVENT_DRAM_UUID; > > ? > > > > Compiler should be able to squish the stuff in the define down to data as as the > > UUID generation logic is pretty simple. > > > > I've not emulated the cper records for these yet, so not tested that works beyond > > compiling. > > We can follow on with this conversion later as I see other usage of uuid > copying in trace events (bcache for instance). Although I probably would > not replace it with straight assignment and instead use the uuid_copy() > helper. Otherwise, why do {uuid,guid}_copy() helpers exist? To copy unknown uuids and guids where the compiler can't optimize things nearly as well because it can't see the values. Jonathan
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index 1ccc3a56e0af..5f3681de10de 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -846,16 +846,16 @@ static void cxl_event_trace_record(const struct cxl_memdev *cxlmd, struct cxl_event_gen_media *rec = (struct cxl_event_gen_media *)record; - trace_cxl_general_media(cxlmd, type, id, rec); + trace_cxl_general_media(cxlmd, type, rec); } else if (uuid_equal(id, &CXL_EVENT_DRAM_UUID)) { struct cxl_event_dram *rec = (struct cxl_event_dram *)record; - trace_cxl_dram(cxlmd, type, id, rec); + trace_cxl_dram(cxlmd, type, rec); } else if (uuid_equal(id, &CXL_EVENT_MEM_MODULE_UUID)) { struct cxl_event_mem_module *rec = (struct cxl_event_mem_module *)record; - trace_cxl_memory_module(cxlmd, type, id, rec); + trace_cxl_memory_module(cxlmd, type, rec); } else { /* For unknown record types print just the header */ trace_cxl_generic_event(cxlmd, type, id, record); diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h index 3da16026b8db..312cfa9e0004 100644 --- a/drivers/cxl/core/trace.h +++ b/drivers/cxl/core/trace.h @@ -181,6 +181,7 @@ TRACE_EVENT(cxl_overflow, * 1) Add CXL_EVT_TP_entry to TP_STRUCT__entry * 2) Use CXL_EVT_TP_fast_assign within TP_fast_assign; * pass the dev, log, and CXL event header + * NOTE: The uuid must be assigned by the specific trace event * 3) Use CXL_EVT_TP_printk() instead of TP_printk() * * See the generic_event tracepoint as an example. @@ -198,12 +199,11 @@ TRACE_EVENT(cxl_overflow, __field(u8, hdr_length) \ __field(u8, hdr_maint_op_class) -#define CXL_EVT_TP_fast_assign(cxlmd, l, uuid, hdr) \ +#define CXL_EVT_TP_fast_assign(cxlmd, l, hdr) \ __assign_str(memdev, dev_name(&(cxlmd)->dev)); \ __assign_str(host, dev_name((cxlmd)->dev.parent)); \ __entry->log = (l); \ __entry->serial = (cxlmd)->cxlds->serial; \ - memcpy(&__entry->hdr_uuid, (uuid), sizeof(uuid_t)); \ __entry->hdr_length = (hdr).length; \ __entry->hdr_flags = get_unaligned_le24((hdr).flags); \ __entry->hdr_handle = le16_to_cpu((hdr).handle); \ @@ -235,7 +235,8 @@ TRACE_EVENT(cxl_generic_event, ), TP_fast_assign( - CXL_EVT_TP_fast_assign(cxlmd, log, uuid, rec->hdr); + CXL_EVT_TP_fast_assign(cxlmd, log, rec->hdr); + memcpy(&__entry->hdr_uuid, uuid, sizeof(uuid_t)); memcpy(__entry->data, &rec->data, CXL_EVENT_RECORD_DATA_LENGTH); ), @@ -315,9 +316,9 @@ TRACE_EVENT(cxl_generic_event, TRACE_EVENT(cxl_general_media, TP_PROTO(const struct cxl_memdev *cxlmd, enum cxl_event_log_type log, - const uuid_t *uuid, struct cxl_event_gen_media *rec), + struct cxl_event_gen_media *rec), - TP_ARGS(cxlmd, log, uuid, rec), + TP_ARGS(cxlmd, log, rec), TP_STRUCT__entry( CXL_EVT_TP_entry @@ -336,7 +337,8 @@ TRACE_EVENT(cxl_general_media, ), TP_fast_assign( - CXL_EVT_TP_fast_assign(cxlmd, log, uuid, rec->hdr); + CXL_EVT_TP_fast_assign(cxlmd, log, rec->hdr); + memcpy(&__entry->hdr_uuid, &CXL_EVENT_GEN_MEDIA_UUID, sizeof(uuid_t)); /* General Media */ __entry->dpa = le64_to_cpu(rec->phys_addr); @@ -398,9 +400,9 @@ TRACE_EVENT(cxl_general_media, TRACE_EVENT(cxl_dram, TP_PROTO(const struct cxl_memdev *cxlmd, enum cxl_event_log_type log, - const uuid_t *uuid, struct cxl_event_dram *rec), + struct cxl_event_dram *rec), - TP_ARGS(cxlmd, log, uuid, rec), + TP_ARGS(cxlmd, log, rec), TP_STRUCT__entry( CXL_EVT_TP_entry @@ -422,7 +424,8 @@ TRACE_EVENT(cxl_dram, ), TP_fast_assign( - CXL_EVT_TP_fast_assign(cxlmd, log, uuid, rec->hdr); + CXL_EVT_TP_fast_assign(cxlmd, log, rec->hdr); + memcpy(&__entry->hdr_uuid, &CXL_EVENT_DRAM_UUID, sizeof(uuid_t)); /* DRAM */ __entry->dpa = le64_to_cpu(rec->phys_addr); @@ -547,9 +550,9 @@ TRACE_EVENT(cxl_dram, TRACE_EVENT(cxl_memory_module, TP_PROTO(const struct cxl_memdev *cxlmd, enum cxl_event_log_type log, - const uuid_t *uuid, struct cxl_event_mem_module *rec), + struct cxl_event_mem_module *rec), - TP_ARGS(cxlmd, log, uuid, rec), + TP_ARGS(cxlmd, log, rec), TP_STRUCT__entry( CXL_EVT_TP_entry @@ -569,7 +572,8 @@ TRACE_EVENT(cxl_memory_module, ), TP_fast_assign( - CXL_EVT_TP_fast_assign(cxlmd, log, uuid, rec->hdr); + CXL_EVT_TP_fast_assign(cxlmd, log, rec->hdr); + memcpy(&__entry->hdr_uuid, &CXL_EVENT_MEM_MODULE_UUID, sizeof(uuid_t)); /* Memory Module Event */ __entry->event_type = rec->event_type;
The UUID data is redundant in the known event trace types. The addition of static defines allows the trace macros to create the UUID data inside the trace thus removing unnecessary code. Have well known trace events use static data to set the uuid field based on the event type. Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com> Signed-off-by: Ira Weiny <ira.weiny@intel.com> --- Changes for v5: New patch --- drivers/cxl/core/mbox.c | 6 +++--- drivers/cxl/core/trace.h | 28 ++++++++++++++++------------ 2 files changed, 19 insertions(+), 15 deletions(-)