Messages in this thread Patch in this message | | | Date | Thu, 12 Jan 2023 08:37:38 -0800 | From | Alison Schofield <> | Subject | Re: [PATCH v4 2/5] cxl/trace: Add TRACE support for CXL media-error records |
| |
On Thu, Jan 12, 2023 at 11:16:52AM +0000, Jonathan Cameron wrote: > On Thu, 15 Dec 2022 13:17:44 -0800 > alison.schofield@intel.com wrote: > > > From: Alison Schofield <alison.schofield@intel.com> > > > > CXL devices may support the retrieval of a device poison list. > > Add a new trace event that the CXL subsystem may use to log > > the media-error records returned in the poison list. > > > > Log each media-error record as a trace event of type 'cxl_poison'. > > > > When the poison list is requested by region, include the region name > > and uuid in the trace event. > > > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > > Hi Alison, > > I'm wondering a bit if it makes sense to log/trace the MORE flag for each > record. That has some unusual semantics. Like the other flags it's a > characteristic of the underlying Get Poison List Output Payload, not a > particularly Poison list entry. Unlike overflow and scanning which are > reasonably a characteristic of each poison record in a given set read > in one command, MORE is not. > Imagine the following. > > Read first N records, MORE set so we have that flag in all the trace > records. > Read next M records, MORE not set as list is less than N+M records. > > Now userspace tooling has no idea about the mapping to underlying > mailbox commands. So it sees this bit set for first N which is fine > as there are more records following, but for the next M - 1 it might > reasonably expect to see MORE set (as more records are getting traced) > but it doesn't see it for any of them. > > That seems less than intuitive. > > I'm not sure what to suggest... Seems wrong to 'make up a MORE flag' for > those M-1 records. Does it make sense to expose this flag at all? In > what way is it useful? > > Jonathan
Thanks Jonathan -
The FLAGs field today simply reflects the contents of the flags field of the payload, which can be MORE, OVERFLOW, SCANNING.
Understand that 'MORE' seems useless to userspace however, I lean towards giving userspace the complete truth, and letting userspace ignore what is of no interest.
Perhaps a user wants to explore the boundaries of a devices poison handling capacity. ie...use inject and get_poison and see when the MORE flag appears. I don't know if it may appear before the inject_max is reached.
So - I say keep MORE as is.
This discussion triggers another thought...
ATM, a devices max_mer - Max Media Error Records is not exposed in sysfs, and this conversation makes me think it should be. It also, comes to mind because I am exposing the inject_poison_limit to sysfs.
Users wanting to explore poison capabilities would be interested in max_mer.
I'll add the max_mer to syfs in next rev.
Alison
> > > > --- > > drivers/cxl/core/mbox.c | 6 ++- > > drivers/cxl/core/trace.h | 83 ++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 88 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > > index dfe24a2adfdb..c345af8a4afd 100644 > > --- a/drivers/cxl/core/mbox.c > > +++ b/drivers/cxl/core/mbox.c > > @@ -10,6 +10,7 @@ > > #include <cxl.h> > > > > #include "core.h" > > +#include "trace.h" > > > > static bool cxl_raw_allow_all; > > > > @@ -899,7 +900,10 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len, > > if (rc) > > break; > > > > - /* TODO TRACE the media error records */ > > + for (int i = 0; i < le16_to_cpu(po->count); i++) > > + trace_cxl_poison(cxlmd, to_pci_dev(cxlds->dev), > > + cxlr, &po->record[i], po->flags, > > + po->overflow_t); > > > > /* Protect against an uncleared _FLAG_MORE */ > > nr_records = nr_records + le16_to_cpu(po->count); > > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h > > index 20ca2fe2ca8e..c7958311ce5f 100644 > > --- a/drivers/cxl/core/trace.h > > +++ b/drivers/cxl/core/trace.h > > @@ -8,6 +8,9 @@ > > > > #include <cxl.h> > > #include <linux/tracepoint.h> > > +#include <linux/pci.h> > > + > > +#include <cxlmem.h> > > > > #define CXL_RAS_UC_CACHE_DATA_PARITY BIT(0) > > #define CXL_RAS_UC_CACHE_ADDR_PARITY BIT(1) > > @@ -103,6 +106,86 @@ TRACE_EVENT(cxl_aer_correctable_error, > > ) > > ); > > > > +#define __show_poison_source(source) \ > > + __print_symbolic(source, \ > > + { CXL_POISON_SOURCE_UNKNOWN, "Unknown" }, \ > > + { CXL_POISON_SOURCE_EXTERNAL, "External" }, \ > > + { CXL_POISON_SOURCE_INTERNAL, "Internal" }, \ > > + { CXL_POISON_SOURCE_INJECTED, "Injected" }, \ > > + { CXL_POISON_SOURCE_VENDOR, "Vendor" }) > > + > > +#define show_poison_source(source) \ > > + (((source > CXL_POISON_SOURCE_INJECTED) && \ > > + (source != CXL_POISON_SOURCE_VENDOR)) ? "Reserved" \ > > + : __show_poison_source(source)) > > + > > +#define show_poison_flags(flags) \ > > + __print_flags(flags, "|", \ > > + { CXL_POISON_FLAG_MORE, "More" }, \ > > + { CXL_POISON_FLAG_OVERFLOW, "Overflow" }, \ > > + { CXL_POISON_FLAG_SCANNING, "Scanning" }) > > + > > +#define __cxl_poison_addr(record) \ > > + (le64_to_cpu(record->address)) > > +#define cxl_poison_record_dpa(record) \ > > + (__cxl_poison_addr(record) & CXL_POISON_START_MASK) > > +#define cxl_poison_record_source(record) \ > > + (__cxl_poison_addr(record) & CXL_POISON_SOURCE_MASK) > > +#define cxl_poison_record_length(record) \ > > + (le32_to_cpu(record->length) * CXL_POISON_LEN_MULT) > > +#define cxl_poison_overflow(flags, time) \ > > + (flags & CXL_POISON_FLAG_OVERFLOW ? le64_to_cpu(time) : 0) > > + > > +TRACE_EVENT(cxl_poison, > > + > > + TP_PROTO(struct cxl_memdev *memdev, const struct pci_dev *pcidev, > > + struct cxl_region *region, > > + const struct cxl_poison_record *record, > > + u8 flags, __le64 overflow_t), > > + > > + TP_ARGS(memdev, pcidev, region, record, flags, overflow_t), > > + > > + TP_STRUCT__entry( > > + __string(memdev, dev_name(&memdev->dev)) > > + __string(pcidev, dev_name(&pcidev->dev)) > > + __string(region, region) > > + __field(u64, overflow_t) > > + __field(u64, dpa) > > + __field(u32, length) > > + __array(char, uuid, 16) > > + __field(u8, source) > > + __field(u8, flags) > > + ), > > + > > + TP_fast_assign( > > + __assign_str(memdev, dev_name(&memdev->dev)); > > + __assign_str(pcidev, dev_name(&pcidev->dev)); > > + __entry->overflow_t = cxl_poison_overflow(flags, overflow_t); > > + __entry->dpa = cxl_poison_record_dpa(record); > > + __entry->length = cxl_poison_record_length(record); > > + __entry->source = cxl_poison_record_source(record); > > + __entry->flags = flags; > > + if (region) { > > + __assign_str(region, dev_name(®ion->dev)); > > + memcpy(__entry->uuid, ®ion->params.uuid, 16); > > + } else { > > + __assign_str(region, ""); > > + memset(__entry->uuid, 0, 16); > > + } > > + ), > > + > > + TP_printk("memdev=%s pcidev=%s region=%s region_uuid=%pU dpa=0x%llx length=0x%x source=%s flags=%s overflow_time=%llu", > > + __get_str(memdev), > > + __get_str(pcidev), > > + __get_str(region), > > + __entry->uuid, > > + __entry->dpa, > > + __entry->length, > > + show_poison_source(__entry->source), > > + show_poison_flags(__entry->flags), > > + __entry->overflow_t) > > +); > > + > > #endif /* _CXL_EVENTS_H */ > > > > #define TRACE_INCLUDE_FILE trace >
| |