Re: [PATCH v2] libtracefs: Add APIs for data streaming

[Yordan Karadzhov <y.karadz@gmail.com>]

On 23.06.21 г. 15:45, Steven Rostedt wrote:
> On Wed, 23 Jun 2021 15:05:26 +0300
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>> The new APIs can be used to dump the content of "trace_pipe" into a
>> file or directly to "stdout". The "splice" system call is used to
>> moves the data without copying. The new functionality is essentially
>> identical to what 'trace-cmd show -p' does.
>>
>> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
>> ---
>>
>> Changes in v2:
>>    - The setting of the signal disposition is removed from the library
>>      and is now responsibility of the caller. This in fact gives more
>>      options for the user.
>>    - The example provided in the documentation is updated accordingly.
>>
>>   Documentation/libtracefs-stream.txt | 106 ++++++++++++++++++++++++++++
>>   include/tracefs-local.h             |   1 +
>>   include/tracefs.h                   |   5 ++
>>   src/tracefs-tools.c                 |  98 +++++++++++++++++++++++++
>>   4 files changed, 210 insertions(+)
>>   create mode 100644 Documentation/libtracefs-stream.txt
>>
>> diff --git a/Documentation/libtracefs-stream.txt b/Documentation/libtracefs-stream.txt
>> new file mode 100644
>> index 0000000..24c2e47
>> --- /dev/null
>> +++ b/Documentation/libtracefs-stream.txt
>> @@ -0,0 +1,106 @@
>> +libtracefs(3)
>> +=============
>> +
>> +NAME
>> +----
>> +tracefs_trace_pipe_stream, tracefs_trace_pipe_print -
>> +redirect the stream of trace data to an output or stdout.
>> +
>> +SYNOPSIS
>> +--------
>> +[verse]
>> +--
>> +*#include <tracefs.h>*
>> +
>> +int tracefs_trace_pipe_stream(int fd, struct tracefs_instance *instance, int flags);
>> +int tracefs_trace_pipe_print(struct tracefs_instance *instance);
>> +
>> +--
>> +
>> +DESCRIPTION
>> +-----------
>> +If NULL is passed as _instance_, the top trace instance is used.
>> +The user can interrupt the streaming of the data by pressing Ctrl-c.
>> +
>> +The _tracefs_trace_pipe_stream()_ function redirects the stream of trace data to an output
>> +file. The "splice" system call is used to moves the data without copying between kernel
>> +address space and user address space. The _fd_ is the file descriptor of the output file
>> +and _flags_ is a bit mask of flags to be passed to the "splice" system call.
>> +
>> +The _tracefs_trace_pipe_print()_ function is similar to _tracefs_trace_pipe_stream()_, but
>> +the stream of trace data is redirected to stdout.
>> +
>> +
>> +RETURN VALUE
>> +------------
>> +The _tracefs_trace_pipe_stream()_, and _tracefs_trace_pipe_print()_ functions return 0 if the operation is
>> +successful, or -1 in case of an error.
>> +
>> +EXAMPLE
>> +-------
>> +[source,c]
>> +--
>> +#include <unistd.h>
>> +#include <signal.h>
>> +
>> +#include <tracefs.h>
>> +
>> +void stop(int sig)
>> +{
>> +	tracefs_trace_pipe_stop(NULL);
>> +}
>> +
>> +int main()
>> +{
>> +	mode_t mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH;
>> +	const char *filename = "trace.txt";
>> +	int fd = creat(filename, mode);
>> +	int ret;
>> +
>> +	signal(SIGINT, stop);
>> +	ret = tracefs_trace_pipe_stream(fd, NULL, 0);
>> +	close(fd);
>> +
>> +	return ret;
>> +}
>> +--
>> +FILES
>> +-----
>> +[verse]
>> +--
>> +*tracefs.h*
>> +	Header file to include in order to have access to the library APIs.
>> +*-ltracefs*
>> +	Linker switch to add when building a program that uses the library.
>> +--
>> +
>> +SEE ALSO
>> +--------
>> +_libtracefs(3)_,
>> +_libtraceevent(3)_,
>> +_trace-cmd(1)_,
>> +Documentation/trace/ftrace.rst from the Linux kernel tree
>> +
>> +AUTHOR
>> +------
>> +[verse]
>> +--
>> +*Steven Rostedt* <rostedt@goodmis.org>
>> +*Tzvetomir Stoyanov* <tz.stoyanov@gmail.com>
>> +--
>> +REPORTING BUGS
>> +--------------
>> +Report bugs to  <linux-trace-devel@vger.kernel.org>
>> +
>> +LICENSE
>> +-------
>> +libtracefs is Free Software licensed under the GNU LGPL 2.1
>> +
>> +RESOURCES
>> +---------
>> +https://git.kernel.org/pub/scm/libs/libtrace/libtracefs.git/
>> +
>> +COPYING
>> +-------
>> +Copyright \(C) 2021 VMware, Inc. Free use of this software is granted under
>> +the terms of the GNU Public License (GPL).
>> diff --git a/include/tracefs-local.h b/include/tracefs-local.h
>> index 9e3aa18..833f136 100644
>> --- a/include/tracefs-local.h
>> +++ b/include/tracefs-local.h
>> @@ -30,6 +30,7 @@ struct tracefs_instance {
>>   	int				ftrace_notrace_fd;
>>   	int				ftrace_marker_fd;
>>   	int				ftrace_marker_raw_fd;
>> +	int				pipe_keep_going;
>>   };
>>   
>>   extern pthread_mutex_t toplevel_lock;
>> diff --git a/include/tracefs.h b/include/tracefs.h
>> index e29b550..3c6741b 100644
>> --- a/include/tracefs.h
>> +++ b/include/tracefs.h
>> @@ -184,4 +184,9 @@ int tracefs_function_notrace(struct tracefs_instance *instance, const char *filt
>>   /* Control library logs */
>>   void tracefs_set_loglevel(enum tep_loglevel level);
>>   
>> +int tracefs_trace_pipe_stream(int fd, struct tracefs_instance *instance, int flags);
>> +int tracefs_trace_pipe_print(struct tracefs_instance *instance);
>> +void tracefs_trace_pipe_stop(struct tracefs_instance *instance);
>> +
>> +
>>   #endif /* _TRACE_FS_H */
>> diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c
>> index 0cbb56d..0c0c44f 100644
>> --- a/src/tracefs-tools.c
>> +++ b/src/tracefs-tools.c
>> @@ -912,3 +912,101 @@ int tracefs_function_notrace(struct tracefs_instance *instance, const char *filt
>>   	tracefs_put_tracing_file(filter_path);
>>   	return ret;
>>   }
>> +
>> +static int top_pipe_keep_going;
>> +
>> +/**
>> + * tracefs_trace_pipe_stream - redirect the stream of trace data to an output
>> + * file. The "splice" system call is used to moves the data without copying
>> + * between kernel address space and user address space. The user can interrupt
>> + * the streaming of the data by pressing Ctrl-c.
>> + * @fd: The file descriptor of the output file.
>> + * @instance: ftrace instance, can be NULL for top tracing instance.
>> + * @flags: flags to be passed to the "splice" system call.
>> + *
>> + * Returns -1 in case of an error or 0 otherwise.
>> + */
>> +int tracefs_trace_pipe_stream(int fd, struct tracefs_instance *instance,
>> +			      int flags)
>> +{
>> +	int *keep_going = instance ? &instance->pipe_keep_going :
>> +				     &top_pipe_keep_going;
> 
> I'm thinking we should invert the above. That is, have a stop instead of
> "keep_going", where it is false by default (when the instance is created).
> 
> That's because, if we start here, and the SIGINT comes in before we get
> here, it keep_going might get set to false, and missed.

I intentionally tried to avoid having any dependency of the initialization value of the "keep_going" flag. We have to 
consider the case when the user creates one instance and then calls this function multiple times in a row.


> 
>> +	const char *file = "trace_pipe";
>> +	int brass[2], in_fd, ret = -1;
>> +	off_t data_size;
>> +
>> +	in_fd = tracefs_instance_file_open(instance, file, O_RDONLY);
>> +	if (in_fd < 0) {
>> +		tracefs_warning("Failed to open 'trace_pipe'.");
>> +		return ret;
>> +	}
>> +
>> +	if(pipe(brass) < 0) {
>> +		tracefs_warning("Failed to open pipe.");
>> +		goto close_file;
>> +	}
>> +
>> +	data_size = fcntl(brass[0], F_GETPIPE_SZ);
>> +	if (data_size <= 0) {
>> +		tracefs_warning("Failed to open pipe (size=0).");
>> +		goto close_all;
>> +	}
>> +
>> +	*keep_going = true;
> 
> Then we get here and we set it back to true.
> 
> Perhaps we should have a:
> 
> 	instance->pipe_stop
> 
> flag that gets set by the user.
> 
> And instead of setting the variable here, add a,
> 
>     tracefs_trace_pipe_start(instance)
> 
> to match the tracefs_trace_pipe_stop().
> 
> If the user stops it, we should make it so that they need to start it again.

I do not understand your point with the "start" function. tracefs_trace_pipe_stream() itself is the start.

Thanks!
Yordan


> 
> -- Steve
> 
> 
>> +	while (*keep_going) {
>> +		ret = splice(in_fd, NULL,
>> +			     brass[1], NULL,
>> +			     data_size, flags);
>> +		if (ret < 0)
>> +			break;
>> +
>> +		ret = splice(brass[0], NULL,
>> +			     fd, NULL,
>> +			     data_size, flags);
>> +		if (ret < 0)
>> +			break;
>> +	}
>> +
>> +	/*
>> +	 * Do not return error in the case when the "splice" system call
>> +	 * was interrupted by the user (pressing Ctrl-c).
>> +	 */
>> +	if (!keep_going)
>> +		ret = 0;
>> +
>> + close_all:
>> +	close(brass[0]);
>> +	close(brass[1]);
>> + close_file:
>> +	close(in_fd);
>> +
>> +	return ret;
>> +}
>> +
>> +/**
>> + * tracefs_trace_pipe_print - redirect the stream of trace data to "stdout".
>> + * The "splice" system call is used to moves the data without copying
>> + * between kernel address space and user address space.
>> + * @instance: ftrace instance, can be NULL for top tracing instance.
>> + *
>> + * Returns -1 in case of an error or 0 otherwise.
>> + */
>> +
>> +int tracefs_trace_pipe_print(struct tracefs_instance *instance)
>> +{
>> +   return tracefs_trace_pipe_stream(STDOUT_FILENO,
>> +				    instance,
>> +				    SPLICE_F_MORE | SPLICE_F_MOVE);
>> +}
>> +
>> +/**
>> + * tracefs_trace_pipe_stop - stop the streaming of trace data.
>> + * @instance: ftrace instance, can be NULL for top tracing instance.
>> + */
>> +void tracefs_trace_pipe_stop(struct tracefs_instance *instance)
>> +{
>> +	if (instance)
>> +		instance->pipe_keep_going = false;
>> +	else
>> +		top_pipe_keep_going = false;
>> +}
>