Messages in this thread | | | Date | Mon, 17 Oct 2022 17:37:17 +0100 | From | Jonathan Cameron <> | Subject | Re: [RFC V2 PATCH 05/11] cxl/mem: Trace General Media Event Record |
| |
> > > > > 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.
Putting the space in the trace buffer definitely doesn't makes sense. Ah. Looking back the CCIX code I assumed it was serialized so could use a static buffer.
Looking at other similar cases though and we have a lot of use of trace_seq_printf() e.g. libata_trace_parse_status() though note there is some magic macro stuff in include/trace/events/libata.h to tie that together. https://elixir.bootlin.com/linux/latest/source/drivers/ata/libata-trace.c#L14
That seems to get you access to the actual buffer we are printing into in similar cases.
> > 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?
With the allocation problem, definitely not. With that solved, it's not a huge amount of extra code. Also rather nicely 'documents' meaning of the valid bits.
I'm a kernel hacker. I like to not need much beyond echo and cat :)
Jonathan
> > 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) > ) > ); > >
| |