| Message ID | 20211101090904.81454-4-tz.stoyanov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libtracefs dynamic events support | expand |
On 1.11.21 г. 11:08, Tzvetomir Stoyanov (VMware) wrote: > In order to be consistent with the other APIs, a new set of kprobe APIs > is introduced: > tracefs_kprobe_alloc(); > tracefs_kretprobe_alloc(); > tracefs_kprobe_create(); > tracefs_kprobe_destroy(); > tracefs_kprobe_free(); > These APIs work with kprobe context, represented by the tracefs_dynevent > structure. > > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> > --- > include/tracefs.h | 8 ++ > src/tracefs-kprobes.c | 168 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 176 insertions(+) > > diff --git a/include/tracefs.h b/include/tracefs.h > index 4e721eb..85185ce 100644 > --- a/include/tracefs.h > +++ b/include/tracefs.h > @@ -247,6 +247,14 @@ enum tracefs_kprobe_type { > TRACEFS_KRETPROBE, > }; > > +struct tracefs_dynevent * > +tracefs_kprobe_alloc(const char *system, const char *event, const char *addr, const char *format); > +struct tracefs_dynevent * > +tracefs_kretprobe_alloc(const char *system, const char *event, > + const char *addr, const char *format, int max); > +int tracefs_kprobe_create(struct tracefs_dynevent *kprobe); > +int tracefs_kprobe_destroy(struct tracefs_dynevent *kprobe, bool force); > +void tracefs_kprobe_free(struct tracefs_dynevent *kprobe); > int tracefs_kprobe_raw(const char *system, const char *event, > const char *addr, const char *format); > int tracefs_kretprobe_raw(const char *system, const char *event, > diff --git a/src/tracefs-kprobes.c b/src/tracefs-kprobes.c > index d4c5f9e..906f914 100644 > --- a/src/tracefs-kprobes.c > +++ b/src/tracefs-kprobes.c > @@ -20,6 +20,134 @@ > #define KPROBE_EVENTS "kprobe_events" > #define KPROBE_DEFAULT_GROUP "kprobes" > > +static struct tracefs_dynevent * > +kprobe_alloc(enum trace_dynevent_type type, const char *system, const char *event, > + const char *addr, const char *format) > +{ > + struct tracefs_dynevent *kp; > + const char *sys = system; > + const char *ename = event; > + char *tmp; > + > + if (!addr) { > + errno = EBADMSG; > + return NULL; > + } > + if (!sys) > + sys = KPROBE_DEFAULT_GROUP; > + > + if (!event) { > + ename = strdup(addr); > + if (!ename) > + return NULL; > + tmp = strchr(ename, ':'); > + if (tmp) > + *tmp = '\0'; > + } > + > + kp = dynevent_alloc(type, sys, ename, addr, format); > + if (!event) > + free((char *)ename); > + > + return kp; > +} > + > + > +/** > + * tracefs_kprobe_alloc - Allocate new kprobe context > + * @system: The system name (NULL for the default kprobes) > + * @event: The event to create (NULL to use @addr for the event) > + * @addr: The function and offset (or address) to insert the probe > + * @format: The format string to define the probe. > + * > + * Allocate a kprobe context that will be in the @system group (or kprobes if > + * @system is NULL). Have the name of @event (or @addr if @event is > + * NULL). Will be inserted to @addr (function name, with or without > + * offset, or a address). And the @format will define the format > + * of the kprobe. See the Linux documentation file under: > + * Documentation/trace/kprobetrace.rst > + * The kprobe is not created in the system. > + * > + * Return a pointer to a kprobe context on success, or NULL on error. > + * The returned pointer must be freed with tracefs_kprobe_free() > + * > + * errno will be set to EBADMSG if addr is NULL. > + */ > +struct tracefs_dynevent * > +tracefs_kprobe_alloc(const char *system, const char *event, const char *addr, const char *format) > + > +{ > + return kprobe_alloc(TRACE_DYNEVENT_KPROBE, system, event, addr, format); > +} > + > +/** > + * tracefs_kretprobe_alloc - Allocate new kretprobe context > + * @system: The system name (NULL for the default kprobes) > + * @event: The event to create (NULL to use @addr for the event) > + * @addr: The function and offset (or address) to insert the retprobe > + * @format: The format string to define the retprobe. > + * @max is not documented. > + * Allocate a kretprobe that will be in the @system group (or kprobes if > + * @system is NULL). Have the name of @event (or @addr if @event is > + * NULL). Will be inserted to @addr (function name, with or without > + * offset, or a address). And the @format will define the raw format > + * of the kprobe. See the Linux documentation file under: > + * Documentation/trace/kprobetrace.rst > + * The kretprobe is not created in the system. > + * > + * Return a pointer to a kprobe context on success, or NULL on error. > + * The returned pointer must be freed with tracefs_kprobe_free() > + * > + * errno will be set to EBADMSG if addr is NULL. > + */ > +struct tracefs_dynevent * > +tracefs_kretprobe_alloc(const char *system, const char *event, > + const char *addr, const char *format, int max) > +{ > + struct tracefs_dynevent *kp; > + int ret; > + > + kp = kprobe_alloc(TRACE_DYNEVENT_KRETPROBE, system, event, addr, format); > + if (!kp) > + return NULL; > + if (max) { > + free(kp->prefix); > + kp->prefix = NULL; > + ret = asprintf(&kp->prefix, "r%d:", max); > + if (ret < 0) > + goto error; > + } > + > + return kp; > +error: > + dynevent_free(kp); > + return NULL; > +} > + > +/** > + * tracefs_kprobe_create - Create a kprobe or kretprobe in the system > + * @kprobe: Pointer to a kprobe context, describing the probe > + * > + * Return 0 on success, or -1 on error. > + */ > +int tracefs_kprobe_create(struct tracefs_dynevent *kprobe) > +{ > + return dynevent_create(kprobe); > +} > + hmm, what will happen if if you have a code like this: struct tracefs_dynevent *synth = tracefs_synth_alloc(...) int ret = tracefs_kprobe_create(synth); Are you just giving several additional fake names to the same function ('dynevent_create()')? > +/** > + * tracefs_kprobe_free - Free a kprobe context > + * @kprobe: Pointer to a kprobe context > + * > + * The kprobe/kretprobe, described by this context, is not > + * removed from the system by this API. It only frees the memory. > + */ > +void tracefs_kprobe_free(struct tracefs_dynevent *kprobe) > +{ > + dynevent_free(kprobe); > +} > + The same argument applys here. Thanks! Yordan > + > static int insert_kprobe(const char *type, const char *system, > const char *event, const char *addr, > const char *format) > @@ -474,3 +602,43 @@ int tracefs_kprobe_clear_probe(const char *system, const char *event, bool force > > return ret < 0 ? -1 : 0; > } > + > +/** > + * tracefs_kprobe_destroy - Remove a kprobe or kretprobe from the system > + * @kprobe: A kprobe context, describing the kprobe that will be deleted. > + * If NULL, all kprobes and kretprobes in the system will be deleted > + * @force: Will attempt to disable all kprobe events and clear them > + * > + * The kprobe/kretprobe context is not freed by this API. > + * It only removes the probe from the system. > + * > + * Return 0 on success, or -1 on error. > + */ > +int tracefs_kprobe_destroy(struct tracefs_dynevent *kprobe, bool force) > +{ > + char **instance_list; > + int ret; > + > + if (!kprobe) { > + if (tracefs_instance_file_clear(NULL, KPROBE_EVENTS) == 0) > + return 0; > + if (!force) > + return -1; > + /* Attempt to disable all kprobe events */ > + return kprobe_clear_probes(NULL, force); > + } > + > + /* > + * Since we know we are disabling a specific event, try > + * to disable it first before clearing it. > + */ > + if (force) { > + instance_list = tracefs_instances(NULL); > + disable_events(kprobe->system, kprobe->event, instance_list); > + tracefs_list_free(instance_list); > + } > + > + ret = dynevent_destroy(kprobe); > + > + return ret < 0 ? -1 : 0; > +} >
On Mon, Nov 1, 2021 at 7:22 PM Yordan Karadzhov <y.karadz@gmail.com> wrote: > > [ ... ] > > +/** > > + * tracefs_kprobe_create - Create a kprobe or kretprobe in the system > > + * @kprobe: Pointer to a kprobe context, describing the probe > > + * > > + * Return 0 on success, or -1 on error. > > + */ > > +int tracefs_kprobe_create(struct tracefs_dynevent *kprobe) > > +{ > > + return dynevent_create(kprobe); > > +} > > + > > hmm, what will happen if if you have a code like this: > > > struct tracefs_dynevent *synth = tracefs_synth_alloc(...) > > int ret = tracefs_kprobe_create(synth); > > > Are you just giving several additional fake names to the same function ('dynevent_create()')? > > Yes, that will work. All dynamic events - kprobes, uprobes, eprobes and synthetic events have almost the same configuration logic, which is handled by these dynevent_... internal APIs. That makes the code more consistent and reusable. I see three possible approaches to handle that: 1. Create structures like that: struct tracefs_kprobe { struct tracefs_dynevent dynevent; }; and use only that type in krpobe specific APIs: int tracefs_kprobe_create(struct tracefs_kprobe *kprobe) { return dynevent_create(&kprobe->dynevent); } 2. Expose the dynevent_... APIs directly: struct tracefs_dynevent *synth = tracefs_dynevent_alloc(TRACE_DYNEVENT_SYNTH, ...); struct tracefs_dynevent *kprobe = tracefs_dynevent_alloc(TRACE_DYNEVENT_KPROBE, ...); ret = tracefs_dynevent_create(synth); ret = tracefs_dynevent_create(kprobe); The problem here is that there are some small differences between dynamic events - some parameters are mandatory for one event and optional for another. That can be handled in the tracefs_dynevent_alloc() API, but could be confusing for the library users. 3. Mixed between 1) and 2), the current approach. The first one adds a lot of overhead, a different set of APIs for each type of dynamic event - but maybe it is more clear for the library users. The second is straightforward for implementation, less APIs - but that could be confusing for the users. > > +/** > > + * tracefs_kprobe_free - Free a kprobe context > > + * @kprobe: Pointer to a kprobe context > > + * > > + * The kprobe/kretprobe, described by this context, is not > > + * removed from the system by this API. It only frees the memory. > > + */ > > +void tracefs_kprobe_free(struct tracefs_dynevent *kprobe) > > +{ > > + dynevent_free(kprobe); > > +} > > + > > The same argument applys here. > > Thanks! > Yordan > Thanks for the review!
On 2.11.21 г. 6:58, Tzvetomir Stoyanov wrote: > On Mon, Nov 1, 2021 at 7:22 PM Yordan Karadzhov <y.karadz@gmail.com> wrote: >> >> > [ ... ] >>> +/** >>> + * tracefs_kprobe_create - Create a kprobe or kretprobe in the system >>> + * @kprobe: Pointer to a kprobe context, describing the probe >>> + * >>> + * Return 0 on success, or -1 on error. >>> + */ >>> +int tracefs_kprobe_create(struct tracefs_dynevent *kprobe) >>> +{ >>> + return dynevent_create(kprobe); >>> +} >>> + >> >> hmm, what will happen if if you have a code like this: >> >> >> struct tracefs_dynevent *synth = tracefs_synth_alloc(...) >> >> int ret = tracefs_kprobe_create(synth); >> >> >> Are you just giving several additional fake names to the same function ('dynevent_create()')? >> >> > > Yes, that will work. All dynamic events - kprobes, uprobes, eprobes > and synthetic events have almost the same configuration logic, which > is handled by these dynevent_... internal APIs. That makes the code > more consistent and reusable. > I see three possible approaches to handle that: > 1. Create structures like that: > struct tracefs_kprobe { > struct tracefs_dynevent dynevent; > }; > and use only that type in krpobe specific APIs: > int tracefs_kprobe_create(struct tracefs_kprobe *kprobe) > { > return dynevent_create(&kprobe->dynevent); > } > > 2. Expose the dynevent_... APIs directly: > struct tracefs_dynevent *synth = > tracefs_dynevent_alloc(TRACE_DYNEVENT_SYNTH, ...); I would prefer taking this approach. It is OK to also have several helper constructors like tracefs_kprobe_alloc(), tracefs_synth_alloc(), etc. that will call internally dynevent_alloc(). In principle having multiple constructors is OK. > struct tracefs_dynevent *kprobe = > tracefs_dynevent_alloc(TRACE_DYNEVENT_KPROBE, ...); > > ret = tracefs_dynevent_create(synth); > ret = tracefs_dynevent_create(kprobe); > Yes, this is OK as well. We only have a single type (struct tracefs_dynevent) so there must be a single create. And what is even more important: there must be ONLY ONE destructor. Think about the usecase of the APIs. You create an object using one of the constructors. Later at some point you want to use the objects and after this finally you want to destroy it. If we have multiple/specialized "create" and "destroy" versions for the same object type, the user must somehow keep its own record of which constructor had been used trough the entire live of the object in order to know which is the proper destructor to call at the end. Thanks! Yordan > The problem here is that there are some small differences between > dynamic events - some parameters are mandatory for one event and > optional for another. That can be handled in the > tracefs_dynevent_alloc() API, but could be confusing for the library > users. > > 3. Mixed between 1) and 2), the current approach. > > The first one adds a lot of overhead, a different set of APIs for each > type of dynamic event - but maybe it is more clear for the library > users. The second is straightforward for implementation, less APIs - > but that could be confusing for the users. > >>> +/** >>> + * tracefs_kprobe_free - Free a kprobe context >>> + * @kprobe: Pointer to a kprobe context >>> + * >>> + * The kprobe/kretprobe, described by this context, is not >>> + * removed from the system by this API. It only frees the memory. >>> + */ >>> +void tracefs_kprobe_free(struct tracefs_dynevent *kprobe) >>> +{ >>> + dynevent_free(kprobe); >>> +} >>> + >> >> The same argument applys here. >> >> Thanks! >> Yordan >> > > Thanks for the review! >
On Tue, 2 Nov 2021 09:43:16 +0200 Yordan Karadzhov <y.karadz@gmail.com> wrote: > > 2. Expose the dynevent_... APIs directly: > > struct tracefs_dynevent *synth = > > tracefs_dynevent_alloc(TRACE_DYNEVENT_SYNTH, ...); > > I would prefer taking this approach. It is OK to also have several helper constructors like tracefs_kprobe_alloc(), > tracefs_synth_alloc(), etc. that will call internally dynevent_alloc(). > > In principle having multiple constructors is OK. I agree with having multiple constructors, as that makes it easier to think about what is being created and how to manipulate it. The dynamic event structure has a type field and it should be used to make sure that modifications that only should happen to kprobes only happens on dynamic events created as a kprobe, and errors on everything else. > > > > struct tracefs_dynevent *kprobe = > > tracefs_dynevent_alloc(TRACE_DYNEVENT_KPROBE, ...); > > > > ret = tracefs_dynevent_create(synth); > > ret = tracefs_dynevent_create(kprobe); > > > > Yes, this is OK as well. Agreed. > > We only have a single type (struct tracefs_dynevent) so there must be a single create. And what is even more important: > there must be ONLY ONE destructor. I wouldn't be so strong as to use "must" but I do agree that it will simplify the usage of the API. It would be more analogous to free() where you have several different constructors that state in the man page (must be freed with free). Same here. Each constructor's man page must state: "must be freed with tracefs_dynevent_free()" Removing from the system any dynamic event should also just use tracefs_dynevent_destroy(). Having the type of event in the structure means that knowing how to destroy it can be multiplexed inside the library, and not burden the user in such cases. This way, the user could create a bunch of dynamic event types in an array or linked list, and destroy them at the end without having to keep track of which one is which. -- Steve > > Think about the usecase of the APIs. You create an object using one of the constructors. Later at some point you want to > use the objects and after this finally you want to destroy it. If we have multiple/specialized "create" and "destroy" > versions for the same object type, the user must somehow keep its own record of which constructor had been used trough > the entire live of the object in order to know which is the proper destructor to call at the end.
diff --git a/include/tracefs.h b/include/tracefs.h index 4e721eb..85185ce 100644 --- a/include/tracefs.h +++ b/include/tracefs.h @@ -247,6 +247,14 @@ enum tracefs_kprobe_type { TRACEFS_KRETPROBE, }; +struct tracefs_dynevent * +tracefs_kprobe_alloc(const char *system, const char *event, const char *addr, const char *format); +struct tracefs_dynevent * +tracefs_kretprobe_alloc(const char *system, const char *event, + const char *addr, const char *format, int max); +int tracefs_kprobe_create(struct tracefs_dynevent *kprobe); +int tracefs_kprobe_destroy(struct tracefs_dynevent *kprobe, bool force); +void tracefs_kprobe_free(struct tracefs_dynevent *kprobe); int tracefs_kprobe_raw(const char *system, const char *event, const char *addr, const char *format); int tracefs_kretprobe_raw(const char *system, const char *event, diff --git a/src/tracefs-kprobes.c b/src/tracefs-kprobes.c index d4c5f9e..906f914 100644 --- a/src/tracefs-kprobes.c +++ b/src/tracefs-kprobes.c @@ -20,6 +20,134 @@ #define KPROBE_EVENTS "kprobe_events" #define KPROBE_DEFAULT_GROUP "kprobes" +static struct tracefs_dynevent * +kprobe_alloc(enum trace_dynevent_type type, const char *system, const char *event, + const char *addr, const char *format) +{ + struct tracefs_dynevent *kp; + const char *sys = system; + const char *ename = event; + char *tmp; + + if (!addr) { + errno = EBADMSG; + return NULL; + } + if (!sys) + sys = KPROBE_DEFAULT_GROUP; + + if (!event) { + ename = strdup(addr); + if (!ename) + return NULL; + tmp = strchr(ename, ':'); + if (tmp) + *tmp = '\0'; + } + + kp = dynevent_alloc(type, sys, ename, addr, format); + if (!event) + free((char *)ename); + + return kp; +} + + +/** + * tracefs_kprobe_alloc - Allocate new kprobe context + * @system: The system name (NULL for the default kprobes) + * @event: The event to create (NULL to use @addr for the event) + * @addr: The function and offset (or address) to insert the probe + * @format: The format string to define the probe. + * + * Allocate a kprobe context that will be in the @system group (or kprobes if + * @system is NULL). Have the name of @event (or @addr if @event is + * NULL). Will be inserted to @addr (function name, with or without + * offset, or a address). And the @format will define the format + * of the kprobe. See the Linux documentation file under: + * Documentation/trace/kprobetrace.rst + * The kprobe is not created in the system. + * + * Return a pointer to a kprobe context on success, or NULL on error. + * The returned pointer must be freed with tracefs_kprobe_free() + * + * errno will be set to EBADMSG if addr is NULL. + */ +struct tracefs_dynevent * +tracefs_kprobe_alloc(const char *system, const char *event, const char *addr, const char *format) + +{ + return kprobe_alloc(TRACE_DYNEVENT_KPROBE, system, event, addr, format); +} + +/** + * tracefs_kretprobe_alloc - Allocate new kretprobe context + * @system: The system name (NULL for the default kprobes) + * @event: The event to create (NULL to use @addr for the event) + * @addr: The function and offset (or address) to insert the retprobe + * @format: The format string to define the retprobe. + * + * Allocate a kretprobe that will be in the @system group (or kprobes if + * @system is NULL). Have the name of @event (or @addr if @event is + * NULL). Will be inserted to @addr (function name, with or without + * offset, or a address). And the @format will define the raw format + * of the kprobe. See the Linux documentation file under: + * Documentation/trace/kprobetrace.rst + * The kretprobe is not created in the system. + * + * Return a pointer to a kprobe context on success, or NULL on error. + * The returned pointer must be freed with tracefs_kprobe_free() + * + * errno will be set to EBADMSG if addr is NULL. + */ +struct tracefs_dynevent * +tracefs_kretprobe_alloc(const char *system, const char *event, + const char *addr, const char *format, int max) +{ + struct tracefs_dynevent *kp; + int ret; + + kp = kprobe_alloc(TRACE_DYNEVENT_KRETPROBE, system, event, addr, format); + if (!kp) + return NULL; + if (max) { + free(kp->prefix); + kp->prefix = NULL; + ret = asprintf(&kp->prefix, "r%d:", max); + if (ret < 0) + goto error; + } + + return kp; +error: + dynevent_free(kp); + return NULL; +} + +/** + * tracefs_kprobe_create - Create a kprobe or kretprobe in the system + * @kprobe: Pointer to a kprobe context, describing the probe + * + * Return 0 on success, or -1 on error. + */ +int tracefs_kprobe_create(struct tracefs_dynevent *kprobe) +{ + return dynevent_create(kprobe); +} + +/** + * tracefs_kprobe_free - Free a kprobe context + * @kprobe: Pointer to a kprobe context + * + * The kprobe/kretprobe, described by this context, is not + * removed from the system by this API. It only frees the memory. + */ +void tracefs_kprobe_free(struct tracefs_dynevent *kprobe) +{ + dynevent_free(kprobe); +} + + static int insert_kprobe(const char *type, const char *system, const char *event, const char *addr, const char *format) @@ -474,3 +602,43 @@ int tracefs_kprobe_clear_probe(const char *system, const char *event, bool force return ret < 0 ? -1 : 0; } + +/** + * tracefs_kprobe_destroy - Remove a kprobe or kretprobe from the system + * @kprobe: A kprobe context, describing the kprobe that will be deleted. + * If NULL, all kprobes and kretprobes in the system will be deleted + * @force: Will attempt to disable all kprobe events and clear them + * + * The kprobe/kretprobe context is not freed by this API. + * It only removes the probe from the system. + * + * Return 0 on success, or -1 on error. + */ +int tracefs_kprobe_destroy(struct tracefs_dynevent *kprobe, bool force) +{ + char **instance_list; + int ret; + + if (!kprobe) { + if (tracefs_instance_file_clear(NULL, KPROBE_EVENTS) == 0) + return 0; + if (!force) + return -1; + /* Attempt to disable all kprobe events */ + return kprobe_clear_probes(NULL, force); + } + + /* + * Since we know we are disabling a specific event, try + * to disable it first before clearing it. + */ + if (force) { + instance_list = tracefs_instances(NULL); + disable_events(kprobe->system, kprobe->event, instance_list); + tracefs_list_free(instance_list); + } + + ret = dynevent_destroy(kprobe); + + return ret < 0 ? -1 : 0; +}
In order to be consistent with the other APIs, a new set of kprobe APIs is introduced: tracefs_kprobe_alloc(); tracefs_kretprobe_alloc(); tracefs_kprobe_create(); tracefs_kprobe_destroy(); tracefs_kprobe_free(); These APIs work with kprobe context, represented by the tracefs_dynevent structure. Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> --- include/tracefs.h | 8 ++ src/tracefs-kprobes.c | 168 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 176 insertions(+)