RE: [PATCH 1/6] cxl/trace: Pass uuid explicitly to event traces

[Ira Weiny <ira.weiny@intel.com>]

Dan Williams wrote:
> Ira Weiny wrote:
> > CPER CXL events do not have a UUID associated with them.  It is
> > desirable to share event structures between the CPER CXL event and the
> > CXL event log events.
> 
> This parses strange to me, maybe it is trying to capture too many
> changes in too few sentences? Something like this instead?

Yea probably just trying to squeeze too much in.

> 
> "CXL CPER events are identified by the CPER Section Type GUID. The GUID
> correlates with the CXL UUID for the event record. It turns out that a
> CXL CPER record is a strict subset of the CXL event record, only the
> UUID header field is chopped.
> 
> In order to unify handling between native and CPER flavors of CXL
> events, prepare the code for the UUID to be passed in rather than
> inferred from the record itself.
> 
> Later patches update the passed in record to only refer to the common
> data between the formats."

That looks much more detailed.  I'll add it in.


[snip]

> > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> > index a0b5819bc70b..2aef185f4cd0 100644
> > --- a/drivers/cxl/core/trace.h
> > +++ b/drivers/cxl/core/trace.h
> > @@ -189,7 +189,7 @@ 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_struct(uuid_t, uuid)				\
> 
> Hmm, is it too late to make this rename? I.e. would a script that is
> looking for "hdr_uuid" in one kernel be broken by the rename of the
> field to "uuid" in the next?

True, probably best to leave the field name alone.  I'll change it back.

> 
> >  	__field(u64, serial)					\
> >  	__field(u32, hdr_flags)					\
> >  	__field(u16, hdr_handle)				\
> > @@ -198,12 +198,12 @@ TRACE_EVENT(cxl_overflow,
> >  	__field(u8, hdr_length)					\
> >  	__field(u8, hdr_maint_op_class)
> >  
> > -#define CXL_EVT_TP_fast_assign(cxlmd, l, hdr)					\
> > +#define CXL_EVT_TP_fast_assign(cxlmd, l, uuid, 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, &(hdr).id, sizeof(uuid_t));			\
> > +	memcpy(&__entry->uuid, (uuid), sizeof(uuid_t));				\
> 
> Per above this would be:
> 
> 	memcpy(&__entry->hdr_uuid, (uuid), sizeof(uuid_t));				\

yep.

> 
> >  	__entry->hdr_length = (hdr).length;					\
> >  	__entry->hdr_flags = get_unaligned_le24((hdr).flags);			\
> >  	__entry->hdr_handle = le16_to_cpu((hdr).handle);			\
> > @@ -217,7 +217,7 @@ TRACE_EVENT(cxl_overflow,
> >  		"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->uuid, __entry->hdr_length,	\
> 
> ...and this change could be dropped.

Yep.

All done.

> 
> Other than that, this looks good to me.

Thanks,
Ira