[PATCH] libtracefs: Add new API for open trace marker file

[PATCH] libtracefs: Add new API for open trace marker file

[Tzvetomir Stoyanov (VMware)]

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

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);
-- 
2.29.2

Re: [PATCH] libtracefs: Add new API for open trace marker file

[Steven Rostedt]

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

Re: [PATCH] libtracefs: Add new API for open trace marker file

[Steven Rostedt]

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

Re: [PATCH] libtracefs: Add new API for open trace marker file

[Tzvetomir Stoyanov]

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



-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

Re: [PATCH] libtracefs: Add new API for open trace marker file

[Steven Rostedt]

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

Re: [PATCH] libtracefs: Add new API for open trace marker file

[Steven Rostedt]

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

> +}
> +