diff mbox series

[ndctl,v10,3/7] cxl/event_trace: add a private context for private parsers

Message ID 6e975df49a62cdb544791633fdd1a998a0b60164.1709748564.git.alison.schofield@intel.com
State Superseded
Headers show
Series Support poison list retrieval | expand

Commit Message

Alison Schofield March 6, 2024, 6:42 p.m. UTC
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(-)

Comments

Dan Williams March 6, 2024, 11:36 p.m. UTC | #1
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.

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.
Alison Schofield March 10, 2024, 10:39 p.m. UTC | #2
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
diff mbox series

Patch

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);
 
 	return cxl_event_to_json(event, record, &event_ctx->jlist_head);
 }
diff --git a/cxl/event_trace.h b/cxl/event_trace.h
index 7f7773b2201f..ec61962abbc6 100644
--- a/cxl/event_trace.h
+++ b/cxl/event_trace.h
@@ -16,8 +16,9 @@  struct event_ctx {
 	struct list_head jlist_head;
 	const char *event_name; /* optional */
 	int event_pid; /* optional */
+	void *private_ctx; /* required with parse_event() */
 	int (*parse_event)(struct tep_event *event, struct tep_record *record,
-			   struct list_head *jlist_head); /* optional */
+			   void *private_ctx);/* optional */
 };
 
 int cxl_parse_events(struct tracefs_instance *inst, struct event_ctx *ectx);