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

[Yordan Karadzhov <y.karadz@xxxxxxxxx>]

On 19.02.22 г. 0:50 ч., Beau Belgrave 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 */
> diff --git a/include/tracefs.h b/include/tracefs.h
> index 1848ad0..7871dfe 100644
> --- a/include/tracefs.h
> +++ b/include/tracefs.h
> @@ -571,4 +571,64 @@ struct tracefs_synth *tracefs_sql(struct tep_handle *tep, const char *name,
>   struct tep_event *
>   tracefs_synth_get_event(struct tep_handle *tep, struct tracefs_synth *synth);
>   
> +/* User events */
> +enum tracefs_uevent_type {
> +	TRACEFS_UEVENT_END,
> +	TRACEFS_UEVENT_u8,
> +	TRACEFS_UEVENT_s8,
> +	TRACEFS_UEVENT_u16,
> +	TRACEFS_UEVENT_s16,
> +	TRACEFS_UEVENT_u32,
> +	TRACEFS_UEVENT_s32,
> +	TRACEFS_UEVENT_u64,
> +	TRACEFS_UEVENT_s64,
> +	TRACEFS_UEVENT_string,
> +	TRACEFS_UEVENT_struct,
> +	TRACEFS_UEVENT_varray,
> +	TRACEFS_UEVENT_vstring,
> +};
> +
> +enum tracefs_uevent_flags {
> +	/* None */
> +	TRACEFS_UEVENT_FLAG_NONE = 0,
> +
> +	/* When BPF is attached, use iterator/no copy */
> +	TRACEFS_UEVENT_FLAG_bpf_iter = 1 << 0,
> +};
> +
> +struct tracefs_uevent_item {
> +	/* Type of item */
> +	enum tracefs_uevent_type type;
> +
> +	/* Length of data, optional during register */
> +	int len;
> +
> +	union {
> +		/* Used during write */
> +		const void *data;
> +
> +		/* Used during register */
> +		const char *name;
> +	};
> +};
> +
> +struct tracefs_user_event;
> +struct tracefs_user_event_group;
> +

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().

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.

> +
> +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?

Thanks!
Yordan