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

[Steven Rostedt <rostedt@goodmis.org>]

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.

-- Steve