Re: [RFC V2 PATCH 05/11] cxl/mem: Trace General Media Event Record

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

On Tue, Oct 11, 2022 at 01:57:02PM +0100, Jonathan Cameron wrote:
> On Mon, 10 Oct 2022 15:41:25 -0700
> ira.weiny@intel.com wrote:
> 
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > CXL rev 3.0 section 8.2.9.2.1.1 defines the General Media Event Record.
> > 
> > Determine if the event read is a general media record and if so trace
> > the record.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> I'll review the rest of these with the assumption that the question
> of reserved bytes in tracepoints will get resolved before these go in.

Yea I'm removing them.  I think I messed up somehow because I thought I had
removed the reserved fields from the records.  But perhaps that was only in my
dreams...  :-/  Sorry.  :-)

> 
> One minor comment on a comment inline.  Other than those lgtm
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 

Thanks!

[snip]

> >  
> > +/*
> > + * General Media Event Record
> > + * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43
> > + */
> > +static const uuid_t gen_media_event_uuid =
> > +	UUID_INIT(0xfbcd0a77, 0xc260, 0x417f,
> > +		  0x85, 0xa9, 0x08, 0x8b, 0x16, 0x21, 0xeb, 0xa6);
> > +
> > +static void cxl_trace_event_record(const char *dev_name,
> > +				   enum cxl_event_log_type type,
> > +				   struct cxl_get_event_payload *payload)
> > +{
> > +	uuid_t *id = &payload->record.hdr.id;
> > +
> > +	if (uuid_equal(id, &gen_media_event_uuid)) {
> > +		struct cxl_event_gen_media *rec =
> > +				(struct cxl_event_gen_media *)&payload->record;
> > +
> > +		trace_general_media(dev_name, type, rec);
> > +		return;
> > +	}
> > +
> > +	/* For unknown record types print just the header */
> > +	trace_generic_event(dev_name, type, &payload->record);
> 
> Looks like it prints the whole thing now...

An unknown record is dumped yes.  I'm ok with skipping this but it was
discussed early on that any unknown record would be passed through.

> 
> 
> > +}
> > +
> 
> 
> > diff --git a/include/trace/events/cxl.h b/include/trace/events/cxl.h
> > index 318ba5fe046e..82a8d3b750a2 100644
> > --- a/include/trace/events/cxl.h
> > +++ b/include/trace/events/cxl.h
> > @@ -122,6 +122,114 @@ TRACE_EVENT(generic_event,
> 
> 
> > +#define CXL_GMER_VALID_CHANNEL				BIT(0)
> > +#define CXL_GMER_VALID_RANK				BIT(1)
> > +#define CXL_GMER_VALID_DEVICE				BIT(2)
> > +#define CXL_GMER_VALID_COMPONENT			BIT(3)
> > +#define show_valid_flags(flags)	__print_flags(flags, "|",		   \
> > +	{ CXL_GMER_VALID_CHANNEL,			"CHANNEL"	}, \
> > +	{ CXL_GMER_VALID_RANK,				"RANK"		}, \
> > +	{ CXL_GMER_VALID_DEVICE,			"DEVICE"	}, \
> > +	{ CXL_GMER_VALID_COMPONENT,			"COMPONENT"	}  \
> > +)
> 
> I'd still rather we only had the TP_printk only print those parts that are valid
> rather than the mess of having to go check the validity bit before deciding whether
> to take notice of the field.  But meh, not that important given thats
> not the main intended way to consume this data.
> 

Ok I spent some time really thinking about this and below is an attempt at
that.

However, I must be missing something in what you are proposing because I don't
like having extra space in the trace buffer to print into like this and I
can't figure out where else to put a print buffer.

Also note that this will have no impact on most of the user space tools which
use libtracefs as they will see all the fields and will need to do a similar
decode.

Do you really think this is worth the effort?

Ira


commit 54c4ee67bcac6a38cbc9b0ea2ef952e197356dee
Author: Ira Weiny <ira.weiny@intel.com>
Date:   Fri Oct 14 16:15:27 2022 -0700

    squash: Attempt to print only valid fields per Jonathan suggestion

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 2add473fc168..9e15028af79c 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -720,6 +720,28 @@ static const uuid_t gen_media_event_uuid =
        UUID_INIT(0xfbcd0a77, 0xc260, 0x417f,
                  0x85, 0xa9, 0x08, 0x8b, 0x16, 0x21, 0xeb, 0xa6);
 
