Re: [PATCH v1 1/3] libtracefs: Add user_events to libtracefs sources

[Beau Belgrave <beaub@linux.microsoft.com>]

On Tue, Feb 22, 2022 at 12:31:43PM -0500, Steven Rostedt wrote:
> On Fri, 18 Feb 2022 14:50:56 -0800
> Beau Belgrave <beaub@linux.microsoft.com> wrote:
> 
> > diff --git a/include/tracefs-local.h b/include/tracefs-local.h
> > index bf157e1..e768cba 100644
> > --- a/include/tracefs-local.h
> > +++ b/include/tracefs-local.h
> > @@ -119,4 +119,28 @@ int trace_rescan_events(struct tep_handle *tep,
> >  struct tep_event *get_tep_event(struct tep_handle *tep,
> >  				const char *system, const char *name);
> >  
> > +/* Internal interface for ftrace user events */
> > +
> > +struct tracefs_user_event_group;
> > +
> > +struct tracefs_user_event
> > +{
> > +	int write_index;
> > +	int status_index;
> > +	int iovecs;
> > +	int rels;
> > +	int len;
> > +	struct tracefs_user_event_group *group;
> > +	struct tracefs_user_event *next;
> > +};
> > +
> > +struct tracefs_user_event_group
> > +{
> > +	int fd;
> > +	int mmap_len;
> > +	char *mmap;
> > +	pthread_mutex_t lock;
> > +	struct tracefs_user_event *events;
> > +};
> > +
> >  #endif /* _TRACE_FS_LOCAL_H */
> >
> 
> 
> 
> > +/**
> > + * tracefs_user_event_test - Tests if an event is currently enabled
> > + * @event: User event to test
> > + *
> > + * Tests if the @event is valid and currently enabled on the system.
> > + *
> > + * Return true if enabled, false otherwise.
> > + */
> > +bool tracefs_user_event_test(struct tracefs_user_event *event)
> > +{
> > +	return event && event->group->mmap[event->status_index] != 0;
> > +}
> > +
> 
> I was thinking we could even make the above faster by making it a static
> inline in the tracefs.h header file.
> 
> In tracefs.h:
> 
> struct tracefs_user_event {
> 	char			*enable;
> };
> 
> static inline bool tracefs_user_event_test(struct tracefs_user_event *event)
> {
> 	return event && event->enable[0] != 0;
> }
> 
> Then in tracefs-local.h:
> 
> struct tracefs_user_event_internal {
> 	struct tracefs_user_event	event_external;
> 	[...]
> };
> 
> Then have in tracefs_user_event_register():
> 
> 	event->write_index = reg.write_index;
> 	event->status_index = reg.status_index;
> 	event->group = group;
> 
> 	event->event_external.enable = &event->group->mmap[event->status_index];
> 
> 	[..]
> 
> 	return &event->event_external;
> 
> All the other functions wouldn't even have to do a container_of() call, as
> the event_external will be the first field in the struct it needs.
> 
> 	struct tracefs_user_event_internal *event = (struct tracefs_user_event_internal *)event_external;
> 
> Or make the above a helper function:
> 
> #define INTERNAL_EVENT(e) ((struct tracefs_user_event_internal *)e)
> 
> 	struct tracefs_user_event_internal *event = INTERNAL_EVENT(event_external);
> 
> 
> Then we save on the function call, and allow the code to do the test inline.

Yes, I was thinking along the same lines. I saw how things were split
between private / public in the headers and wasn't sure how to
accomplish this.

What you have above seems like a great way to accomplish this without exposing
too much out.

Thanks,
-Beau