Re: [RFC V2 PATCH 02/11] cxl/mem: Implement Get Event Records command

[Ira Weiny]

On Sat, Oct 15, 2022 at 07:28:40AM -0400, Steven Rostedt wrote:
> On Mon, 10 Oct 2022 15:41:22 -0700
> ira.weiny@xxxxxxxxx wrote:
> 
> > new file mode 100644
> > index 000000000000..318ba5fe046e
> > --- /dev/null
> > +++ b/include/trace/events/cxl.h
> > @@ -0,0 +1,130 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#undef TRACE_SYSTEM
> > +#define TRACE_SYSTEM cxl
> > +
> > +#if !defined(_CXL_TRACE_EVENTS_H) ||  defined(TRACE_HEADER_MULTI_READ)
> > +#define _CXL_TRACE_EVENTS_H
> > +
> > +#include <asm-generic/unaligned.h>
> > +#include <linux/tracepoint.h>
> > +
> > +TRACE_EVENT(overflow,
> 
> Please do not use generic names for grouped events. As most tooling can
> use the name and not associate it with the group, it makes it ambiguous
> for what event it wants to enable.
> 
> That is, call it "cxl_overflow" and not "overflow".

I did not realize that.[1]

[1] https://lore.kernel.org/linux-cxl/67c4e0da-87e3-44a1-1902-220dbc312700@xxxxxxxxx/

> 
> > +
> > +	TP_PROTO(const char *dev_name, enum cxl_event_log_type log,
> > +		 struct cxl_get_event_payload *payload),
> > +
> > +	TP_ARGS(dev_name, log, payload),
> > +
> > +	TP_STRUCT__entry(
> > +		__string(dev_name, dev_name)
> > +		__field(int, log)
> > +		__field(u64, first)
> > +		__field(u64, last)
> > +		__field(u16, count)
> > +	),
> > +
> > +	TP_fast_assign(
> > +		__assign_str(dev_name, dev_name);
> > +		__entry->log = log;
> > +		__entry->count = le16_to_cpu(payload->overflow_err_count);
> > +		__entry->first = le64_to_cpu(payload->first_overflow_timestamp);
> > +		__entry->last = le64_to_cpu(payload->last_overflow_timestamp);
> > +	),
> > +
> > +	TP_printk("%s: EVENT LOG OVERFLOW log=%s : %u records from %llu to %llu",
> > +		__get_str(dev_name), cxl_event_log_type_str(__entry->log),
> > +		__entry->count, __entry->first, __entry->last)
> > +
> > +);
> > +
> 
> > +TRACE_EVENT(generic_event,
> 
> Same here. It's a "cxl_generic_event" not a "generic_event" that will
> clash with any other subsystem that would (but shouldn't) use the same
> name.

Sure I'll change it back.

Thanks for setting us straight,
Ira

> 
> -- Steve
> 
> > +
> > +	TP_PROTO(const char *dev_name, enum cxl_event_log_type log,
> > +		 struct cxl_event_record_raw *rec),
> > +
> > +	TP_ARGS(dev_name, log, rec),
> > +
> > +	TP_STRUCT__entry(
> > +		CXL_EVT_TP_entry
> > +		__array(u8, data, CXL_EVENT_RECORD_DATA_LENGTH)
> > +	),
> > +
> > +	TP_fast_assign(
> > +		CXL_EVT_TP_fast_assign(dev_name, log, rec->hdr);
> > +		memcpy(__entry->data, &rec->data, CXL_EVENT_RECORD_DATA_LENGTH);
> > +	),
> > +
> > +	CXL_EVT_TP_printk("%s",
> > +		__print_hex(__entry->data, CXL_EVENT_RECORD_DATA_LENGTH))
> > +);
> > +
> > +#endif /* _CXL_TRACE_EVENTS_H */
> >