| Message ID | 20230601-cxl-cper-v3-1-0189d61f7956@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | efi/cxl-cper: Report CPER CXL component events through trace events | expand |
On Wed, 01 Nov 2023 14:11:18 -0700 Ira Weiny <ira.weiny@intel.com> wrote: > The uuid printed in the well known events is redundant. The uuid > defines what the event was. > > Remove the uuid from the known events and only report it in the generic > event as it remains informative there. > > Reviewed-by: Davidlohr Bueso <dave@stgolabs.net> > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Ira Weiny <ira.weiny@intel.com> Removing the print is fine, but look like this also removes the actual trace point field. That's userspace ABI. Expanding it is fine, but taking fields away is more problematic. Are we sure we don't break anyone? Shiju, will rasdaemon be fine with this change? Thanks, Jonathan > --- > drivers/cxl/core/trace.h | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h > index a0b5819bc70b..79ed03637604 100644 > --- a/drivers/cxl/core/trace.h > +++ b/drivers/cxl/core/trace.h > @@ -189,7 +189,6 @@ TRACE_EVENT(cxl_overflow, > __string(memdev, dev_name(&cxlmd->dev)) \ > __string(host, dev_name(cxlmd->dev.parent)) \ > __field(int, log) \ > - __field_struct(uuid_t, hdr_uuid) \ > __field(u64, serial) \ > __field(u32, hdr_flags) \ > __field(u16, hdr_handle) \ > @@ -203,7 +202,6 @@ TRACE_EVENT(cxl_overflow, > __assign_str(host, dev_name((cxlmd)->dev.parent)); \ > __entry->log = (l); \ > __entry->serial = (cxlmd)->cxlds->serial; \ > - memcpy(&__entry->hdr_uuid, &(hdr).id, sizeof(uuid_t)); \ > __entry->hdr_length = (hdr).length; \ > __entry->hdr_flags = get_unaligned_le24((hdr).flags); \ > __entry->hdr_handle = le16_to_cpu((hdr).handle); \ > @@ -212,12 +210,12 @@ TRACE_EVENT(cxl_overflow, > __entry->hdr_maint_op_class = (hdr).maint_op_class > > #define CXL_EVT_TP_printk(fmt, ...) \ > - TP_printk("memdev=%s host=%s serial=%lld log=%s : time=%llu uuid=%pUb " \ > + TP_printk("memdev=%s host=%s serial=%lld log=%s : time=%llu " \ > "len=%d flags='%s' handle=%x related_handle=%x " \ > "maint_op_class=%u : " fmt, \ > __get_str(memdev), __get_str(host), __entry->serial, \ > cxl_event_log_type_str(__entry->log), \ > - __entry->hdr_timestamp, &__entry->hdr_uuid, __entry->hdr_length,\ > + __entry->hdr_timestamp, __entry->hdr_length, \ > show_hdr_flags(__entry->hdr_flags), __entry->hdr_handle, \ > __entry->hdr_related_handle, __entry->hdr_maint_op_class, \ > ##__VA_ARGS__) > @@ -231,15 +229,17 @@ TRACE_EVENT(cxl_generic_event, > > TP_STRUCT__entry( > CXL_EVT_TP_entry > + __field_struct(uuid_t, hdr_uuid) > __array(u8, data, CXL_EVENT_RECORD_DATA_LENGTH) > ), > > TP_fast_assign( > CXL_EVT_TP_fast_assign(cxlmd, log, rec->hdr); > + memcpy(&__entry->hdr_uuid, &rec->hdr.id, sizeof(uuid_t)); > memcpy(__entry->data, &rec->data, CXL_EVENT_RECORD_DATA_LENGTH); > ), > > - CXL_EVT_TP_printk("%s", > + CXL_EVT_TP_printk("uuid=%pUb %s", &__entry->hdr_uuid, > __print_hex(__entry->data, CXL_EVENT_RECORD_DATA_LENGTH)) > ); > >
>-----Original Message----- >From: Jonathan Cameron <jonathan.cameron@huawei.com> >Sent: 03 November 2023 14:28 >To: Ira Weiny <ira.weiny@intel.com> >Cc: Dan Williams <dan.j.williams@intel.com>; Smita Koralahalli ><Smita.KoralahalliChannabasappa@amd.com>; Yazen Ghannam ><yazen.ghannam@amd.com>; Davidlohr Bueso <dave@stgolabs.net>; Dave >Jiang <dave.jiang@intel.com>; Alison Schofield <alison.schofield@intel.com>; >Vishal Verma <vishal.l.verma@intel.com>; Ard Biesheuvel <ardb@kernel.org>; >linux-efi@vger.kernel.org; linux-kernel@vger.kernel.org; linux- >cxl@vger.kernel.org; Shiju Jose <shiju.jose@huawei.com> >Subject: Re: [PATCH RFC v3 1/6] cxl/trace: Remove uuid from event trace known >events > >On Wed, 01 Nov 2023 14:11:18 -0700 >Ira Weiny <ira.weiny@intel.com> wrote: > >> The uuid printed in the well known events is redundant. The uuid >> defines what the event was. >> >> Remove the uuid from the known events and only report it in the >> generic event as it remains informative there. >> >> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net> >> Reviewed-by: Dan Williams <dan.j.williams@intel.com> >> Signed-off-by: Ira Weiny <ira.weiny@intel.com> > >Removing the print is fine, but look like this also removes the actual trace point >field. That's userspace ABI. Expanding it is fine, but taking fields away is more >problematic. > >Are we sure we don't break anyone? Shiju, will rasdaemon be fine with this >change? The field hdr_uuid is removed from the common CXL_EVT_TP_entry shared by the trace events cxl_generic_event, cxl_general_media, cxl_dram and cxl_memory_module . rasdaemon will break because of this while processing these trace events and also affects the corresponding error records in the SQLite data base. Rasdaemon needs update to avoid this. > >Thanks, > >Jonathan > Thanks, Shiju > > >> --- >> drivers/cxl/core/trace.h | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h index >> a0b5819bc70b..79ed03637604 100644 >> --- a/drivers/cxl/core/trace.h >> +++ b/drivers/cxl/core/trace.h >> @@ -189,7 +189,6 @@ TRACE_EVENT(cxl_overflow, >> __string(memdev, dev_name(&cxlmd->dev)) \ >> __string(host, dev_name(cxlmd->dev.parent)) \ >> __field(int, log) \ >> - __field_struct(uuid_t, hdr_uuid) \ >> __field(u64, serial) \ >> __field(u32, hdr_flags) \ >> __field(u16, hdr_handle) \ >> @@ -203,7 +202,6 @@ TRACE_EVENT(cxl_overflow, >> __assign_str(host, dev_name((cxlmd)->dev.parent)); > \ >> __entry->log = (l); > \ >> __entry->serial = (cxlmd)->cxlds->serial; \ >> - memcpy(&__entry->hdr_uuid, &(hdr).id, sizeof(uuid_t)); > \ >> __entry->hdr_length = (hdr).length; > \ >> __entry->hdr_flags = get_unaligned_le24((hdr).flags); > \ >> __entry->hdr_handle = le16_to_cpu((hdr).handle); > \ >> @@ -212,12 +210,12 @@ TRACE_EVENT(cxl_overflow, >> __entry->hdr_maint_op_class = (hdr).maint_op_class >> >> #define CXL_EVT_TP_printk(fmt, ...) \ >> - TP_printk("memdev=%s host=%s serial=%lld log=%s : time=%llu >uuid=%pUb " \ >> + TP_printk("memdev=%s host=%s serial=%lld log=%s : time=%llu " > \ >> "len=%d flags='%s' handle=%x related_handle=%x " > \ >> "maint_op_class=%u : " fmt, > \ >> __get_str(memdev), __get_str(host), __entry->serial, > \ >> cxl_event_log_type_str(__entry->log), > \ >> - __entry->hdr_timestamp, &__entry->hdr_uuid, __entry- >>hdr_length,\ >> + __entry->hdr_timestamp, __entry->hdr_length, > \ >> show_hdr_flags(__entry->hdr_flags), __entry->hdr_handle, > \ >> __entry->hdr_related_handle, __entry->hdr_maint_op_class, > \ >> ##__VA_ARGS__) >> @@ -231,15 +229,17 @@ TRACE_EVENT(cxl_generic_event, >> >> TP_STRUCT__entry( >> CXL_EVT_TP_entry >> + __field_struct(uuid_t, hdr_uuid) >> __array(u8, data, CXL_EVENT_RECORD_DATA_LENGTH) >> ), >> >> TP_fast_assign( >> CXL_EVT_TP_fast_assign(cxlmd, log, rec->hdr); >> + memcpy(&__entry->hdr_uuid, &rec->hdr.id, sizeof(uuid_t)); >> memcpy(__entry->data, &rec->data, >CXL_EVENT_RECORD_DATA_LENGTH); >> ), >> >> - CXL_EVT_TP_printk("%s", >> + CXL_EVT_TP_printk("uuid=%pUb %s", &__entry->hdr_uuid, >> __print_hex(__entry->data, >CXL_EVENT_RECORD_DATA_LENGTH)) ); >> >>
Shiju Jose wrote: > > > >-----Original Message----- > >From: Jonathan Cameron <jonathan.cameron@huawei.com> > >Sent: 03 November 2023 14:28 > >To: Ira Weiny <ira.weiny@intel.com> > >Cc: Dan Williams <dan.j.williams@intel.com>; Smita Koralahalli > ><Smita.KoralahalliChannabasappa@amd.com>; Yazen Ghannam > ><yazen.ghannam@amd.com>; Davidlohr Bueso <dave@stgolabs.net>; Dave > >Jiang <dave.jiang@intel.com>; Alison Schofield <alison.schofield@intel.com>; > >Vishal Verma <vishal.l.verma@intel.com>; Ard Biesheuvel <ardb@kernel.org>; > >linux-efi@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > >cxl@vger.kernel.org; Shiju Jose <shiju.jose@huawei.com> > >Subject: Re: [PATCH RFC v3 1/6] cxl/trace: Remove uuid from event trace known > >events > > > >On Wed, 01 Nov 2023 14:11:18 -0700 > >Ira Weiny <ira.weiny@intel.com> wrote: > > > >> The uuid printed in the well known events is redundant. The uuid > >> defines what the event was. > >> > >> Remove the uuid from the known events and only report it in the > >> generic event as it remains informative there. > >> > >> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net> > >> Reviewed-by: Dan Williams <dan.j.williams@intel.com> > >> Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > > >Removing the print is fine, but look like this also removes the actual trace point > >field. That's userspace ABI. Expanding it is fine, but taking fields away is more > >problematic. > > > >Are we sure we don't break anyone? Shiju, will rasdaemon be fine with this > >change? > > The field hdr_uuid is removed from the common CXL_EVT_TP_entry shared by the > trace events cxl_generic_event, cxl_general_media, cxl_dram and cxl_memory_module . > rasdaemon will break because of this while processing these trace events > and also affects the corresponding error records in the SQLite data base. > Rasdaemon needs update to avoid this. > Ok we can leave the uuid field in easy enough. But does rasdaemon use the value of the field for anything? In other words does CPER record processing need to generate a proper UUID value? Ira
Hi Ira, >-----Original Message----- >From: Ira Weiny <ira.weiny@intel.com> >Sent: 06 November 2023 22:06 >To: Shiju Jose <shiju.jose@huawei.com>; Jonathan Cameron ><jonathan.cameron@huawei.com>; Ira Weiny <ira.weiny@intel.com> >Cc: Dan Williams <dan.j.williams@intel.com>; Smita Koralahalli ><Smita.KoralahalliChannabasappa@amd.com>; Yazen Ghannam ><yazen.ghannam@amd.com>; Davidlohr Bueso <dave@stgolabs.net>; Dave >Jiang <dave.jiang@intel.com>; Alison Schofield <alison.schofield@intel.com>; >Vishal Verma <vishal.l.verma@intel.com>; Ard Biesheuvel <ardb@kernel.org>; >linux-efi@vger.kernel.org; linux-kernel@vger.kernel.org; linux- >cxl@vger.kernel.org >Subject: RE: [PATCH RFC v3 1/6] cxl/trace: Remove uuid from event trace known >events > >Shiju Jose wrote: >> >> >> >-----Original Message----- >> >From: Jonathan Cameron <jonathan.cameron@huawei.com> >> >Sent: 03 November 2023 14:28 >> >To: Ira Weiny <ira.weiny@intel.com> >> >Cc: Dan Williams <dan.j.williams@intel.com>; Smita Koralahalli >> ><Smita.KoralahalliChannabasappa@amd.com>; Yazen Ghannam >> ><yazen.ghannam@amd.com>; Davidlohr Bueso <dave@stgolabs.net>; Dave >> >Jiang <dave.jiang@intel.com>; Alison Schofield >> ><alison.schofield@intel.com>; Vishal Verma >> ><vishal.l.verma@intel.com>; Ard Biesheuvel <ardb@kernel.org>; >> >linux-efi@vger.kernel.org; linux-kernel@vger.kernel.org; linux- >> >cxl@vger.kernel.org; Shiju Jose <shiju.jose@huawei.com> >> >Subject: Re: [PATCH RFC v3 1/6] cxl/trace: Remove uuid from event >> >trace known events >> > >> >On Wed, 01 Nov 2023 14:11:18 -0700 >> >Ira Weiny <ira.weiny@intel.com> wrote: >> > >> >> The uuid printed in the well known events is redundant. The uuid >> >> defines what the event was. >> >> >> >> Remove the uuid from the known events and only report it in the >> >> generic event as it remains informative there. >> >> >> >> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net> >> >> Reviewed-by: Dan Williams <dan.j.williams@intel.com> >> >> Signed-off-by: Ira Weiny <ira.weiny@intel.com> >> > >> >Removing the print is fine, but look like this also removes the >> >actual trace point field. That's userspace ABI. Expanding it is >> >fine, but taking fields away is more problematic. >> > >> >Are we sure we don't break anyone? Shiju, will rasdaemon be fine >> >with this change? >> >> The field hdr_uuid is removed from the common CXL_EVT_TP_entry shared >> by the trace events cxl_generic_event, cxl_general_media, cxl_dram and >cxl_memory_module . >> rasdaemon will break because of this while processing these trace >> events and also affects the corresponding error records in the SQLite data >base. >> Rasdaemon needs update to avoid this. >> > >Ok we can leave the uuid field in easy enough. > >But does rasdaemon use the value of the field for anything? In other words does >CPER record processing need to generate a proper UUID value? No. Presently used for logging purpose only in the rasdaemon. > >Ira Thanks, Shiju
diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h index a0b5819bc70b..79ed03637604 100644 --- a/drivers/cxl/core/trace.h +++ b/drivers/cxl/core/trace.h @@ -189,7 +189,6 @@ TRACE_EVENT(cxl_overflow, __string(memdev, dev_name(&cxlmd->dev)) \ __string(host, dev_name(cxlmd->dev.parent)) \ __field(int, log) \ - __field_struct(uuid_t, hdr_uuid) \ __field(u64, serial) \ __field(u32, hdr_flags) \ __field(u16, hdr_handle) \ @@ -203,7 +202,6 @@ TRACE_EVENT(cxl_overflow, __assign_str(host, dev_name((cxlmd)->dev.parent)); \ __entry->log = (l); \ __entry->serial = (cxlmd)->cxlds->serial; \ - memcpy(&__entry->hdr_uuid, &(hdr).id, sizeof(uuid_t)); \ __entry->hdr_length = (hdr).length; \ __entry->hdr_flags = get_unaligned_le24((hdr).flags); \ __entry->hdr_handle = le16_to_cpu((hdr).handle); \ @@ -212,12 +210,12 @@ TRACE_EVENT(cxl_overflow, __entry->hdr_maint_op_class = (hdr).maint_op_class #define CXL_EVT_TP_printk(fmt, ...) \ - TP_printk("memdev=%s host=%s serial=%lld log=%s : time=%llu uuid=%pUb " \ + TP_printk("memdev=%s host=%s serial=%lld log=%s : time=%llu " \ "len=%d flags='%s' handle=%x related_handle=%x " \ "maint_op_class=%u : " fmt, \ __get_str(memdev), __get_str(host), __entry->serial, \ cxl_event_log_type_str(__entry->log), \ - __entry->hdr_timestamp, &__entry->hdr_uuid, __entry->hdr_length,\ + __entry->hdr_timestamp, __entry->hdr_length, \ show_hdr_flags(__entry->hdr_flags), __entry->hdr_handle, \ __entry->hdr_related_handle, __entry->hdr_maint_op_class, \ ##__VA_ARGS__) @@ -231,15 +229,17 @@ TRACE_EVENT(cxl_generic_event, TP_STRUCT__entry( CXL_EVT_TP_entry + __field_struct(uuid_t, hdr_uuid) __array(u8, data, CXL_EVENT_RECORD_DATA_LENGTH) ), TP_fast_assign( CXL_EVT_TP_fast_assign(cxlmd, log, rec->hdr); + memcpy(&__entry->hdr_uuid, &rec->hdr.id, sizeof(uuid_t)); memcpy(__entry->data, &rec->data, CXL_EVENT_RECORD_DATA_LENGTH); ), - CXL_EVT_TP_printk("%s", + CXL_EVT_TP_printk("uuid=%pUb %s", &__entry->hdr_uuid, __print_hex(__entry->data, CXL_EVENT_RECORD_DATA_LENGTH)) );