From: Alison Schofield <alison.schofield@intel.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>,
Jonathan Cameron <jonathan.cameron@huawei.com>,
Dave Jiang <dave.jiang@intel.com>,
Vishal Verma <vishal.l.verma@intel.com>,
Ira Weiny <ira.weiny@intel.com>,
linux-cxl@vger.kernel.org, Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH v4 3/3] cxl/core: Add region info to cxl_general_media and cxl_dram events
Date: Sun, 28 Apr 2024 19:31:05 -0700 [thread overview]
Message-ID: <Zi8GaXl3odQqQiMz@aschofie-mobl2> (raw)
In-Reply-To: <662c471aefe43_b6e029445@dwillia2-mobl3.amr.corp.intel.com.notmuch>
On Fri, Apr 26, 2024 at 05:30:19PM -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.
> >
> > 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>
> > ---
> > drivers/cxl/core/mbox.c | 36 ++++++++++++++++++++++++++------
> > drivers/cxl/core/trace.h | 44 +++++++++++++++++++++++++++++++--------
> > include/linux/cxl-event.h | 10 +++++++++
> > 3 files changed, 75 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 9adda4795eb7..df0fc2a4570f 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -842,14 +842,38 @@ 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_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)
> > + if (event_type == CXL_CPER_EVENT_MEM_MODULE) {
> > trace_cxl_memory_module(cxlmd, type, &evt->mem_module);
> > - else
> > + return;
> > + }
> > + if (event_type == CXL_CPER_EVENT_GENERIC) {
> > trace_cxl_generic_event(cxlmd, type, uuid, &evt->generic);
> > + return;
>
> Minor, but I think you could shave a couple more lines by keeping the
> else-if tree going and skip the early "return" calls with a final:
>
> else if (trace_cxl_general_media_enabled() || trace_cxl_dram_enabled())
>
I think I didn't do that because this last 'if' breaks the pattern
since it's not based on event_type. We're doing something different
in this chunk of code.
A switch would be well suited, but it uses even more LOC - like this:
+
+
+ switch (event_type) {
+ case CXL_CPER_EVENT_MEM_MODULE:
+ trace_cxl_memory_module(cxlmd, type, &evt->mem_module);
+ break;
+
+ case CXL_CPER_EVENT_GENERIC:
+ trace_cxl_generic_event(cxlmd, type, uuid, &evt->generic);
+ break;
+
+ case CXL_CPER_EVENT_GEN_MEDIA:
+ case CXL_CPER_EVENT_DRAM:
+ u64 dpa, hpa = ULLONG_MAX;
+ struct cxl_region *cxlr;
+
+ if (!trace_cxl_general_media_enabled() &&
+ !trace_cxl_dram_enabled())
+ break;
+ /*
+ * These trace points are annotated with HPA and region
+ * translations. 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);
+
+ dpa = le64_to_cpu(evt->common.phys_addr) & CXL_DPA_MASK;
+ cxlr = cxl_dpa_to_region(cxlmd, dpa);
+ if (cxlr)
+ hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
+
+ if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
+ trace_cxl_general_media(cxlmd, type, cxlr, hpa,
+ &evt->gen_media);
+ else if (event_type == CXL_CPER_EVENT_DRAM)
+ trace_cxl_dram(cxlmd, type, cxlr, hpa, &evt->dram);
+ break;
+ default:
+ break;
+ }
+
Maybe if I just move the comment before the changed chunk of code,
the break in flow will be easier to follow.
> > + }
/* Move the comment below to here */
> > +
> > + if (trace_cxl_general_media_enabled() || trace_cxl_dram_enabled()) {
> > + u64 dpa, hpa = ULLONG_MAX;
> > + struct cxl_region *cxlr;
> > +
> > + /*
> > + * These trace points are annotated with HPA and region
> > + * translations. 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);
> > +
> > + dpa = le64_to_cpu(evt->common.phys_addr) & CXL_DPA_MASK;
>
> A merge issue here for Dave to handle here since CXL_DPA_MASK is known
> broken. Do you want to wait until that bikeshedding on CXL_DPA_MASK
> replacement name completes, or just take the simple:
>
> -#define CXL_DPA_FLAGS_MASK 0x3F
> +#define CXL_DPA_FLAGS_MASK 0x3FULL
>
> ...for now?
I'm optimistic we can get that fixes patch in. Don't worry about it ;)
>
> Otherwise,
>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
prev parent reply other threads:[~2024-04-29 2:31 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-26 3:46 [PATCH v4 0/3] Add DPA->HPA translation to dram & general_media events alison.schofield
2024-04-26 3:46 ` [PATCH v4 1/3] cxl/region: Move cxl_dpa_to_region() work to the region driver alison.schofield
2024-04-26 3:46 ` [PATCH v4 2/3] cxl/region: Move cxl_trace_hpa() " alison.schofield
2024-04-26 3:46 ` [PATCH v4 3/3] cxl/core: Add region info to cxl_general_media and cxl_dram events alison.schofield
2024-04-27 0:30 ` Dan Williams
2024-04-29 2:31 ` Alison Schofield [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Zi8GaXl3odQqQiMz@aschofie-mobl2 \
--to=alison.schofield@intel.com \
--cc=dan.j.williams@intel.com \
--cc=dave.jiang@intel.com \
--cc=dave@stgolabs.net \
--cc=ira.weiny@intel.com \
--cc=jonathan.cameron@huawei.com \
--cc=linux-cxl@vger.kernel.org \
--cc=rostedt@goodmis.org \
--cc=vishal.l.verma@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).