Message ID | 20210219055353.2244340-1-tz.stoyanov@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | libtracefs: Add new API for open trace marker file | expand |
On Fri, 19 Feb 2021 07:53:53 +0200 "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote: > Added new API for opening trace_marker file of given instance: > tracefs_trace_marker_get_fd(); > > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> > --- > As I wrote the perf-trace.c program, I was thinking what we really should have is the following API. We can keep this API, but what would be nice is: int tracefs_print_init(struct tracefs_instance *instance); int tracefs_print(struct tracefs_instance *instance, const char *fmt, ...); int tracefs_vprint(struct tracefs_instance *instance, const char *fmt, va_list ap); void tracefs_print_reset(struct tracefs_instance *instance); Where tracefs_print_init() will open the trace_marker for that instance (NULL being the top level), and storing it in the instance structure. tracefs_print() and tracefs_vprint() will check if the trace_marker file has already been opened (tracefs_print_init() was previously called), and if not, it will open it and keep it open. Then it will write to the trace_marker file the passed in print data after formatting it (see my trace_print in perf-trace.c). The tracefs_print_reset() will simply close the trace_marker file if it was previously opened, note, so will any of the destructors of the instance. We could also have: int tracefs_raw_print_init(struct tracefs_instance *instance); int tracefs_raw_print(struct tracefs_instance *instance, void *data, int len); void tracefs_raw_print_reset(struct tracefs_instance *instance); That is the same, but instead of writing string data to the trace_marker, it would write in memory into trace_marker_raw. -- Steve
On Fri, 19 Feb 2021 09:26:42 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > Where tracefs_print_init() will open the trace_marker for that instance > (NULL being the top level), and storing it in the instance structure. BTW, in all cases, if NULL is used, we need to have a global variable to keep track of the trace_marker file descriptor. All should be marked as "close on exec", as we don't want this to be passed to new executables (if someone wants that, then they can use the API in this thread). -- Steve
On Fri, 19 Feb 2021 07:53:53 +0200 "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote: > +/** > + * tracefs_trace_marker_get_fd - Get a file descriptor of "trace_marker" in > + * given instance > + * @instance: ftrace instance, can be NULL for the top instance > + * > + * Returns -1 in case of an error, or a valid file descriptor to "trace_marker" > + * file for reading and writing. The returned FD must be closed with close(). > + */ > +static inline int tracefs_trace_marker_get_fd(struct tracefs_instance *instance) > +{ > + return tracefs_instance_file_open(instance, "trace_marker", O_RDWR); And this should be opened with "O_WRONLY", as trace_marker can't be read. -- Steve > +} > +
Hi Steven, On Fri, Feb 19, 2021 at 4:26 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Fri, 19 Feb 2021 07:53:53 +0200 > "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote: > > > Added new API for opening trace_marker file of given instance: > > tracefs_trace_marker_get_fd(); > > > > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> > > --- > > > > As I wrote the perf-trace.c program, I was thinking what we really should > have is the following API. We can keep this API, but what would be nice is: > > > int tracefs_print_init(struct tracefs_instance *instance); > > int tracefs_print(struct tracefs_instance *instance, > const char *fmt, ...); > > int tracefs_vprint(struct tracefs_instance *instance, > const char *fmt, va_list ap); > > void tracefs_print_reset(struct tracefs_instance *instance); > > Where tracefs_print_init() will open the trace_marker for that instance > (NULL being the top level), and storing it in the instance structure. You mean to hold the marker fd in the tracefs_instance structure ? I like such approach, to hold some library specific context in that structure, internally and not visible from the user. In that case we do not need tracefs_print_init() at all, the first call to some tracefs_print API will open the file. But that will make the APIs not thread safe, is it OK marker fd to be used from multiple threads at the same time ? > > tracefs_print() and tracefs_vprint() will check if the trace_marker file > has already been opened (tracefs_print_init() was previously called), and > if not, it will open it and keep it open. Then it will write to the > trace_marker file the passed in print data after formatting it (see my > trace_print in perf-trace.c). > > The tracefs_print_reset() will simply close the trace_marker file if it was > previously opened, note, so will any of the destructors of the instance. > > We could also have: > > int tracefs_raw_print_init(struct tracefs_instance *instance); > > int tracefs_raw_print(struct tracefs_instance *instance, > void *data, int len); > > void tracefs_raw_print_reset(struct tracefs_instance *instance); > > > That is the same, but instead of writing string data to the trace_marker, > it would write in memory into trace_marker_raw. I'm afraid that having too many APIs with sort of overlapping functionality could make the library hard to use ? Actually the proposed new API by this patch, tracefs_trace_marker_get_fd(), already duplicates the existing tracefs_instance_file_open() API. > > -- Steve
On Wed, 24 Feb 2021 07:35:30 +0200 Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote: > > > > As I wrote the perf-trace.c program, I was thinking what we really should > > have is the following API. We can keep this API, but what would be nice is: > > > > > > int tracefs_print_init(struct tracefs_instance *instance); > > > > int tracefs_print(struct tracefs_instance *instance, > > const char *fmt, ...); > > > > int tracefs_vprint(struct tracefs_instance *instance, > > const char *fmt, va_list ap); > > > > void tracefs_print_reset(struct tracefs_instance *instance); > > > > Where tracefs_print_init() will open the trace_marker for that instance > > (NULL being the top level), and storing it in the instance structure. > > You mean to hold the marker fd in the tracefs_instance structure ? > I like such approach, to hold some library specific context in that > structure, internally and not visible from the user. In that case we do > not need tracefs_print_init() at all, the first call to some tracefs_print > API will open the file. We need to have the tracefs_print_init() because when trace_marker writes are done, it can happen in a place that we don't want any unnecessary kernel actions. For example, cyclictest could use this, and it uses the write to trace_marker in a place that there should be minimal disturbance to the system. Having the first write open the file would require the open system call, and that would be unacceptable for the use case. The normal use cases that use trace marker does exactly what is explained above. The file descriptor is opened at the start of execution, so that it can be used later in the the code when needed without needing that open system call. Not to mention, it causes a large latency to open it first. Hence, why we need the init function. > But that will make the APIs not thread safe, is > it OK marker fd to be used from multiple threads at the same time ? Yes, because the locking would happen in the kernel. Now, the opening should have pthread mutexes around it, which would be another reason to have the init, to not need the mutex calls to open. That is, we should have this: int tracefs_print_init(struct tracefs_instance *instance) { int *print_fd = instance ? &instance->print_fd : &global_print_fd; if (*print_fd >= 0) return *print_fd; pthread_mutex_lock(&open_print_mutex); if (*print_fd < 0) { path = // get path to trace_marker for instance *print_fd = open(path, O_WRONLY); } pthread_mutex_unlock(&open_print_mutex); return *print_fd; } int tracefs_vprint(struct tracefs_instance *instance, const char *fmt, va_list ap) { int *print_fd = instance ? &instance->print_fd : &global_print_fd; if (*print_fd <= 0) { *print_fd = tracefs_print_init(instance); if (*print_fd <= 0) return -1; } // do write here. } That is, only the opening needs a mutex protection. > > > > > tracefs_print() and tracefs_vprint() will check if the trace_marker > > file has already been opened (tracefs_print_init() was previously > > called), and if not, it will open it and keep it open. Then it will > > write to the trace_marker file the passed in print data after > > formatting it (see my trace_print in perf-trace.c). > > > > The tracefs_print_reset() will simply close the trace_marker file > > if it was previously opened, note, so will any of the destructors > > of the instance. > > > > We could also have: > > > > int tracefs_raw_print_init(struct tracefs_instance > > *instance); > > > > int tracefs_raw_print(struct tracefs_instance *instance, > > void *data, int len); > > > > void tracefs_raw_print_reset(struct tracefs_instance > > *instance); > > > > > > That is the same, but instead of writing string data to the > > trace_marker, it would write in memory into trace_marker_raw. > > I'm afraid that having too many APIs with sort of overlapping > functionality could make the library hard to use ? Actually the > proposed new API by this patch, tracefs_trace_marker_get_fd(), > already duplicates the existing tracefs_instance_file_open() API. I don't think it would be more difficult to use. We can advertise the normal use cases, but always leave the choice of those hard core users that want to do a little bit extra available. I hate libraries that "hand hold" too much, where I can't do what I want to do, because the authors cater too much to the novice and not the advance users. Being that this is going to be used by people that need to know kernel events, I'd suspect that you'll have more advance users that novices once this takes off. -- Steve
diff --git a/Documentation/libtracefs-utils.txt b/Documentation/libtracefs-utils.txt index 41544ab..0935199 100644 --- a/Documentation/libtracefs-utils.txt +++ b/Documentation/libtracefs-utils.txt @@ -3,7 +3,8 @@ libtracefs(3) NAME ---- -tracefs_tracers, tracefs_get_clock, tracefs_list_free - +tracefs_tracers, tracefs_get_clock, tracefs_list_free, +tracefs_trace_marker_get_fd - Helper functions for working with trace file system. SYNOPSIS @@ -15,6 +16,7 @@ SYNOPSIS char pass:[*]pass:[*]*tracefs_tracers*(const char pass:[*]_tracing_dir_); char pass:[*]*tracefs_get_clock*(struct tracefs_instance pass:[*]_instance_); void *tracefs_list_free*(char pass:[*]pass:[*]_list_); +int *tracefs_trace_marker_get_fd*(struct tracefs_instance pass:[*]_instance_); -- DESCRIPTION @@ -36,6 +38,10 @@ The _tracefs_list_free()_ function frees an array of strings, returned by _tracefs_event_systems()_, _tracefs_system_events()_ and _tracefs_tracers()_ APIs. +The _tracefs_trace_marker_get_fd()_ function returns a file deascriptor to the "trace_marker" file +from the given _instance_. If _instance_ is NULL, the top trace instance is used. The returned +descriptor can be used for writing trace markers in the trace buffer of the instance. + RETURN VALUE ------------ The _tracefs_tracers()_ returns array of strings. The last element in that @@ -45,6 +51,10 @@ In case of an error, NULL is returned. The _tracefs_get_clock()_ returns string, that must be freed with free(), or NULL in case of an error. +The _tracefs_trace_marker_get_fd()_ function returns a file descriptor to "trace_marker" +file for reading and writing, which must be closed wuth close(). In case of an error -1 is returned. + + EXAMPLE ------- [source,c] @@ -66,6 +76,14 @@ char *clock = tracefs_get_clock(NULL); ... free(clock); } +... +int marker_fd = tracefs_trace_marker_get_fd(NULL); +char *my_marker = "User tarce event"; + if (marker_fd < 0) { + /* Failed to open marker file */ + } + write(marker_fd, my_marker, strlen(my_marker)); + close(marker_fd); -- FILES ----- diff --git a/include/tracefs.h b/include/tracefs.h index f3eec62..dff365a 100644 --- a/include/tracefs.h +++ b/include/tracefs.h @@ -63,6 +63,19 @@ static inline int tracefs_trace_on_get_fd(struct tracefs_instance *instance) return tracefs_instance_file_open(instance, "tracing_on", O_RDWR); } +/** + * tracefs_trace_marker_get_fd - Get a file descriptor of "trace_marker" in + * given instance + * @instance: ftrace instance, can be NULL for the top instance + * + * Returns -1 in case of an error, or a valid file descriptor to "trace_marker" + * file for reading and writing. The returned FD must be closed with close(). + */ +static inline int tracefs_trace_marker_get_fd(struct tracefs_instance *instance) +{ + return tracefs_instance_file_open(instance, "trace_marker", O_RDWR); +} + /* events */ void tracefs_list_free(char **list); char **tracefs_event_systems(const char *tracing_dir);
Added new API for opening trace_marker file of given instance: tracefs_trace_marker_get_fd(); Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> --- Documentation/libtracefs-utils.txt | 20 +++++++++++++++++++- include/tracefs.h | 13 +++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-)