From: Alison Schofield <alison.schofield@intel.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>,
nvdimm@lists.linux.dev, linux-cxl@vger.kernel.org,
Dave Jiang <dave.jiang@intel.com>
Subject: Re: [ndctl PATCH v10 3/7] cxl/event_trace: add a private context for private parsers
Date: Sun, 10 Mar 2024 15:39:09 -0700 [thread overview]
Message-ID: <Ze42jVuoaFcshrV4@aschofie-mobl2> (raw)
In-Reply-To: <65e8fe00756cd_12713294f2@dwillia2-mobl3.amr.corp.intel.com.notmuch>
On Wed, Mar 06, 2024 at 03:36:32PM -0800, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> >
> > CXL event tracing provides helpers to iterate through a trace
> > buffer and extract events of interest. It offers two parsing
> > options: a default parser that adds every field of an event to
> > a json object, and a private parsing option where the caller can
> > parse each event as it wishes.
> >
> > Although the private parser can do some conditional parsing based
> > on field values, it has no method to receive additional information
> > needed to make parsing decisions in the callback.
> >
> > Add a private_ctx field to the existing 'struct event_context'.
> > Replace the jlist_head parameter, used in the default parser,
> > with the private_ctx.
> >
> > This is in preparation for adding a private parser requiring
> > additional context for cxl_poison events.
> >
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> > ---
> > cxl/event_trace.c | 2 +-
> > cxl/event_trace.h | 3 ++-
> > 2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/cxl/event_trace.c b/cxl/event_trace.c
> > index 93a95f9729fd..bdad0c19dbd4 100644
> > --- a/cxl/event_trace.c
> > +++ b/cxl/event_trace.c
> > @@ -221,7 +221,7 @@ static int cxl_event_parse(struct tep_event *event, struct tep_record *record,
> >
> > if (event_ctx->parse_event)
> > return event_ctx->parse_event(event, record,
> > - &event_ctx->jlist_head);
> > + event_ctx->private_ctx);
>
> Given ->parse_event() is already a method of an event_ctx object, might
> as will pass the entirety of event_ctx to its own method as a typical
> 'this' pointer.
Thanks, done!
Now passing event_ctx struct as a param to its parse_event method.
>
> You could then also use container_of() to get to event_ctx creator data
> and skip the type-unsafety of a 'void *' pointer. However, I say that
> without having looked to see how feasible it is to wrap private data
> around an event_ctx instance.
Got rid of the void but didn't go down above path.
First off, I don't see need to find an event_ctx field from private_ctx.
I can see the use, but not the need. There are 2 users presently, one with
no private data and one with private data (cxl_poison).
If there were multiple users, or multiple users in sight, adding a flexible
array member to a private context struct and using that in container_of to
find the event_ctx might be useful.
Next rev simply includes the poison_ctx directly in the event_ctx,
avoiding the void ptr usage.
Please take a look at next rev.
Thanks,
Alison
next prev parent reply other threads:[~2024-03-10 22:39 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-06 18:42 [ndctl PATCH v10 0/7] Support poison list retrieval alison.schofield
2024-03-06 18:42 ` [ndctl PATCH v10 1/7] libcxl: add interfaces for GET_POISON_LIST mailbox commands alison.schofield
2024-03-06 23:07 ` Dan Williams
2024-03-06 18:42 ` [ndctl PATCH v10 2/7] cxl: add an optional pid check to event parsing alison.schofield
2024-03-06 23:11 ` Dan Williams
2024-03-06 18:42 ` [ndctl PATCH v10 3/7] cxl/event_trace: add a private context for private parsers alison.schofield
2024-03-06 23:36 ` Dan Williams
2024-03-10 22:39 ` Alison Schofield [this message]
2024-03-06 18:42 ` [ndctl PATCH v10 4/7] cxl/event_trace: add helpers to retrieve tep fields by type alison.schofield
2024-03-06 23:53 ` Dan Williams
2024-03-08 4:06 ` Alison Schofield
2024-03-06 18:42 ` [ndctl PATCH v10 5/7] cxl/list: collect and parse media_error records alison.schofield
2024-03-07 0:50 ` Dan Williams
2024-03-06 18:42 ` [ndctl PATCH v10 6/7] cxl/list: add --media-errors option to cxl list alison.schofield
2024-03-07 1:09 ` Dan Williams
2024-03-06 18:42 ` [ndctl PATCH v10 7/7] cxl/test: add cxl-poison.sh unit test alison.schofield
2024-03-06 23:03 ` [ndctl PATCH v10 0/7] Support poison list retrieval Dan Williams
2024-03-08 3:58 ` Alison Schofield
2024-03-08 4:03 ` Dan Williams
2024-03-10 19:21 ` Alison Schofield
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=Ze42jVuoaFcshrV4@aschofie-mobl2 \
--to=alison.schofield@intel.com \
--cc=dan.j.williams@intel.com \
--cc=dave.jiang@intel.com \
--cc=linux-cxl@vger.kernel.org \
--cc=nvdimm@lists.linux.dev \
--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).