| Message ID | 6e975df49a62cdb544791633fdd1a998a0b60164.1709748564.git.alison.schofield@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support poison list retrieval | expand |
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.
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 --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);