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

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

On 23.06.21 г. 16:19, Steven Rostedt wrote:
> On Wed, 23 Jun 2021 16:02:00 +0300
> Yordan Karadzhov <y.karadz@gmail.com> wrote:
> 
>>> 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.
> 
> If anything then, make it the first thing that gets done, and not just
> before the loop.
> 
> That is:
> 
> 
> +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;
> +	const char *file = "trace_pipe";
> +	int brass[2], in_fd, ret = -1;
> +	off_t data_size;
> 
> 	*keep_going = true;
> 
> +
> +	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;
> 
> And not here.
> 
> +	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;
> +	}
> 
> 
> And I think we need to make it volatile, otherwise the compiler is free to
> ignore it. Because the compiler does not need to know about threads.
> 
> 	*keep_going = true;
> 	while (*keep_going) {
> 		[ do something ]
> 	}
> 
> To the compiler that is the same as:
> 
> 	while (1) {
> 		[ do something ]
> 	}
> 
> And is free to make that change when optimizing.
> 
> What needs to be done is:
> 
> 	(*(volatile bool *)keep_going) = true;
> 
> and
> 
> 	while (*(volatile bool *)keep_going) {
> 
> That way the compiler knows that the value can change from outside its
> knowledge.
> 

OK, I will make the changes and will send v3.

Thanks a lot!
Yordan

> -- Steve
>