Re: [PATCH] trace-cmd: Stop recording when processes traced with -P exit

Re: [PATCH] trace-cmd: Stop recording when processes traced with -P exit

[Steven Rostedt]

On Sat, 18 Apr 2020 11:36:05 -0500
Bijan Tabatabai <bijan311@yahoo.com> wrote:

> When the -F flag is used in trace-cmd record, trace-cmd stops recording
> when the executable it is tracing terminates.
> This patch makes the -P flag act similarly.

I don't mind the idea, but I'm worried about this approach.

> 
> Signed-off-by: Bijan Tabatabai <bijan311@yahoo.com>
> ---
>  tracecmd/trace-record.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index d89b32d..b5cd4c4 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -121,8 +121,10 @@ struct filter_pids {
>  };
>  
>  static struct filter_pids *filter_pids;
> +static struct filter_pids *process_pids;
>  static int nr_filter_pids;
>  static int len_filter_pids;
> +static int nr_process_pids = 0;
>  
>  static int have_set_event_pid;
>  static int have_event_fork;
> @@ -5799,7 +5801,9 @@ static void parse_record_options(int argc,
>  			while (pid) {
>  				add_filter_pid(atoi(pid), 0);
>  				pid = strtok_r(NULL, ",", &sav);
> +				nr_process_pids++;
>  			}
> +			process_pids = filter_pids;
>  			free(pids);
>  			break;
>  		case 'c':
> @@ -6273,10 +6277,19 @@ static void record_trace(int argc, char **argv,
>  					ptrace_attach(pid->pid);
>  			}
>  		}
> -		/* sleep till we are woken with Ctrl^C */
> -		printf("Hit Ctrl^C to stop recording\n");
> -		while (!finished)
> -			trace_or_sleep(type);
> +		if (nr_process_pids) {
> +			for (pid = process_pids; pid && nr_process_pids; pid = pid->next) {
> +				if (!pid->exclude)
> +					/* Wait for the process to end */
> +					while(!kill(pid->pid, 0));

Won't that while loop just go into a spin?

What about something like a fsnotify on a /proc/<pid>/ file?

I rather have trace-cmd sleep and wake up when a task exits, than to do a
spin. Like it does for the -F option.

Thanks!

-- Steve


> +				nr_process_pids--;
> +			}
> +		} else {
> +			/* sleep till we are woken with Ctrl^C */
> +			printf("Hit Ctrl^C to stop recording\n");
> +			while (!finished)
> +				trace_or_sleep(type);
> +		}
>  	}
>  
>  	tell_guests_to_stop();

Re: [PATCH] trace-cmd: Stop recording when processes traced with -P exit

[bijan tabatabai]

Hi Steve,

> Won't that while loop just go into a spin?
>
> What about something like a fsnotify on a /proc/<pid>/ file?
>
> I rather have trace-cmd sleep and wake up when a task exits, than to do a
> spin. Like it does for the -F option.

I just tried doing this using the inotify API and hit a snag.
It turns out inotify can't monitor pseudo-filesystems like /proc, so I
don't think that suggestion will work.

What if I put a call to sleep in the loop?
It's not the most elegant solution, but that way it wouldn't be too
different from the current code that sleeps for 10 seconds after
checking if finished is set to true.

Thanks,
Bijan

Re: [PATCH] trace-cmd: Stop recording when processes traced with -P exit

[Steven Rostedt]

On Mon, 20 Apr 2020 21:01:00 -0500
bijan tabatabai <bijan311@gmail.com> wrote:

> Hi Steve,
> 
> > Won't that while loop just go into a spin?
> >
> > What about something like a fsnotify on a /proc/<pid>/ file?
> >
> > I rather have trace-cmd sleep and wake up when a task exits, than to do a
> > spin. Like it does for the -F option.  
> 
> I just tried doing this using the inotify API and hit a snag.
> It turns out inotify can't monitor pseudo-filesystems like /proc, so I
> don't think that suggestion will work.
> 
> What if I put a call to sleep in the loop?
> It's not the most elegant solution, but that way it wouldn't be too
> different from the current code that sleeps for 10 seconds after
> checking if finished is set to true.

Perhaps using the new pidfd interface may work here.

  https://lwn.net/Articles/794707/

It was added in 5.4 (which is still rather new), but this would be a
new trace-cmd feature as well.

-- Steve

Re: [PATCH] trace-cmd: Stop recording when processes traced with -P exit

[bijan tabatabai]

On Mon, Apr 20, 2020 at 9:31 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> > Hi Steve,
> >
> > > Won't that while loop just go into a spin?
> > >
> > > What about something like a fsnotify on a /proc/<pid>/ file?
> > >
> > > I rather have trace-cmd sleep and wake up when a task exits, than to do a
> > > spin. Like it does for the -F option.
> >
> > I just tried doing this using the inotify API and hit a snag.
> > It turns out inotify can't monitor pseudo-filesystems like /proc, so I
> > don't think that suggestion will work.
> >
> > What if I put a call to sleep in the loop?
> > It's not the most elegant solution, but that way it wouldn't be too
> > different from the current code that sleeps for 10 seconds after
> > checking if finished is set to true.
>
> Perhaps using the new pidfd interface may work here.
>
>   https://lwn.net/Articles/794707/
>
> It was added in 5.4 (which is still rather new), but this would be a
> new trace-cmd feature as well.
>
> -- Steve
>
This is a stupid question, but what would be the best way to use the
pidfd interface and also keep backwards compatibility with older
kernel versions?
Or is that not really a concern?

Bijan

Re: [PATCH] trace-cmd: Stop recording when processes traced with -P exit

[Steven Rostedt]

On Mon, 20 Apr 2020 22:16:09 -0500
bijan tabatabai <bijan311@gmail.com> wrote:

> > Perhaps using the new pidfd interface may work here.
> >
> >   https://lwn.net/Articles/794707/
> >
> > It was added in 5.4 (which is still rather new), but this would be a
> > new trace-cmd feature as well.
> >
> > -- Steve
> >  
> This is a stupid question, but what would be the best way to use the
> pidfd interface and also keep backwards compatibility with older
> kernel versions?
> Or is that not really a concern?

Not a stupid question at all.

The way to check is to call the system call and see if it works. If it
doesn't simply fall back to the original behavior (to have the user do the
Ctrl^C). A system call should error (errno) with -ENOSYS, if the kernel
does not support the system call.

As I stated, the pidfd is a new feature, but so is stopping trace-cmd when
the pids end. This wouldn't be a regression. It is just an enhancement if
the kernel supports it, which there's lots of other cases in trace-cmd that
does the same thing (add features depending on support of the kernel).

-- Steve