Re: [PATCH v2 3/4] cxl/core: Add region info to cxl_general_media and cxl_dram events

[Dan Williams <dan.j.williams@intel.com>]

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.