linux-trace-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Yordan Karadzhov <y.karadz@gmail.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-trace-devel@vger.kernel.org
Subject: Re: [PATCH v2] libtracefs: Add APIs for data streaming
Date: Wed, 23 Jun 2021 16:23:34 +0300	[thread overview]
Message-ID: <1c038eff-954a-ac8f-8f1b-bcff439f2d41@gmail.com> (raw)
In-Reply-To: <20210623091951.7eed6b50@gandalf.local.home>



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
> 

      reply	other threads:[~2021-06-23 13:23 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
2021-06-23 13:23       ` Yordan Karadzhov [this message]

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=1c038eff-954a-ac8f-8f1b-bcff439f2d41@gmail.com \
    --to=y.karadz@gmail.com \
    --cc=linux-trace-devel@vger.kernel.org \
    --cc=rostedt@goodmis.org \
    --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
on how to clone and mirror all data and code used for this inbox