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

[Yordan Karadzhov <y.karadz@gmail.com>]

On 21.02.22 г. 21:16 ч., Steven Rostedt wrote:
> On Mon, 21 Feb 2022 09:57:20 -0800
> Beau Belgrave <beaub@linux.microsoft.com> 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.

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