| Message ID | 800328a3fdffa0f3ece709be337bd64a07089bff.1713842838.git.alison.schofield@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add DPA->HPA translation to dram & general_media | expand |
alison.schofield@ wrote: > From: Alison Schofield <alison.schofield@intel.com> > [snip] > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h > index 161bdb5734b0..2e24364b2b8d 100644 > --- a/drivers/cxl/core/trace.h > +++ b/drivers/cxl/core/trace.h > @@ -14,6 +14,28 @@ > #include <cxlmem.h> > #include "core.h" > > +#ifndef __CXL_EVENTS_DECLARE_ONCE_ONLY > +#define __CXL_EVENTS_DECLARE_ONCE_ONLY > +static inline > +void store_region_info(const struct cxl_memdev *cxlmd, u64 dpa, uuid_t *uuid, > + u64 *hpa) > +{ > + struct cxl_region *cxlr; > + > + cxlr = cxl_dpa_to_region(cxlmd, dpa); This would normally be: (somewhat nitty though...) if (!cxlr) { uuid_copy(uuid, &uuid_null); *hpa = ULLONG_MAX; return; } uuid_copy(uuid, &cxlr->params.uuid); *hpa = cxl_trace_hpa(cxlr, cxlmd, dpa); } But in this context I don't think it is critical. Reviewed-by: Ira Weiny <ira.weiny@intel.com> [snip]
On Mon, Apr 22, 2024 at 09:23:32PM -0700, Ira Weiny wrote: > alison.schofield@ wrote: > > From: Alison Schofield <alison.schofield@intel.com> > > > > [snip] > > > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h > > index 161bdb5734b0..2e24364b2b8d 100644 > > --- a/drivers/cxl/core/trace.h > > +++ b/drivers/cxl/core/trace.h > > @@ -14,6 +14,28 @@ > > #include <cxlmem.h> > > #include "core.h" > > > > +#ifndef __CXL_EVENTS_DECLARE_ONCE_ONLY > > +#define __CXL_EVENTS_DECLARE_ONCE_ONLY > > +static inline > > +void store_region_info(const struct cxl_memdev *cxlmd, u64 dpa, uuid_t *uuid, > > + u64 *hpa) > > +{ > > + struct cxl_region *cxlr; > > + > > + cxlr = cxl_dpa_to_region(cxlmd, dpa); > > This would normally be: (somewhat nitty though...) > > if (!cxlr) { > uuid_copy(uuid, &uuid_null); > *hpa = ULLONG_MAX; > return; > } > > uuid_copy(uuid, &cxlr->params.uuid); > *hpa = cxl_trace_hpa(cxlr, cxlmd, dpa); > } > > But in this context I don't think it is critical. Thanks for pointing out. When I switch to inline I didn't 'see' that. I'd like to fix it up. Let me see what else comes in. --Alison > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > > > [snip]
alison.schofield@ wrote: > From: Alison Schofield <alison.schofield@intel.com> > > User space may need to know which region, if any, maps the DPAs > (device physical addresses) reported in a cxl_general_media or > cxl_dram event. Since the mapping can change, the kernel provides > this information at the time the event occurs. This informs user > space that at event <timestamp> this <region> mapped this <DPA> > to this <HPA>. > > Add the same region info that is included in the cxl_poison trace > event: the DPA->HPA translation, region name, and region uuid. > Introduce and use new helpers that lookup that region info using > the struct cxl_memdev and a DPA. > > The new fields are inserted in the trace event and no existing > fields are modified. If the DPA is not mapped, user will see: > hpa=ULLONG_MAX, region="", and uuid=0 > > This work must be protected by dpa_rwsem & region_rwsem since > it is looking up region mappings. > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/core.h | 6 +++++ > drivers/cxl/core/mbox.c | 17 ++++++++++--- > drivers/cxl/core/region.c | 8 ++++++ > drivers/cxl/core/trace.h | 52 +++++++++++++++++++++++++++++++++++---- > 4 files changed, 74 insertions(+), 9 deletions(-) > > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h > index 625394486459..2fd8d9797f36 100644 > --- a/drivers/cxl/core/core.h > +++ b/drivers/cxl/core/core.h > @@ -30,8 +30,14 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port); > struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa); > u64 cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd, > u64 dpa); > +const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa); > > #else > +static inline > +const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa) > +{ > + return ""; > +} > static inline u64 > cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd, u64 dpa) > { > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index 9adda4795eb7..3c1c37d5fcb0 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -842,14 +842,23 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd, > enum cxl_event_type event_type, > const uuid_t *uuid, union cxl_event *evt) > { > + if (event_type == CXL_CPER_EVENT_MEM_MODULE) { > + trace_cxl_memory_module(cxlmd, type, &evt->mem_module); > + return; > + } > + if (event_type == CXL_CPER_EVENT_GENERIC) { > + trace_cxl_generic_event(cxlmd, type, uuid, &evt->generic); > + return; > + } > + > + /* Protect trace events that do DPA->HPA translations */ > + guard(rwsem_read)(&cxl_region_rwsem); > + guard(rwsem_read)(&cxl_dpa_rwsem); > + > if (event_type == CXL_CPER_EVENT_GEN_MEDIA) > trace_cxl_general_media(cxlmd, type, &evt->gen_media); > else if (event_type == CXL_CPER_EVENT_DRAM) > trace_cxl_dram(cxlmd, type, &evt->dram); > - else if (event_type == CXL_CPER_EVENT_MEM_MODULE) > - trace_cxl_memory_module(cxlmd, type, &evt->mem_module); > - else > - trace_cxl_generic_event(cxlmd, type, uuid, &evt->generic); > } > EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, CXL); > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 45eb9c560fd6..a5b1eaee1e58 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -2723,6 +2723,14 @@ struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa) > return ctx.cxlr; > } > > +const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa) > +{ > + struct cxl_region *cxlr = cxl_dpa_to_region(cxlmd, dpa); > + > + /* trace __string() assignment requires "", not NULL */ > + return cxlr ? dev_name(&cxlr->dev) : ""; > +} > + > static bool cxl_is_hpa_in_range(u64 hpa, struct cxl_region *cxlr, int pos) > { > struct cxl_region_params *p = &cxlr->params; > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h > index 161bdb5734b0..2e24364b2b8d 100644 > --- a/drivers/cxl/core/trace.h > +++ b/drivers/cxl/core/trace.h > @@ -14,6 +14,28 @@ > #include <cxlmem.h> > #include "core.h" > > +#ifndef __CXL_EVENTS_DECLARE_ONCE_ONLY > +#define __CXL_EVENTS_DECLARE_ONCE_ONLY > +static inline > +void store_region_info(const struct cxl_memdev *cxlmd, u64 dpa, uuid_t *uuid, > + u64 *hpa) > +{ > + struct cxl_region *cxlr; > + > + cxlr = cxl_dpa_to_region(cxlmd, dpa); > + if (cxlr) { > + uuid_copy(uuid, &cxlr->params.uuid); > + *hpa = cxl_trace_hpa(cxlr, cxlmd, dpa); > + } else { > + uuid_copy(uuid, &uuid_null); > + *hpa = ULLONG_MAX; > + } > +} > +#endif /* __CXL_EVENTS_DECLARE_ONCE_ONLY */ This ifdef usage looks awkward... > + > +#define rec_pa_to_dpa(record) \ > + (le64_to_cpu(rec->phys_addr) & CXL_DPA_MASK) > + > #define CXL_RAS_UC_CACHE_DATA_PARITY BIT(0) > #define CXL_RAS_UC_CACHE_ADDR_PARITY BIT(1) > #define CXL_RAS_UC_CACHE_BE_PARITY BIT(2) > @@ -330,10 +352,14 @@ TRACE_EVENT(cxl_general_media, > __field(u8, channel) > __field(u32, device) > __array(u8, comp_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE) > - __field(u16, validity_flags) > /* Following are out of order to pack trace record */ > + __field(u64, hpa) > + __field_struct(uuid_t, region_uuid) > + __field(u16, validity_flags) > __field(u8, rank) > __field(u8, dpa_flags) > + __string(region_name, > + cxl_trace_to_region_name(cxlmd, rec_pa_to_dpa(record))) ...and this looks complicated. A bit too much dynamic resolution happening within the trace function for my taste. Just do the region lookup in cxl_event_trace_record() and pass it in. That also makes the rwsem usage more apparent rather than digging through trace to find the dependency.
On Tue, Apr 23, 2024 at 10:17:44PM -0700, Dan Williams wrote: > alison.schofield@ wrote: > > From: Alison Schofield <alison.schofield@intel.com> > > > > User space may need to know which region, if any, maps the DPAs > > (device physical addresses) reported in a cxl_general_media or > > cxl_dram event. Since the mapping can change, the kernel provides > > this information at the time the event occurs. This informs user > > space that at event <timestamp> this <region> mapped this <DPA> > > to this <HPA>. > > > > Add the same region info that is included in the cxl_poison trace > > event: the DPA->HPA translation, region name, and region uuid. > > Introduce and use new helpers that lookup that region info using > > the struct cxl_memdev and a DPA. > > > > The new fields are inserted in the trace event and no existing > > fields are modified. If the DPA is not mapped, user will see: > > hpa=ULLONG_MAX, region="", and uuid=0 > > > > This work must be protected by dpa_rwsem & region_rwsem since > > it is looking up region mappings. > > > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- > > drivers/cxl/core/core.h | 6 +++++ > > drivers/cxl/core/mbox.c | 17 ++++++++++--- > > drivers/cxl/core/region.c | 8 ++++++ > > drivers/cxl/core/trace.h | 52 +++++++++++++++++++++++++++++++++++---- > > 4 files changed, 74 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h > > index 625394486459..2fd8d9797f36 100644 > > --- a/drivers/cxl/core/core.h > > +++ b/drivers/cxl/core/core.h > > @@ -30,8 +30,14 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port); > > struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa); > > u64 cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd, > > u64 dpa); > > +const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa); > > > > #else > > +static inline > > +const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa) > > +{ > > + return ""; > > +} > > static inline u64 > > cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd, u64 dpa) > > { > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > > index 9adda4795eb7..3c1c37d5fcb0 100644 > > --- a/drivers/cxl/core/mbox.c > > +++ b/drivers/cxl/core/mbox.c > > @@ -842,14 +842,23 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd, > > enum cxl_event_type event_type, > > const uuid_t *uuid, union cxl_event *evt) > > { > > + if (event_type == CXL_CPER_EVENT_MEM_MODULE) { > > + trace_cxl_memory_module(cxlmd, type, &evt->mem_module); > > + return; > > + } > > + if (event_type == CXL_CPER_EVENT_GENERIC) { > > + trace_cxl_generic_event(cxlmd, type, uuid, &evt->generic); > > + return; > > + } > > + > > + /* Protect trace events that do DPA->HPA translations */ > > + guard(rwsem_read)(&cxl_region_rwsem); > > + guard(rwsem_read)(&cxl_dpa_rwsem); > > + > > if (event_type == CXL_CPER_EVENT_GEN_MEDIA) > > trace_cxl_general_media(cxlmd, type, &evt->gen_media); > > else if (event_type == CXL_CPER_EVENT_DRAM) > > trace_cxl_dram(cxlmd, type, &evt->dram); > > - else if (event_type == CXL_CPER_EVENT_MEM_MODULE) > > - trace_cxl_memory_module(cxlmd, type, &evt->mem_module); > > - else > > - trace_cxl_generic_event(cxlmd, type, uuid, &evt->generic); > > } > > EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, CXL); > > > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > > index 45eb9c560fd6..a5b1eaee1e58 100644 > > --- a/drivers/cxl/core/region.c > > +++ b/drivers/cxl/core/region.c > > @@ -2723,6 +2723,14 @@ struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa) > > return ctx.cxlr; > > } > > > > +const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa) > > +{ > > + struct cxl_region *cxlr = cxl_dpa_to_region(cxlmd, dpa); > > + > > + /* trace __string() assignment requires "", not NULL */ > > + return cxlr ? dev_name(&cxlr->dev) : ""; > > +} > > + > > static bool cxl_is_hpa_in_range(u64 hpa, struct cxl_region *cxlr, int pos) > > { > > struct cxl_region_params *p = &cxlr->params; > > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h > > index 161bdb5734b0..2e24364b2b8d 100644 > > --- a/drivers/cxl/core/trace.h > > +++ b/drivers/cxl/core/trace.h > > @@ -14,6 +14,28 @@ > > #include <cxlmem.h> > > #include "core.h" > > > > +#ifndef __CXL_EVENTS_DECLARE_ONCE_ONLY > > +#define __CXL_EVENTS_DECLARE_ONCE_ONLY > > +static inline > > +void store_region_info(const struct cxl_memdev *cxlmd, u64 dpa, uuid_t *uuid, > > + u64 *hpa) > > +{ > > + struct cxl_region *cxlr; > > + > > + cxlr = cxl_dpa_to_region(cxlmd, dpa); > > + if (cxlr) { > > + uuid_copy(uuid, &cxlr->params.uuid); > > + *hpa = cxl_trace_hpa(cxlr, cxlmd, dpa); > > + } else { > > + uuid_copy(uuid, &uuid_null); > > + *hpa = ULLONG_MAX; > > + } > > +} > > +#endif /* __CXL_EVENTS_DECLARE_ONCE_ONLY */ > > This ifdef usage looks awkward... I only mimic'd others that defined static inline funcs in trace header files. I originally thought it would be protected from this define that already wraps this header file content, but it doesn't. #if !defined(_CXL_EVENTS_H) || defined(TRACE_HEADER_MULTI_READ) #define _CXL_EVENTS_H > > > + > > +#define rec_pa_to_dpa(record) \ > > + (le64_to_cpu(rec->phys_addr) & CXL_DPA_MASK) > > + > > #define CXL_RAS_UC_CACHE_DATA_PARITY BIT(0) > > #define CXL_RAS_UC_CACHE_ADDR_PARITY BIT(1) > > #define CXL_RAS_UC_CACHE_BE_PARITY BIT(2) > > @@ -330,10 +352,14 @@ TRACE_EVENT(cxl_general_media, > > __field(u8, channel) > > __field(u32, device) > > __array(u8, comp_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE) > > - __field(u16, validity_flags) > > /* Following are out of order to pack trace record */ > > + __field(u64, hpa) > > + __field_struct(uuid_t, region_uuid) > > + __field(u16, validity_flags) > > __field(u8, rank) > > __field(u8, dpa_flags) > > + __string(region_name, > > + cxl_trace_to_region_name(cxlmd, rec_pa_to_dpa(record))) > > ...and this looks complicated. > > A bit too much dynamic resolution happening within the trace function > for my taste. Just do the region lookup in cxl_event_trace_record() and > pass it in. That also makes the rwsem usage more apparent rather than > digging through trace to find the dependency. When these cxl_general_media,dram,poison trace events were originally created once of the goals was to push work down *here* so that if the trace events were not enable, no needless work is done. When you suggest doing the region lookup before calling the trace handler, I'm thinking something like this where the region lookup work still gets skipped if tracing is not enabled: if (trace_cxl_general_media_enabled()) { cxlr = lookup_region(cxlmd, record); trace_cxl_general_media(...., cxlr); -- Alison
Alison Schofield wrote: > On Tue, Apr 23, 2024 at 10:17:44PM -0700, Dan Williams wrote: > > alison.schofield@ wrote: > > > From: Alison Schofield <alison.schofield@intel.com> > > > > > > User space may need to know which region, if any, maps the DPAs > > > (device physical addresses) reported in a cxl_general_media or > > > cxl_dram event. Since the mapping can change, the kernel provides > > > this information at the time the event occurs. This informs user > > > space that at event <timestamp> this <region> mapped this <DPA> > > > to this <HPA>. > > > > > > Add the same region info that is included in the cxl_poison trace > > > event: the DPA->HPA translation, region name, and region uuid. > > > Introduce and use new helpers that lookup that region info using > > > the struct cxl_memdev and a DPA. > > > > > > The new fields are inserted in the trace event and no existing > > > fields are modified. If the DPA is not mapped, user will see: > > > hpa=ULLONG_MAX, region="", and uuid=0 > > > > > > This work must be protected by dpa_rwsem & region_rwsem since > > > it is looking up region mappings. > > > > > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> [..] > > > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h > > > index 161bdb5734b0..2e24364b2b8d 100644 > > > --- a/drivers/cxl/core/trace.h > > > +++ b/drivers/cxl/core/trace.h > > > @@ -14,6 +14,28 @@ > > > #include <cxlmem.h> > > > #include "core.h" > > > > > > +#ifndef __CXL_EVENTS_DECLARE_ONCE_ONLY > > > +#define __CXL_EVENTS_DECLARE_ONCE_ONLY > > > +static inline > > > +void store_region_info(const struct cxl_memdev *cxlmd, u64 dpa, uuid_t *uuid, > > > + u64 *hpa) > > > +{ > > > + struct cxl_region *cxlr; > > > + > > > + cxlr = cxl_dpa_to_region(cxlmd, dpa); > > > + if (cxlr) { > > > + uuid_copy(uuid, &cxlr->params.uuid); > > > + *hpa = cxl_trace_hpa(cxlr, cxlmd, dpa); > > > + } else { > > > + uuid_copy(uuid, &uuid_null); > > > + *hpa = ULLONG_MAX; > > > + } > > > +} > > > +#endif /* __CXL_EVENTS_DECLARE_ONCE_ONLY */ > > > > This ifdef usage looks awkward... > > I only mimic'd others that defined static inline funcs in trace header files. That's fine, but that awkward trace hack can be skipped altogether if an @region argument is passed to the tracepoints. > I originally thought it would be protected from this define that already > wraps this header file content, but it doesn't. > > #if !defined(_CXL_EVENTS_H) || defined(TRACE_HEADER_MULTI_READ) > #define _CXL_EVENTS_H That's what TRACE_HEADER_MULTI_READ allows which is needed for the tricky way that tracepoints are built. > > > + > > > +#define rec_pa_to_dpa(record) \ > > > + (le64_to_cpu(rec->phys_addr) & CXL_DPA_MASK) > > > + > > > #define CXL_RAS_UC_CACHE_DATA_PARITY BIT(0) > > > #define CXL_RAS_UC_CACHE_ADDR_PARITY BIT(1) > > > #define CXL_RAS_UC_CACHE_BE_PARITY BIT(2) > > > @@ -330,10 +352,14 @@ TRACE_EVENT(cxl_general_media, > > > __field(u8, channel) > > > __field(u32, device) > > > __array(u8, comp_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE) > > > - __field(u16, validity_flags) > > > /* Following are out of order to pack trace record */ > > > + __field(u64, hpa) > > > + __field_struct(uuid_t, region_uuid) > > > + __field(u16, validity_flags) > > > __field(u8, rank) > > > __field(u8, dpa_flags) > > > + __string(region_name, > > > + cxl_trace_to_region_name(cxlmd, rec_pa_to_dpa(record))) > > > > ...and this looks complicated. > > > > A bit too much dynamic resolution happening within the trace function > > for my taste. Just do the region lookup in cxl_event_trace_record() and > > pass it in. That also makes the rwsem usage more apparent rather than > > digging through trace to find the dependency. > > When these cxl_general_media,dram,poison trace events were originally > created once of the goals was to push work down *here* so that if the > trace events were not enable, no needless work is done. The moment a code path needs to hold a lock, (2 locks in this case!), over a tracepoint it has already incurred significant overhead. So if the name of the game is "skip doing unnecessary work when the tracepoint is disabled", then just check if the tracepoint is disabled, something like: diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index 60a51ea3ff25..31f44101582e 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -851,14 +851,23 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd, return; } - /* Protect trace events that do DPA->HPA translations */ - guard(rwsem_read)(&cxl_region_rwsem); - guard(rwsem_read)(&cxl_dpa_rwsem); - - if (event_type == CXL_CPER_EVENT_GEN_MEDIA) - trace_cxl_general_media(cxlmd, type, &evt->gen_media); - else if (event_type == CXL_CPER_EVENT_DRAM) - trace_cxl_dram(cxlmd, type, &evt->dram); + if (trace_cxl_general_media_enabled() || trace_cxl_dram_enabled()) { + /* + * These trace points are annotated with HPA and corresponding + * region translation. Take topology mutation locks and lookup + * { HPA, REGION } from { DPA, MEMDEV } in the event record. + */ + guard(rwsem_read)(&cxl_region_rwsem); + guard(rwsem_read)(&cxl_dpa_rwsem); + + region = ... + hpa = ... + + if (event_type == CXL_CPER_EVENT_GEN_MEDIA) + trace_cxl_general_media(cxlmd, type, region, hpa, &evt->gen_media); + else if (event_type == CXL_CPER_EVENT_DRAM) + trace_cxl_dram(cxlmd, type, region, hpa, &evt->dram); + } } EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, CXL); > When you suggest doing the region lookup before calling the trace handler, > I'm thinking something like this where the region lookup work still gets > skipped if tracing is not enabled: > > if (trace_cxl_general_media_enabled()) { > cxlr = lookup_region(cxlmd, record); > trace_cxl_general_media(...., cxlr); > I am chuckling because I wrote all of the above diatribe after finishing that "no needless work is done sentence". After building the example, compile testing a version of it, and pasting it into the mail *then* I read this next paragraph. So yes, we came to the same conclusion. Please use trace_*_enabled() which makes it clear what the locks are protecting. Some lockdep_assert_held() usage in the lookup helpers would not hurt either.
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h index 625394486459..2fd8d9797f36 100644 --- a/drivers/cxl/core/core.h +++ b/drivers/cxl/core/core.h @@ -30,8 +30,14 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port); struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa); u64 cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd, u64 dpa); +const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa); #else +static inline +const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa) +{ + return ""; +} static inline u64 cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd, u64 dpa) { diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index 9adda4795eb7..3c1c37d5fcb0 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -842,14 +842,23 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd, enum cxl_event_type event_type, const uuid_t *uuid, union cxl_event *evt) { + if (event_type == CXL_CPER_EVENT_MEM_MODULE) { + trace_cxl_memory_module(cxlmd, type, &evt->mem_module); + return; + } + if (event_type == CXL_CPER_EVENT_GENERIC) { + trace_cxl_generic_event(cxlmd, type, uuid, &evt->generic); + return; + } + + /* Protect trace events that do DPA->HPA translations */ + guard(rwsem_read)(&cxl_region_rwsem); + guard(rwsem_read)(&cxl_dpa_rwsem); + if (event_type == CXL_CPER_EVENT_GEN_MEDIA) trace_cxl_general_media(cxlmd, type, &evt->gen_media); else if (event_type == CXL_CPER_EVENT_DRAM) trace_cxl_dram(cxlmd, type, &evt->dram); - else if (event_type == CXL_CPER_EVENT_MEM_MODULE) - trace_cxl_memory_module(cxlmd, type, &evt->mem_module); - else - trace_cxl_generic_event(cxlmd, type, uuid, &evt->generic); } EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, CXL); diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 45eb9c560fd6..a5b1eaee1e58 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -2723,6 +2723,14 @@ struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa) return ctx.cxlr; } +const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa) +{ + struct cxl_region *cxlr = cxl_dpa_to_region(cxlmd, dpa); + + /* trace __string() assignment requires "", not NULL */ + return cxlr ? dev_name(&cxlr->dev) : ""; +} + static bool cxl_is_hpa_in_range(u64 hpa, struct cxl_region *cxlr, int pos) { struct cxl_region_params *p = &cxlr->params; diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h index 161bdb5734b0..2e24364b2b8d 100644 --- a/drivers/cxl/core/trace.h +++ b/drivers/cxl/core/trace.h @@ -14,6 +14,28 @@ #include <cxlmem.h> #include "core.h" +#ifndef __CXL_EVENTS_DECLARE_ONCE_ONLY +#define __CXL_EVENTS_DECLARE_ONCE_ONLY +static inline +void store_region_info(const struct cxl_memdev *cxlmd, u64 dpa, uuid_t *uuid, + u64 *hpa) +{ + struct cxl_region *cxlr; + + cxlr = cxl_dpa_to_region(cxlmd, dpa); + if (cxlr) { + uuid_copy(uuid, &cxlr->params.uuid); + *hpa = cxl_trace_hpa(cxlr, cxlmd, dpa); + } else { + uuid_copy(uuid, &uuid_null); + *hpa = ULLONG_MAX; + } +} +#endif /* __CXL_EVENTS_DECLARE_ONCE_ONLY */ + +#define rec_pa_to_dpa(record) \ + (le64_to_cpu(rec->phys_addr) & CXL_DPA_MASK) + #define CXL_RAS_UC_CACHE_DATA_PARITY BIT(0) #define CXL_RAS_UC_CACHE_ADDR_PARITY BIT(1) #define CXL_RAS_UC_CACHE_BE_PARITY BIT(2) @@ -330,10 +352,14 @@ TRACE_EVENT(cxl_general_media, __field(u8, channel) __field(u32, device) __array(u8, comp_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE) - __field(u16, validity_flags) /* Following are out of order to pack trace record */ + __field(u64, hpa) + __field_struct(uuid_t, region_uuid) + __field(u16, validity_flags) __field(u8, rank) __field(u8, dpa_flags) + __string(region_name, + cxl_trace_to_region_name(cxlmd, rec_pa_to_dpa(record))) ), TP_fast_assign( @@ -354,18 +380,24 @@ TRACE_EVENT(cxl_general_media, memcpy(__entry->comp_id, &rec->component_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE); __entry->validity_flags = get_unaligned_le16(&rec->validity_flags); + __assign_str(region_name, + cxl_trace_to_region_name(cxlmd, __entry->dpa)); + store_region_info(cxlmd, __entry->dpa, &__entry->region_uuid, + &__entry->hpa); ), CXL_EVT_TP_printk("dpa=%llx dpa_flags='%s' " \ "descriptor='%s' type='%s' transaction_type='%s' channel=%u rank=%u " \ - "device=%x comp_id=%s validity_flags='%s'", + "device=%x comp_id=%s validity_flags='%s' " \ + "hpa=%llx region=%s region_uuid=%pUb", __entry->dpa, show_dpa_flags(__entry->dpa_flags), 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) + show_valid_flags(__entry->validity_flags), + __entry->hpa, __get_str(region_name), &__entry->region_uuid ) ); @@ -417,10 +449,14 @@ TRACE_EVENT(cxl_dram, __field(u32, nibble_mask) __field(u32, row) __array(u8, cor_mask, CXL_EVENT_DER_CORRECTION_MASK_SIZE) + __field(u64, hpa) + __field_struct(uuid_t, region_uuid) __field(u8, rank) /* Out of order to pack trace record */ __field(u8, bank_group) /* Out of order to pack trace record */ __field(u8, bank) /* Out of order to pack trace record */ __field(u8, dpa_flags) /* Out of order to pack trace record */ + __string(region_name, + cxl_trace_to_region_name(cxlmd, rec_pa_to_dpa(record))) ), TP_fast_assign( @@ -444,12 +480,17 @@ TRACE_EVENT(cxl_dram, __entry->column = get_unaligned_le16(rec->column); memcpy(__entry->cor_mask, &rec->correction_mask, CXL_EVENT_DER_CORRECTION_MASK_SIZE); + __assign_str(region_name, + cxl_trace_to_region_name(cxlmd, __entry->dpa)); + store_region_info(cxlmd, __entry->dpa, &__entry->region_uuid, + &__entry->hpa); ), CXL_EVT_TP_printk("dpa=%llx dpa_flags='%s' descriptor='%s' type='%s' " \ "transaction_type='%s' channel=%u rank=%u nibble_mask=%x " \ "bank_group=%u bank=%u row=%u column=%u cor_mask=%s " \ - "validity_flags='%s'", + "validity_flags='%s' " \ + "hpa=%llx region=%s region_uuid=%pUb", __entry->dpa, show_dpa_flags(__entry->dpa_flags), show_event_desc_flags(__entry->descriptor), show_mem_event_type(__entry->type), @@ -458,7 +499,8 @@ TRACE_EVENT(cxl_dram, __entry->bank_group, __entry->bank, __entry->row, __entry->column, __print_hex(__entry->cor_mask, CXL_EVENT_DER_CORRECTION_MASK_SIZE), - show_dram_valid_flags(__entry->validity_flags) + show_dram_valid_flags(__entry->validity_flags), + __entry->hpa, __get_str(region_name), &__entry->region_uuid ) );