Re: [PATCH v4 2/5] cxl/trace: Add TRACE support for CXL media-error records

From: Jonathan Cameron
Date: Thu Jan 12 2023 - 06:26:56 EST

On Thu, 15 Dec 2022 13:17:44 -0800
alison.schofield@xxxxxxxxx wrote:

> From: Alison Schofield <alison.schofield@xxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Alison Schofield <alison.schofield@xxxxxxxxx>

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


> ---
> 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(&region->dev));
> + memcpy(__entry->uuid, &region->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