RE: [PATCH RFC v3 1/6] cxl/trace: Remove uuid from event trace known events

[Shiju Jose <shiju.jose@huawei.com>]

>-----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))  );
>>
>>