On 21.02.22 г. 21:16 ч., Steven Rostedt wrote:
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/#m2bcf53c373fbeaba2c46d1a053b3174171167e4eint 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.
Thanks a lot for clarifying this and sorry about my confusion!I have one last question. Do you consider as a valid use case that the library must support, someone to do a just 'test" without writing after this, or to "write" without testing first?
thanks! Yordan
+ +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
|