On Mon, Feb 21, 2022 at 01:34:00PM +0200, Yordan Karadzhov wrote:
>
>
> 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().
Are you and Steven aligned on this convention?
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.
> > +
> > +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?
>
I'm happy to do whatever, I just want to ensure you and Steven are
aligned.
> Thanks!
> Yordan
Thanks,
-Beau
|