| Message ID | 169480882977.2690926.2796694282356938267.stgit@djiang5-mobl3 |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] cxl: Move command enumeration from dev_dbg() to traceevent | expand |
Dave Jiang wrote: > Given that event logs outputs are all emitted to traceevent, move the > enumeration of log types to traceevent as well in order to keep all the > outputs at the same location. > > Suggested-by: Alison Schofield <alison.schofield@intel.com> > Signed-off-by: Dave Jiang <dave.jiang@intel.com> My gut reaction was to nak this patch because I'm not sure how to enable trace events prior to module load. Then I realized that this is in the cxl_core and is only triggered by driver loads. So I've not tested this patch but I think the following sequence will work for these 2 patches. $ modprobe cxl_core $ echo 1 > /sys/kernel/tracing/events/cxl/cxl_log_type/enable $ modprobe cxl_pci Is that how you tested this? It seems like a pain. But perhaps that pain is fine because these debug messages are not as useful as others? Especially for all the unsupported messages mentioned in patch 2? Ira
On 9/15/23 15:11, Ira Weiny wrote: > Dave Jiang wrote: >> Given that event logs outputs are all emitted to traceevent, move the >> enumeration of log types to traceevent as well in order to keep all the >> outputs at the same location. >> >> Suggested-by: Alison Schofield <alison.schofield@intel.com> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > My gut reaction was to nak this patch because I'm not sure how to enable > trace events prior to module load. Then I realized that this is in the > cxl_core and is only triggered by driver loads. So I've not tested this > patch but I think the following sequence will work for these 2 patches. > > $ modprobe cxl_core > $ echo 1 > /sys/kernel/tracing/events/cxl/cxl_log_type/enable > $ modprobe cxl_pci I don't see any other way to do this. I saw this [1] but it doesn't appeared to be upstream. [1]: https://lwn.net/Articles/432186/ > > Is that how you tested this? It seems like a pain. But perhaps that pain > is fine because these debug messages are not as useful as others? > Especially for all the unsupported messages mentioned in patch 2? Yes. Alison mentioned seeing maybe 500+ of those. > > Ira
Dave Jiang wrote: > > > On 9/15/23 15:11, Ira Weiny wrote: > > Dave Jiang wrote: > > [snip] > > My gut reaction was to nak this patch because I'm not sure how to enable > > trace events prior to module load. Then I realized that this is in the > > cxl_core and is only triggered by driver loads. So I've not tested this > > patch but I think the following sequence will work for these 2 patches. > > > > $ modprobe cxl_core > > $ echo 1 > /sys/kernel/tracing/events/cxl/cxl_log_type/enable > > $ modprobe cxl_pci > > I don't see any other way to do this. I saw this [1] but it doesn't appeared to be upstream. > > [1]: https://lwn.net/Articles/432186/ > > > > > Is that how you tested this? It seems like a pain. But perhaps that pain > > is fine because these debug messages are not as useful as others? > > Especially for all the unsupported messages mentioned in patch 2? > > Yes. Alison mentioned seeing maybe 500+ of those. Ok it seems reasonable to me. But perhaps we can document the above method somewhere? Not sure if the commit message is the best place but it is better than nothing. Thanks, Ira
On 9/18/23 10:49, Ira Weiny wrote: > Dave Jiang wrote: >> >> >> On 9/15/23 15:11, Ira Weiny wrote: >>> Dave Jiang wrote: >>> > > [snip] > >>> My gut reaction was to nak this patch because I'm not sure how to enable >>> trace events prior to module load. Then I realized that this is in the >>> cxl_core and is only triggered by driver loads. So I've not tested this >>> patch but I think the following sequence will work for these 2 patches. >>> >>> $ modprobe cxl_core >>> $ echo 1 > /sys/kernel/tracing/events/cxl/cxl_log_type/enable >>> $ modprobe cxl_pci >> >> I don't see any other way to do this. I saw this [1] but it doesn't appeared to be upstream. >> >> [1]: https://lwn.net/Articles/432186/ >> >>> >>> Is that how you tested this? It seems like a pain. But perhaps that pain >>> is fine because these debug messages are not as useful as others? >>> Especially for all the unsupported messages mentioned in patch 2? >> >> Yes. Alison mentioned seeing maybe 500+ of those. > > Ok it seems reasonable to me. But perhaps we can document the above method > somewhere? Not sure if the commit message is the best place but it is better > than nothing. I can add it to the commit log. Maybe I'll send out v2 of this independent of the other patch given there may be some differing opinions on what to do about that. > > Thanks, > Ira
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index ca60bb8114f2..ab6b6c4d7a48 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -787,7 +787,6 @@ static const uuid_t log_uuid[] = { int cxl_enumerate_cmds(struct cxl_memdev_state *mds) { struct cxl_mbox_get_supported_logs *gsl; - struct device *dev = mds->cxlds.dev; struct cxl_mem_command *cmd; int i, rc; @@ -801,7 +800,7 @@ int cxl_enumerate_cmds(struct cxl_memdev_state *mds) uuid_t uuid = gsl->entry[i].uuid; u8 *log; - dev_dbg(dev, "Found LOG type %pU of size %d", &uuid, size); + trace_cxl_log_type(mds->cxlds.cxlmd, &uuid, size); if (!uuid_equal(&uuid, &log_uuid[CEL_UUID])) continue; diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h index a0b5819bc70b..817c5377eca2 100644 --- a/drivers/cxl/core/trace.h +++ b/drivers/cxl/core/trace.h @@ -703,6 +703,37 @@ TRACE_EVENT(cxl_poison, ) ); +TRACE_EVENT(cxl_log_type, + + TP_PROTO(const struct cxl_memdev *cxlmd, uuid_t *uuid, int size), + + TP_ARGS(cxlmd, uuid, size), + + TP_STRUCT__entry( + __string(memdev, dev_name(&cxlmd->dev)) + __string(host, dev_name(cxlmd->dev.parent)) + __array(char, uuid, 16) + __field(u64, serial) + __field(int, size) + ), + + TP_fast_assign( + __assign_str(memdev, dev_name(&cxlmd->dev)); + __assign_str(host, dev_name(cxlmd->dev.parent)); + __entry->serial = cxlmd->cxlds->serial; + memcpy(__entry->uuid, uuid, 16); + __entry->size = size; + ), + + TP_printk("memdev=%s host=%s serial=%lld log_type=%pU size=%d", + __get_str(memdev), + __get_str(host), + __entry->serial, + __entry->uuid, + __entry->size + ) +); + #endif /* _CXL_EVENTS_H */ #define TRACE_INCLUDE_FILE trace
Given that event logs outputs are all emitted to traceevent, move the enumeration of log types to traceevent as well in order to keep all the outputs at the same location. Suggested-by: Alison Schofield <alison.schofield@intel.com> Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- drivers/cxl/core/mbox.c | 3 +-- drivers/cxl/core/trace.h | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-)