On Mon, 21 Feb 2022 09:57:20 -0800
Beau Belgrave <beaub@xxxxxxxxxxxxxxxxxxx> wrote:
> > We've been trying to follow certain naming convention for the APIs and to
> > provide similar usage patterns for all types of trace events that are
> > supported by the library so far (dynamic, synthetic and histograms). If
> > 'XXX' is the type of the event ('user_event' in your case), the pattern
> > looks like this:
> >
> > tracefs_XXX_alloc() - this constructor just allocates memory and initializes
> > the descriptor object without modifying anything on the system. We allow for
> > multiple constructor function, in the case when your objects has to many
> > possible configurations and it is hard to do everything in a single API.
> > Looking into your implementation, such constructor can do half of the work
> > done in 'tracefs_user_event_group_create()'
> >
> > int tracefs_XXX_create(struct tracefs_XXX *evt) - This is the API that
> > actually adds your event on the system. Note that it takes just one argument
> > that is the object itself. Again, looking into your implementation, this
> > will combine the other half of tracefs_user_event_group_create() and
> > tracefs_user_event_register().
>
> Are you and Steven aligned on this convention?
I would say you are both correct ;-)
The problem is, this doesn't really match the other events. The other
events are created in the kernel, and become a normal event. The big
difference here is that the event is tested in user space. It will be
hard to make it match the same criteria as kprobes, dynamic events, and
eprobes.
>
> The request looked slightly different:
> See https://lore.kernel.org/linux-trace-devel/20220121192833.GA3128@kbox/T/#m2bcf53c373fbeaba2c46d1a053b3174171167e4e
>
> >
> > int tracefs_XXX_destroy(struct tracefs_XXX *evt) This API removes your event
> > from the system. The first argument is again the object. If needed, here you
> > can use a second argument that is 'bool force'.
> >
> > int tracefs_XXX_free(struct tracefs_XXX *evt) - just to free the memory
> >
> >
> > > +struct tracefs_user_event_group *tracefs_user_event_group_create(void);
> > > +
> > > +void tracefs_user_event_group_close(struct tracefs_user_event_group *group);
> > > +
> > > +int tracefs_user_event_delete(const char *name);
> > > +
> > > +struct tracefs_user_event *
> > > +tracefs_user_event_register(struct tracefs_user_event_group *group,
> > > + const char *name, enum tracefs_uevent_flags flags,
> > > + struct tracefs_uevent_item *items);
> > > +
> > > +bool tracefs_user_event_test(struct tracefs_user_event *event);
> >
> > And maybe "test" is redundant. You can do the check in tracefs_XXX_create() and return an error if it fails.
> >
>
> Test is required so user programs can tell when write should be called.
> Otherwise excessive calculations and stack data are pushed for no good
> reason.
This is the part I'm talking about. The "user_event" here is actually
executed in the application, and this API is to handle it. This is
where the user events diverge from the other events.
The user events create the event like any other dynamic event (we could
look at keeping that part similar with the other APIs). But then
everything is different.
When the event is created, the application is given a location on a
special mmapped page that represents if the event is enabled or not.
Then the application is to read it *at the event site* to know if it
should record the event into the ring buffer or not.
That's what the test is. It's equivalent to the "static_branch" that
tracepoints use in the kernel, except this isn't a static branch
(although we could possibly do something like that in the future!).
The application will:
if (tracefs_user_event_test(event)) {
tracefs_user_event_write(event, ...);
}
Where if the event is enabled (by an external program), then it is to
record the events. This is the API that does that.
And test is not redundant at all.
>
> > > +
> > > +int tracefs_user_event_write(struct tracefs_user_event *event,
> > > + struct tracefs_uevent_item *items);
> >
> > The "write" is OK.
> >
> > Maybe we can change 'tracefs_user_event' to 'tracefs_usrevent' since we already use 'tracefs_dynevent' for dynamic events.
> >
> > What do you think?
Not really. This is closer to tracefs_printf(). But thinking about this
more, perhaps "write" is a bit confusing as we use it to write to
tracefs files.
What about: tracefs_user_event_record() ?
As this is exactly what it is doing. It's recording the event.
> >
>
> I'm happy to do whatever, I just want to ensure you and Steven are
> aligned.
Thanks!
-- Steve
|