linux-trace-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: Yordan Karadzhov <y.karadz@gmail.com>
Cc: linux-trace-devel@vger.kernel.org
Subject: Re: [PATCH v2] libtracefs: Add APIs for data streaming
Date: Wed, 23 Jun 2021 09:19:51 -0400	[thread overview]
Message-ID: <20210623091951.7eed6b50@gandalf.local.home> (raw)
In-Reply-To: <481ebd67-050b-06bf-bcce-ae0febd79ab5@gmail.com>

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

  reply	other threads:[~2021-06-23 13:19 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-23 12:05 Yordan Karadzhov (VMware)
2021-06-23 12:45 ` Steven Rostedt
2021-06-23 13:02   ` Yordan Karadzhov
2021-06-23 13:19     ` Steven Rostedt [this message]
2021-06-23 13:23       ` Yordan Karadzhov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210623091951.7eed6b50@gandalf.local.home \
    --to=rostedt@goodmis.org \
    --cc=linux-trace-devel@vger.kernel.org \
    --cc=y.karadz@gmail.com \
    --subject='Re: [PATCH v2] libtracefs: Add APIs for data streaming' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).