+const char *cxl_gem_print_valid(u8 *buf, int nr, u16 valid_flags, u8 channel,
+                               u8 rank, u32 device, u8 *comp_id)
+{
+       char *str = buf;
+       int rc = 0;
+
+       if (valid_flags & CXL_GMER_VALID_CHANNEL)
+               rc = snprintf(str, nr, "channel=%u ", channel);
+
+       if (valid_flags & CXL_GMER_VALID_RANK)
+               rc = snprintf(str + rc, nr - rc, "rank=%u ", rank);
+
+       if (valid_flags & CXL_GMER_VALID_DEVICE)
+               rc = snprintf(str + rc, nr - rc, "device=%u ", device);
+
+       if (valid_flags & CXL_GMER_VALID_COMPONENT)
+               rc = snprintf(str + rc, nr - rc, "comp_id=%*ph ",
+                             CXL_EVENT_GEN_MED_COMP_ID_SIZE, comp_id);
+
+       return str;
+}
+
 static void cxl_trace_event_record(const char *dev_name,
                                   enum cxl_event_log_type type,
                                   struct cxl_get_event_payload *payload)
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 8fbd20d8a0b2..3d3bfef69d32 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -429,6 +429,13 @@ struct cxl_event_gen_media {
        u8 reserved[0x2e];
 } __packed;
 
+#define CXL_GMER_VALID_CHANNEL                         BIT(0)
+#define CXL_GMER_VALID_RANK                            BIT(1)
+#define CXL_GMER_VALID_DEVICE                          BIT(2)
+#define CXL_GMER_VALID_COMPONENT                       BIT(3)
+const char *cxl_gem_print_valid(u8 *buf, int nr, u16 valid_flags, u8 channel,
+                               u8 rank, u32 device, u8 *comp_id);
+
 struct cxl_mbox_get_partition_info {
        __le64 active_volatile_cap;
        __le64 active_persistent_cap;
diff --git a/include/trace/events/cxl.h b/include/trace/events/cxl.h
index e3e11c9055ba..27bb790cb685 100644
--- a/include/trace/events/cxl.h
+++ b/include/trace/events/cxl.h
@@ -161,16 +161,7 @@ TRACE_EVENT(generic_event,
        { CXL_GMER_TRANS_INTERNAL_MEDIA_MANAGEMENT,     "Internal Media Management" }   \
 )
 
-#define CXL_GMER_VALID_CHANNEL                         BIT(0)
-#define CXL_GMER_VALID_RANK                            BIT(1)
-#define CXL_GMER_VALID_DEVICE                          BIT(2)
-#define CXL_GMER_VALID_COMPONENT                       BIT(3)
-#define show_valid_flags(flags)        __print_flags(flags, "|",                  \
-       { CXL_GMER_VALID_CHANNEL,                       "CHANNEL"       }, \
-       { CXL_GMER_VALID_RANK,                          "RANK"          }, \
-       { CXL_GMER_VALID_DEVICE,                        "DEVICE"        }, \
-       { CXL_GMER_VALID_COMPONENT,                     "COMPONENT"     }  \
-)
+#define CXL_VALID_PRINT_STR_LEN 512
 
 TRACE_EVENT(general_media,
 
@@ -191,6 +182,7 @@ TRACE_EVENT(general_media,
                __array(u8, comp_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE)
                __field(u16, validity_flags)
                __field(u8, rank) /* Out of order to pack trace record */
+               __array(u8, str, CXL_VALID_PRINT_STR_LEN)
        ),
 
        TP_fast_assign(
@@ -209,17 +201,17 @@ TRACE_EVENT(general_media,
                __entry->validity_flags = get_unaligned_le16(&rec->validity_flags);
        ),
 
-       CXL_EVT_TP_printk("phys_addr=%llx volatile=%s desc='%s' type='%s' " \
-               "trans_type='%s' channel=%u rank=%u device=%x comp_id=%s " \
-               "valid_flags='%s'",
+       CXL_EVT_TP_printk("phys_addr=%llx volatile=%s desc='%s' type='%s' "     \
+               "trans_type='%s' %s",
                __entry->phys_addr & CXL_GMER_PHYS_ADDR_MASK,
                (__entry->phys_addr & CXL_GMER_PHYS_ADDR_VOLATILE) ? "TRUE" : "FALSE",
                show_event_desc_flags(__entry->descriptor),
                show_mem_event_type(__entry->type),
                show_trans_type(__entry->transaction_type),
-               __entry->channel, __entry->rank, __entry->device,
-               __print_hex(__entry->comp_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE),
-               show_valid_flags(__entry->validity_flags)
+               cxl_gem_print_valid(__entry->str, CXL_VALID_PRINT_STR_LEN,
+                                   __entry->validity_flags, __entry->channel,
+                                   __entry->rank, __entry->device,
+                                   __entry->comp_id)
        )
 );