[PATCH 0/3] trace-cmd record: Support daemonization after recording starts

[PATCH 0/3] trace-cmd record: Support daemonization after recording starts

[avidanborisov]

From: Avidan Borisov <avidanborisov@gmail.com>

This patch series adds support for daemonizing trace-cmd record after tracing
has started. The motivation is to be able to use trace-cmd more easily
in automated setups.

The main reason to do this in trace-cmd itself, is that it allows us to do the daemonization
at the latest point possible. This has few advantages over daemonizing trace-cmd as a whole:

  1. We can cleanly fail if something failed during initialization, and signal
     it up to the caller normally with a status code. This is not possible if we
     daemonize trace-cmd as a whole, since the the error could happen after the
     daemonization, but at that point trace-cmd is already a background process,
     and it's cumbersome to check if it failed or not.

  2. The next thing that executes after trace-cmd exits will definitely be recorded
     in the trace, since we exit to the caller only after tracing has started. This
     is especially important when we trace guests as well, since the KVM/PTP time synchronization
     can take a while to complete, and we can't know for sure when the actual tracing
     starts. With --daemonize we know that once trace-cmd exits, the tracing has started.

Here's an example how naive daemonization fails:

$ sudo setsid trace-cmd record -p nop -e 'net:sched_process_exec' -A guest -p nop -e net & \
    > ping -c 1 10.20.1.2 &&
    > sudo kill -SIGINT $! &&
    > sudo trace-cmd report -i trace.dat -i trace-guest.dat | head
[1] 3087529
PING 10.20.1.2 (10.20.1.2) 56(84) bytes of data.
64 bytes from 10.20.1.2: icmp_seq=1 ttl=64 time=0.233 ms
--- 10.20.1.2 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 0.233/0.233/0.233/0.000 ms
[1]+  Interrupt               sudo setsid trace-cmd record -p nop -e 'net:sched_process_exec' -A guest -p nop -e net
trace-cmd: No such file or directory
  opening 'trace.dat'

Here we have stopped trace-cmd too early, and no trace was generated. Now with --daemonize:

$ sudo trace-cmd record --daemonize -p nop -e 'sched:sched_process_exec' -A guest -p nop -e net &&
    > ping -c 1 10.20.1.2 &&
    > sudo start-stop-daemon --stop --signal INT --retry 20 --pidfile /var/run/trace-cmd-record.pid &&
    > sudo trace-cmd report -i trace.dat -i trace-guest.dat | head
Negotiated kvm time sync protocol with guest guest
Send SIGINT to pid 3071371 to stop recording
PING 10.20.1.2 (10.20.1.2) 56(84) bytes of data.
64 bytes from 10.20.1.2: icmp_seq=1 ttl=64 time=0.134 ms
--- 10.20.1.2 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 0.134/0.134/0.134/0.000 ms
CPU0 data recorded at offset=0x14f000
    229 bytes in size (4096 uncompressed)
....
      trace.dat: cpus=28
trace-guest.dat: cpus=1
      trace.dat:           ping-3071450 [013] 1196830.834258: sched_process_exec:     filename=/bin/ping pid=3071450 old_pid=3071450
trace-guest.dat:           <idle>-0     [000] 1196830.835990: napi_gro_receive_entry: dev=eth1 napi_id=0x2002 queue_mapping=1 skbaddr=0xffff95d051a5c400 vlan_tagged=0 vlan_proto=0x0000 vlan_tci=0x0000 protocol=0x0800 ip_summed=0 hash=0x00000000 l4_hash=0 len=84 data_len=0 truesize=768 mac_header_valid=1 mac_header=-14 nr_frags=0 gso_size=0 gso_type=0
trace-guest.dat:           <idle>-0     [000] 1196830.835997: napi_gro_receive_exit:  ret=3
trace-guest.dat:           <idle>-0     [000] 1196830.835998: netif_receive_skb:      dev=eth1 skbaddr=0xffff95d051a5c400x len=84
trace-guest.dat:           <idle>-0     [000] 1196830.836021: net_dev_queue:          dev=eth1 skbaddr=0xffff95d051a5c700x len=98
trace-guest.dat:           <idle>-0     [000] 1196830.836024: net_dev_start_xmit:     dev=eth1 queue_mapping=0 skbaddr=0xffff95d051a5c700 vlan_tagged=0 vlan_proto=0x0000 vlan_tci=0x0000 protocol=0x0800 ip_summed=0 len=98 data_len=0 network_offset=14 transport_offset_valid=1 transport_offset=34 tx_flags=0 gso_size=0 gso_segs=0 gso_type=0
trace-guest.dat:           <idle>-0     [000] 1196830.836069: net_dev_xmit:           dev=eth1 skbaddr=0xffff95d051a5c700 len=98 rc=0
      trace.dat:           sudo-3071451 [015] 1196830.838262: sched_process_exec:     filename=/usr/bin/sudo pid=3071451 old_pid=3071451

As for the implementation itself - unfortunately since daemon(3) calls fork(), we can't call it after
creating other processes/threads (if we call it after creating the recorder processes it'll mess up the
process hierarchy, and if after the tsync thread creation, it will mess up memory sharing).
However the point in time where we want to exit to the caller is after those processes/threads
have started, since that's when tracing actually starts.

Therefore, we implement the daemonization ourselves in the following way:
    1. We fork() early, and make the parent waitpid on the child.
    2. When the child finishes initializing everything, it detaches
       from the session and sends a signal to the parent.
    3. The parent can now exit, causing the entire process hierarchy
       of the child to be inherited by init.
    4. The daemonization is now complete, and the caller to trace-cmd
       now executes the next thing, which will be traced.

Avidan Borisov (3):
  trace-cmd record: Add --daemonize
  trace-cmd: export pidfile functions from trace-listen.c
  trace-cmd record: Create a pidfile when using --daemonize

 .../trace-cmd/trace-cmd-record.1.txt          |   4 +
 tracecmd/include/trace-local.h                |   4 +
 tracecmd/trace-listen.c                       |  32 ++--
 tracecmd/trace-record.c                       | 141 +++++++++++++++++-
 tracecmd/trace-usage.c                        |   3 +
 5 files changed, 164 insertions(+), 20 deletions(-)

-- 
2.25.1

[PATCH 1/3] trace-cmd record: Add --daemonize

[avidanborisov]

From: Avidan Borisov <avidanborisov@gmail.com>

Add a --daemonize flag to trace-cmd record which allows the user to record
in the background, after tracing has completely initialized.

Signed-off-by: Avidan Borisov <avidanborisov@gmail.com>
---
 .../trace-cmd/trace-cmd-record.1.txt          |   3 +
 tracecmd/trace-record.c                       | 128 +++++++++++++++++-
 tracecmd/trace-usage.c                        |   1 +
 3 files changed, 130 insertions(+), 2 deletions(-)

diff --git a/Documentation/trace-cmd/trace-cmd-record.1.txt b/Documentation/trace-cmd/trace-cmd-record.1.txt
index b709e48..366ab46 100644
--- a/Documentation/trace-cmd/trace-cmd-record.1.txt
+++ b/Documentation/trace-cmd/trace-cmd-record.1.txt
@@ -418,6 +418,9 @@ OPTIONS
     run on a privileged guest that the host is aware of (as denoted by the
     'cid' in the *-P* option for the agent).
 
+*--daemonize*
+    Run trace-cmd in the background as a daemon after recording has started.
+
 EXAMPLES
 --------
 
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 7f0cebe..0eee8dd 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -91,6 +91,8 @@ static bool quiet;
 
 static bool fork_process;
 
+static bool do_daemonize;
+
 /* Max size to let a per cpu file get */
 static int max_kb;
 
@@ -1636,6 +1638,109 @@ static inline void ptrace_attach(struct buffer_instance *instance, int pid) { }
 
 #endif /* NO_PTRACE */
 
+static bool child_detached;
+
+static void daemonize_set_child_detached(int s)
+{
+	child_detached = true;
+}
+
+static void daemonize_start(void)
+{
+	int devnull;
+	int status;
+	int pid;
+	int rc;
+
+	pid = fork();
+	if (pid == -1)
+		die("daemonize: fork failed");
+
+	if (pid == 0) { /* child */
+		/*
+		 * We keep stdout and stderr open to allow the user to
+		 * see output and errors after the daemonization (he can
+		 * choose to supress it with >/dev/null if he wants).
+		 *
+		 * No reason to keep stdin open (it might interfere with the
+		 * shell), we redirect it to /dev/null.
+		 */
+		devnull = open("/dev/null", O_RDONLY);
+		if (devnull == -1)
+			die("daemonize: open /dev/null failed");
+
+		if (devnull > 0) {
+			if (dup2(devnull, 0) == -1)
+				die("daemonize: dup2");
+			close(0);
+		}
+
+		return;
+
+		/*
+		 * The child returns to back to the caller, but the parent waits until
+		 * SIGRTMIN is received from the child (by calling daemonize_finish()),
+		 * or the child exits for some reason (usually an indication of
+		 * an error), which ever comes first.
+		 *
+		 * Then the parent exits (with the status code of the child,
+		 * if it finished early, or with 0 if SIGRTMIN was received),
+		 * which causes the child (and its entire process tree) to be
+		 * inherited by init.
+		 *
+		 * Note that until the child calls daemonize_finish(), it still has
+		 * the same session id as the parent, so it can die together with
+		 * the parent before daemonization finished (purposefully, since the
+		 * user might send a quick Ctrl^C to cancel the command, and we don't
+		 * want background processes staying alive in that case)
+		 */
+	} else { /* parent */
+		struct sigaction sa = {
+			/* disable SA_RESTART, to allow waitpid() to be interrupted by SIGRTMIN */
+			.sa_flags = 0,
+			.sa_handler = daemonize_set_child_detached
+		};
+
+		if (sigemptyset(&sa.sa_mask) == -1)
+			die("daemonize: sigemptyset failed");
+
+		if (sigaddset(&sa.sa_mask, SIGRTMIN) == -1)
+			die("daemonize: sigaddset failed");
+
+		if (sigprocmask(SIG_UNBLOCK, &sa.sa_mask, NULL) == -1)
+			die("daemonize: sigprocmask failed");
+
+		if (sigaction(SIGRTMIN, &sa, NULL) == -1)
+			die("daemonize: sigaction failed");
+
+		do {
+			rc = waitpid(pid, &status, 0);
+		} while (!child_detached && ((rc == -1) && (errno == EINTR)));
+
+		if (child_detached)
+			exit(0);
+		else if (rc == pid)
+			exit(WIFEXITED(status));
+		else
+			die("daemonize: waitpid failed");
+
+		__builtin_unreachable();
+	}
+}
+
+static void daemonize_finish(void)
+{
+	/*
+	 * setsid() will also set the sid to be the pgid to all currently
+	 * running threads in the process group (such as the tsync thread).
+	 */
+	if (setsid() == -1)
+		die("daemonize: setsid");
+
+	if (kill(getppid(), SIGRTMIN) == -1)
+		die("daemonize: kill");
+}
+
 static void trace_or_sleep(enum trace_type type, bool pwait)
 {
 	int i;
@@ -1716,6 +1821,9 @@ static void run_cmd(enum trace_type type, const char *user, int argc, char **arg
 			die("Failed to exec %s", argv[0]);
 		}
 	}
+
+	if (do_daemonize)
+		daemonize_finish();
 	if (fork_process)
 		exit(0);
 	if (do_ptrace) {
@@ -5757,6 +5865,7 @@ enum {
 	OPT_proxy		= 261,
 	OPT_temp		= 262,
 	OPT_notimeout		= 264,
+	OPT_daemonize		= 265,
 };
 
 void trace_stop(int argc, char **argv)
@@ -6183,6 +6292,7 @@ static void parse_record_options(int argc,
 			{"file-version", required_argument, NULL, OPT_file_ver},
 			{"proxy", required_argument, NULL, OPT_proxy},
 			{"temp", required_argument, NULL, OPT_temp},
+			{"daemonize", no_argument, NULL, OPT_daemonize},
 			{NULL, 0, NULL, 0}
 		};
 
@@ -6656,6 +6766,10 @@ static void parse_record_options(int argc,
 				die("--fork option used for 'start' command only");
 			fork_process = true;
 			break;
+		case OPT_daemonize:
+			if (!IS_RECORD(ctx))
+				die("--daemonize option used for 'record' command only");
+			do_daemonize = true;
 		case OPT_tsc2nsec:
 			ret = get_tsc_nsec(&ctx->tsc2nsec.shift,
 					   &ctx->tsc2nsec.mult);
@@ -6899,6 +7013,9 @@ static void record_trace(int argc, char **argv,
 	struct buffer_instance *instance;
 	struct filter_pids *pid;
 
+	if (do_daemonize)
+		daemonize_start();
+
 	/*
 	 * If top_instance doesn't have any plugins or events, then
 	 * remove it from being processed.
@@ -7017,8 +7134,15 @@ static void record_trace(int argc, char **argv,
 				}
 			}
 		}
-		/* sleep till we are woken with Ctrl^C */
-		printf("Hit Ctrl^C to stop recording\n");
+
+		if (do_daemonize) {
+			daemonize_finish();
+			printf("Send SIGINT to pid %d to stop recording\n", getpid());
+		} else {
+			/* sleep till we are woken with Ctrl^C */
+			printf("Hit Ctrl^C to stop recording\n");
+		}
+
 		for_all_instances(instance) {
 			/* If an instance is not tracing individual processes
 			 * or there is an error while waiting for a process to
diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
index 42a8e7d..0630e8e 100644
--- a/tracecmd/trace-usage.c
+++ b/tracecmd/trace-usage.c
@@ -81,6 +81,7 @@ static struct usage_help usage_help[] = {
 		"                        available algorithms can be listed with trace-cmd list -c\n"
 		"          --proxy vsocket to reach the agent. Acts the same as -A (for an agent)\n"
 		"              but will send the proxy connection to the agent.\n"
+		"          --daemonize run trace-cmd in the background as a daemon after recording has started\n"
 	},
 	{
 		"set",
-- 
2.25.1

[PATCH 2/3] trace-cmd: export pidfile functions from trace-listen.c

[avidanborisov]

From: Avidan Borisov <avidanborisov@gmail.com>

trace-listen.c has some utility functions for creating and removing
pidfiles, to avoid code duplication we make those functions generic
and export them to the rest of the codebase.

Signed-off-by: Avidan Borisov <avidanborisov@gmail.com>
---
 tracecmd/include/trace-local.h |  4 ++++
 tracecmd/trace-listen.c        | 32 ++++++++++++++------------------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/tracecmd/include/trace-local.h b/tracecmd/include/trace-local.h
index ae208fb..73e93dc 100644
--- a/tracecmd/include/trace-local.h
+++ b/tracecmd/include/trace-local.h
@@ -457,4 +457,8 @@ static inline bool is_digits(const char *s)
 
 bool trace_tsc2nsec_is_supported(void);
 
+void make_pid_name(char *buf, const char *pidfile_basename);
+void remove_pid_file(const char *pidfile_basename);
+void make_pid_file(const char *pidfile_basename);
+
 #endif /* __TRACE_LOCAL_H */
diff --git a/tracecmd/trace-listen.c b/tracecmd/trace-listen.c
index e95c571..5894a92 100644
--- a/tracecmd/trace-listen.c
+++ b/tracecmd/trace-listen.c
@@ -34,6 +34,8 @@
 
 #define VAR_RUN_DIR		VAR_DIR_Q(VAR_DIR) "/run"
 
+#define LISTEN_PIDFILE		"trace-cmd-net.pid"
+
 static char *default_output_dir = ".";
 static char *output_dir;
 static char *default_output_file = "trace";
@@ -52,7 +54,8 @@ static bool done;
 #define pdie(fmt, ...)					\
 	do {						\
 		tracecmd_plog_error(fmt, ##__VA_ARGS__);\
-		remove_pid_file();			\
+		if (do_daemon)				\
+			remove_pid_file(LISTEN_PIDFILE);\
 		exit(-1);				\
 	} while (0)
 
@@ -126,21 +129,16 @@ static void finish(int sig)
 	done = true;
 }
 
-static void make_pid_name(int mode, char *buf)
+void make_pid_name(char *buf, const char *pidfile_basename)
 {
-	snprintf(buf, PATH_MAX, VAR_RUN_DIR "/trace-cmd-net.pid");
+	snprintf(buf, PATH_MAX, VAR_RUN_DIR "/%s", pidfile_basename);
 }
 
-static void remove_pid_file(void)
+void remove_pid_file(const char *pidfile_basename)
 {
 	char buf[PATH_MAX];
-	int mode = do_daemon;
-
-	if (!do_daemon)
-		return;
-
-	make_pid_name(mode, buf);
 
+	make_pid_name(buf, pidfile_basename);
 	unlink(buf);
 }
 
@@ -991,16 +989,12 @@ static void do_accept_loop(int sfd)
 	clean_up();
 }
 
-static void make_pid_file(void)
+void make_pid_file(const char *pidfile_basename)
 {
 	char buf[PATH_MAX];
-	int mode = do_daemon;
 	int fd;
 
-	if (!do_daemon)
-		return;
-
-	make_pid_name(mode, buf);
+	make_pid_name(buf, pidfile_basename);
 
 	fd = open(buf, O_WRONLY | O_CREAT | O_TRUNC, 0644);
 	if (fd < 0) {
@@ -1075,7 +1069,8 @@ static void do_listen(char *port)
 	if (!tracecmd_get_debug())
 		signal_setup(SIGCHLD, sigstub);
 
-	make_pid_file();
+	if (do_daemon)
+		make_pid_file(LISTEN_PIDFILE);
 
 	if (use_vsock)
 		sfd = get_vsock(port);
@@ -1090,7 +1085,8 @@ static void do_listen(char *port)
 
 	kill_clients();
 
-	remove_pid_file();
+	if (do_daemon)
+		remove_pid_file(LISTEN_PIDFILE);
 }
 
 static void start_daemon(void)
-- 
2.25.1

[PATCH 3/3] trace-cmd record: Create a pidfile when using --daemonize

[avidanborisov]

From: Avidan Borisov <avidanborisov@gmail.com>

When used with --daemonize, create a pidfile at
/var/run/trace-cmd-record.pid during the duration
of the recording, and delete it after we're done.

Signed-off-by: Avidan Borisov <avidanborisov@gmail.com>
---
 Documentation/trace-cmd/trace-cmd-record.1.txt |  1 +
 tracecmd/trace-record.c                        | 13 +++++++++++++
 tracecmd/trace-usage.c                         |  4 +++-
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/Documentation/trace-cmd/trace-cmd-record.1.txt b/Documentation/trace-cmd/trace-cmd-record.1.txt
index 366ab46..0c9a914 100644
--- a/Documentation/trace-cmd/trace-cmd-record.1.txt
+++ b/Documentation/trace-cmd/trace-cmd-record.1.txt
@@ -420,6 +420,7 @@ OPTIONS
 
 *--daemonize*
     Run trace-cmd in the background as a daemon after recording has started.
+    Creates a pidfile at /var/run/trace-cmd-record.pid with the pid of trace-cmd during the recording.
 
 EXAMPLES
 --------
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 0eee8dd..85ad808 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -46,6 +46,8 @@
 #define _STR(x) #x
 #define STR(x) _STR(x)
 
+#define RECORD_PIDFILE	"trace-cmd-record.pid"
+
 #define TRACE_CTRL	"tracing_on"
 #define TRACE		"trace"
 #define AVAILABLE	"available_tracers"
@@ -93,6 +95,8 @@ static bool fork_process;
 
 static bool do_daemonize;
 
+static bool created_pidfile;
+
 /* Max size to let a per cpu file get */
 static int max_kb;
 
@@ -648,6 +652,9 @@ void die(const char *fmt, ...)
 	else
 		ret = -1;
 
+	if (created_pidfile)
+		remove_pid_file(RECORD_PIDFILE);
+
 	kill_threads();
 	va_start(ap, fmt);
 	fprintf(stderr, "  ");
@@ -1739,6 +1746,9 @@ static void daemonize_finish(void)
 
 	if (kill(getppid(), SIGRTMIN) == -1)
 		die("daemonize: kill");
+
+	make_pid_file(RECORD_PIDFILE);
+	created_pidfile = true;
 }
 
 static void trace_or_sleep(enum trace_type type, bool pwait)
@@ -7190,6 +7200,9 @@ static void record_trace(int argc, char **argv,
 
 	destroy_stats();
 	finalize_record_trace(ctx);
+
+	if (created_pidfile)
+		remove_pid_file(RECORD_PIDFILE);
 }
 
 /*
diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
index 0630e8e..2e5d861 100644
--- a/tracecmd/trace-usage.c
+++ b/tracecmd/trace-usage.c
@@ -81,7 +81,9 @@ static struct usage_help usage_help[] = {
 		"                        available algorithms can be listed with trace-cmd list -c\n"
 		"          --proxy vsocket to reach the agent. Acts the same as -A (for an agent)\n"
 		"              but will send the proxy connection to the agent.\n"
-		"          --daemonize run trace-cmd in the background as a daemon after recording has started\n"
+		"          --daemonize run trace-cmd in the background as a daemon after recording has started.\n"
+		"                      creates a pidfile at /var/run/trace-cmd-record.pid with the pid of trace-cmd\n"
+		"                      during the recording.\n"
 	},
 	{
 		"set",
-- 
2.25.1

Re: [PATCH 0/3] trace-cmd record: Support daemonization after recording starts

[Steven Rostedt]

On Mon,  1 May 2023 23:31:15 +0300
avidanborisov@gmail.com wrote:

> From: Avidan Borisov <avidanborisov@gmail.com>

Hi Avidan,

I finally got around to looking at this. Nice work! Some nits though.

> 
> This patch series adds support for daemonizing trace-cmd record after tracing
> has started. The motivation is to be able to use trace-cmd more easily
> in automated setups.
> 
> The main reason to do this in trace-cmd itself, is that it allows us to do the daemonization
> at the latest point possible. This has few advantages over daemonizing trace-cmd as a whole:
> 
>   1. We can cleanly fail if something failed during initialization, and signal
>      it up to the caller normally with a status code. This is not possible if we
>      daemonize trace-cmd as a whole, since the the error could happen after the
>      daemonization, but at that point trace-cmd is already a background process,
>      and it's cumbersome to check if it failed or not.
> 
>   2. The next thing that executes after trace-cmd exits will definitely be recorded
>      in the trace, since we exit to the caller only after tracing has started. This
>      is especially important when we trace guests as well, since the KVM/PTP time synchronization
>      can take a while to complete, and we can't know for sure when the actual tracing
>      starts. With --daemonize we know that once trace-cmd exits, the tracing has started.
> 
> Here's an example how naive daemonization fails:
> 
> $ sudo setsid trace-cmd record -p nop -e 'net:sched_process_exec' -A guest -p nop -e net & \
>     > ping -c 1 10.20.1.2 &&
>     > sudo kill -SIGINT $! &&
>     > sudo trace-cmd report -i trace.dat -i trace-guest.dat | head  
> [1] 3087529
> PING 10.20.1.2 (10.20.1.2) 56(84) bytes of data.
> 64 bytes from 10.20.1.2: icmp_seq=1 ttl=64 time=0.233 ms
> --- 10.20.1.2 ping statistics ---
> 1 packets transmitted, 1 received, 0% packet loss, time 0ms
> rtt min/avg/max/mdev = 0.233/0.233/0.233/0.000 ms
> [1]+  Interrupt               sudo setsid trace-cmd record -p nop -e 'net:sched_process_exec' -A guest -p nop -e net
> trace-cmd: No such file or directory
>   opening 'trace.dat'
> 
> Here we have stopped trace-cmd too early, and no trace was generated. Now with --daemonize:
> 
> $ sudo trace-cmd record --daemonize -p nop -e 'sched:sched_process_exec' -A guest -p nop -e net &&
>     > ping -c 1 10.20.1.2 &&
>     > sudo start-stop-daemon --stop --signal INT --retry 20 --pidfile /var/run/trace-cmd-record.pid &&
>     > sudo trace-cmd report -i trace.dat -i trace-guest.dat | head  
> Negotiated kvm time sync protocol with guest guest
> Send SIGINT to pid 3071371 to stop recording
> PING 10.20.1.2 (10.20.1.2) 56(84) bytes of data.
> 64 bytes from 10.20.1.2: icmp_seq=1 ttl=64 time=0.134 ms
> --- 10.20.1.2 ping statistics ---
> 1 packets transmitted, 1 received, 0% packet loss, time 0ms
> rtt min/avg/max/mdev = 0.134/0.134/0.134/0.000 ms
> CPU0 data recorded at offset=0x14f000
>     229 bytes in size (4096 uncompressed)
> ....
>       trace.dat: cpus=28
> trace-guest.dat: cpus=1
>       trace.dat:           ping-3071450 [013] 1196830.834258: sched_process_exec:     filename=/bin/ping pid=3071450 old_pid=3071450
> trace-guest.dat:           <idle>-0     [000] 1196830.835990: napi_gro_receive_entry: dev=eth1 napi_id=0x2002 queue_mapping=1 skbaddr=0xffff95d051a5c400 vlan_tagged=0 vlan_proto=0x0000 vlan_tci=0x0000 protocol=0x0800 ip_summed=0 hash=0x00000000 l4_hash=0 len=84 data_len=0 truesize=768 mac_header_valid=1 mac_header=-14 nr_frags=0 gso_size=0 gso_type=0
> trace-guest.dat:           <idle>-0     [000] 1196830.835997: napi_gro_receive_exit:  ret=3
> trace-guest.dat:           <idle>-0     [000] 1196830.835998: netif_receive_skb:      dev=eth1 skbaddr=0xffff95d051a5c400x len=84
> trace-guest.dat:           <idle>-0     [000] 1196830.836021: net_dev_queue:          dev=eth1 skbaddr=0xffff95d051a5c700x len=98
> trace-guest.dat:           <idle>-0     [000] 1196830.836024: net_dev_start_xmit:     dev=eth1 queue_mapping=0 skbaddr=0xffff95d051a5c700 vlan_tagged=0 vlan_proto=0x0000 vlan_tci=0x0000 protocol=0x0800 ip_summed=0 len=98 data_len=0 network_offset=14 transport_offset_valid=1 transport_offset=34 tx_flags=0 gso_size=0 gso_segs=0 gso_type=0
> trace-guest.dat:           <idle>-0     [000] 1196830.836069: net_dev_xmit:           dev=eth1 skbaddr=0xffff95d051a5c700 len=98 rc=0
>       trace.dat:           sudo-3071451 [015] 1196830.838262: sched_process_exec:     filename=/usr/bin/sudo pid=3071451 old_pid=3071451

Could you add the above example to the man pages?

-- Steve

> 
> As for the implementation itself - unfortunately since daemon(3) calls fork(), we can't call it after
> creating other processes/threads (if we call it after creating the recorder processes it'll mess up the
> process hierarchy, and if after the tsync thread creation, it will mess up memory sharing).
> However the point in time where we want to exit to the caller is after those processes/threads
> have started, since that's when tracing actually starts.
> 
> Therefore, we implement the daemonization ourselves in the following way:
>     1. We fork() early, and make the parent waitpid on the child.
>     2. When the child finishes initializing everything, it detaches
>        from the session and sends a signal to the parent.
>     3. The parent can now exit, causing the entire process hierarchy
>        of the child to be inherited by init.
>     4. The daemonization is now complete, and the caller to trace-cmd
>        now executes the next thing, which will be traced.
> 
> Avidan Borisov (3):
>   trace-cmd record: Add --daemonize
>   trace-cmd: export pidfile functions from trace-listen.c
>   trace-cmd record: Create a pidfile when using --daemonize
> 
>  .../trace-cmd/trace-cmd-record.1.txt          |   4 +
>  tracecmd/include/trace-local.h                |   4 +
>  tracecmd/trace-listen.c                       |  32 ++--
>  tracecmd/trace-record.c                       | 141 +++++++++++++++++-
>  tracecmd/trace-usage.c                        |   3 +
>  5 files changed, 164 insertions(+), 20 deletions(-)
> 

Re: [PATCH 1/3] trace-cmd record: Add --daemonize

[Steven Rostedt]

On Mon,  1 May 2023 23:31:16 +0300
avidanborisov@gmail.com wrote:

> +static void daemonize_start(void)
> +{
> +	int devnull;
> +	int status;
> +	int pid;
> +	int rc;
> +
> +	pid = fork();
> +	if (pid == -1)
> +		die("daemonize: fork failed");
> +
> +	if (pid == 0) { /* child */
> +		/*
> +		 * We keep stdout and stderr open to allow the user to
> +		 * see output and errors after the daemonization (he can
> +		 * choose to supress it with >/dev/null if he wants).

Please don't use "he", we do have women users.

The above should be:

		 * We keep stdout and stderr open to allow the user to
		 * see output and errors after the daemonization (the user can
		 * choose to supress it with >/dev/null if the user wants).


> +		 *
> +		 * No reason to keep stdin open (it might interfere with the
> +		 * shell), we redirect it to /dev/null.
> +		 */
> +		devnull = open("/dev/null", O_RDONLY);
> +		if (devnull == -1)
> +			die("daemonize: open /dev/null failed");
> +
> +		if (devnull > 0) {
> +			if (dup2(devnull, 0) == -1)
> +				die("daemonize: dup2");
> +			close(0);
> +		}
> +
> +		return;

[..]

> @@ -6656,6 +6766,10 @@ static void parse_record_options(int argc,
>  				die("--fork option used for 'start' command only");
>  			fork_process = true;
>  			break;
> +		case OPT_daemonize:
> +			if (!IS_RECORD(ctx))
> +				die("--daemonize option used for 'record' command only");
> +			do_daemonize = true;

Missing "break".

I found this because I tested it on a VM that doesn't have tsc_nsec
support.

Thanks!

-- Steve

>  		case OPT_tsc2nsec:
>  			ret = get_tsc_nsec(&ctx->tsc2nsec.shift,
>  					   &ctx->tsc2nsec.mult);
> @@ -6899,6 +7013,9 @@ static void record_trace(int argc, char **argv,
>  	struct buffer_instance *instance;
>  	struct filter_pids *pid;
>  
> +	if (do_daemonize)
> +		daemonize_start();
> +
>  	/*
>  	 * If top_instance doesn't have any plugins or events, then
>  	 * remove it from being processed.
> @@ -7017,8 +7134,15 @@ static void record_trace(int argc, char **argv,
>  				}
>  			}
>  		}
> -		/* sleep till we are woken with Ctrl^C */
> -		printf("Hit Ctrl^C to stop recording\n");
> +
> +		if (do_daemonize) {
> +			daemonize_finish();
> +			printf("Send SIGINT to pid %d to stop recording\n", getpid());
> +		} else {
> +			/* sleep till we are woken with Ctrl^C */
> +			printf("Hit Ctrl^C to stop recording\n");
> +		}
> +
>  		for_all_instances(instance) {
>  			/* If an instance is not tracing individual processes
>  			 * or there is an error while waiting for a process to
> diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
> index 42a8e7d..0630e8e 100644
> --- a/tracecmd/trace-usage.c
> +++ b/tracecmd/trace-usage.c
> @@ -81,6 +81,7 @@ static struct usage_help usage_help[] = {
>  		"                        available algorithms can be listed with trace-cmd list -c\n"
>  		"          --proxy vsocket to reach the agent. Acts the same as -A (for an agent)\n"
>  		"              but will send the proxy connection to the agent.\n"
> +		"          --daemonize run trace-cmd in the background as a daemon after recording has started\n"
>  	},
>  	{
>  		"set",

[PATCH v2 0/4] trace-cmd record: Improvements to --daemonize option

[avidanborisov]

From: Avidan Borisov <avidanborisov@gmail.com>

Hi Steven,

This is the second version of my patch series for adding the --daemonize option to trace-cmd record, incorporating your feedback.

Changes since v1:
- Replaced all instances of "he" with "the user".
- Fixed the issue with the `break` statement in the switch case.
- Added a usage example of --daemonize to the man page.

Let me know if there are any further changes required.

Thanks,
Avidan

Avidan Borisov (4):
  trace-cmd record: Add --daemonize
  trace-cmd: export pidfile functions from trace-listen.c
  trace-cmd record: Create a pidfile when using --daemonize
  trace-cmd record: Add --daemonize example to man page

 .../trace-cmd/trace-cmd-record.1.txt          |  33 ++++
 tracecmd/include/trace-local.h                |   4 +
 tracecmd/trace-listen.c                       |  32 ++--
 tracecmd/trace-record.c                       | 142 +++++++++++++++++-
 tracecmd/trace-usage.c                        |   3 +
 5 files changed, 194 insertions(+), 20 deletions(-)

-- 
2.25.1

[PATCH v2 1/4] trace-cmd record: Add --daemonize

[avidanborisov]

From: Avidan Borisov <avidanborisov@gmail.com>

Add a --daemonize flag to trace-cmd record which allows the user to record
in the background, after tracing has completely initialized.

Signed-off-by: Avidan Borisov <avidanborisov@gmail.com>
---
 .../trace-cmd/trace-cmd-record.1.txt          |   3 +
 tracecmd/trace-record.c                       | 129 +++++++++++++++++-
 tracecmd/trace-usage.c                        |   1 +
 3 files changed, 131 insertions(+), 2 deletions(-)

diff --git a/Documentation/trace-cmd/trace-cmd-record.1.txt b/Documentation/trace-cmd/trace-cmd-record.1.txt
index b709e48..366ab46 100644
--- a/Documentation/trace-cmd/trace-cmd-record.1.txt
+++ b/Documentation/trace-cmd/trace-cmd-record.1.txt
@@ -418,6 +418,9 @@ OPTIONS
     run on a privileged guest that the host is aware of (as denoted by the
     'cid' in the *-P* option for the agent).
 
+*--daemonize*
+    Run trace-cmd in the background as a daemon after recording has started.
+
 EXAMPLES
 --------
 
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index bced804..0049768 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -91,6 +91,8 @@ static bool quiet;
 
 static bool fork_process;
 
+static bool do_daemonize;
+
 /* Max size to let a per cpu file get */
 static int max_kb;
 
@@ -1634,6 +1636,109 @@ static inline void ptrace_attach(struct buffer_instance *instance, int pid) { }
 
 #endif /* NO_PTRACE */
 
+static bool child_detached;
+
+static void daemonize_set_child_detached(int s)
+{
+	child_detached = true;
+}
+
+static void daemonize_start(void)
+{
+	int devnull;
+	int status;
+	int pid;
+	int rc;
+
+	pid = fork();
+	if (pid == -1)
+		die("daemonize: fork failed");
+
+	if (pid == 0) { /* child */
+		/*
+		 * We keep stdout and stderr open to allow the user to
+		 * see output and errors after the daemonization (the user can
+		 * choose to supress it with >/dev/null if the user wants).
+		 *
+		 * No reason to keep stdin open (it might interfere with the
+		 * shell), we redirect it to /dev/null.
+		 */
+		devnull = open("/dev/null", O_RDONLY);
+		if (devnull == -1)
+			die("daemonize: open /dev/null failed");
+
+		if (devnull > 0) {
+			if (dup2(devnull, 0) == -1)
+				die("daemonize: dup2");
+			close(0);
+		}
+
+		return;
+
+		/*
+		 * The child returns to back to the caller, but the parent waits until
+		 * SIGRTMIN is received from the child (by calling daemonize_finish()),
+		 * or the child exits for some reason (usually an indication of
+		 * an error), which ever comes first.
+		 *
+		 * Then the parent exits (with the status code of the child,
+		 * if it finished early, or with 0 if SIGRTMIN was received),
+		 * which causes the child (and its entire process tree) to be
+		 * inherited by init.
+		 *
+		 * Note that until the child calls daemonize_finish(), it still has
+		 * the same session id as the parent, so it can die together with
+		 * the parent before daemonization finished (purposefully, since the
+		 * user might send a quick Ctrl^C to cancel the command, and we don't
+		 * want background processes staying alive in that case)
+		 */
+	} else { /* parent */
+		struct sigaction sa = {
+			/* disable SA_RESTART, to allow waitpid() to be interrupted by SIGRTMIN */
+			.sa_flags = 0,
+			.sa_handler = daemonize_set_child_detached
+		};
+
+		if (sigemptyset(&sa.sa_mask) == -1)
+			die("daemonize: sigemptyset failed");
+
+		if (sigaddset(&sa.sa_mask, SIGRTMIN) == -1)
+			die("daemonize: sigaddset failed");
+
+		if (sigprocmask(SIG_UNBLOCK, &sa.sa_mask, NULL) == -1)
+			die("daemonize: sigprocmask failed");
+
+		if (sigaction(SIGRTMIN, &sa, NULL) == -1)
+			die("daemonize: sigaction failed");
+
+		do {
+			rc = waitpid(pid, &status, 0);
+		} while (!child_detached && ((rc == -1) && (errno == EINTR)));
+
+		if (child_detached)
+			exit(0);
+		else if (rc == pid)
+			exit(WIFEXITED(status));
+		else
+			die("daemonize: waitpid failed");
+
+		__builtin_unreachable();
+	}
+}
+
+static void daemonize_finish(void)
+{
+	/*
+	 * setsid() will also set the sid to be the pgid to all currently
+	 * running threads in the process group (such as the tsync thread).
+	 */
+	if (setsid() == -1)
+		die("daemonize: setsid");
+
+	if (kill(getppid(), SIGRTMIN) == -1)
+		die("daemonize: kill");
+}
+
 static void trace_or_sleep(enum trace_type type, bool pwait)
 {
 	int i;
@@ -1748,6 +1853,9 @@ static void run_cmd(enum trace_type type, const char *user, int argc, char **arg
 
 		execute_program(argc, argv);
 	}
+
+	if (do_daemonize)
+		daemonize_finish();
 	if (fork_process)
 		exit(0);
 	if (do_ptrace) {
@@ -5790,6 +5898,7 @@ enum {
 	OPT_proxy		= 261,
 	OPT_temp		= 262,
 	OPT_notimeout		= 264,
+	OPT_daemonize		= 265,
 };
 
 void trace_stop(int argc, char **argv)
@@ -6217,6 +6326,7 @@ static void parse_record_options(int argc,
 			{"file-version", required_argument, NULL, OPT_file_ver},
 			{"proxy", required_argument, NULL, OPT_proxy},
 			{"temp", required_argument, NULL, OPT_temp},
+			{"daemonize", no_argument, NULL, OPT_daemonize},
 			{NULL, 0, NULL, 0}
 		};
 
@@ -6690,6 +6800,11 @@ static void parse_record_options(int argc,
 				die("--fork option used for 'start' command only");
 			fork_process = true;
 			break;
+		case OPT_daemonize:
+			if (!IS_RECORD(ctx))
+				die("--daemonize option used for 'record' command only");
+			do_daemonize = true;
+			break;
 		case OPT_tsc2nsec:
 			ret = get_tsc_nsec(&ctx->tsc2nsec.shift,
 					   &ctx->tsc2nsec.mult);
@@ -6933,6 +7048,9 @@ static void record_trace(int argc, char **argv,
 	struct buffer_instance *instance;
 	struct filter_pids *pid;
 
+	if (do_daemonize)
+		daemonize_start();
+
 	/*
 	 * If top_instance doesn't have any plugins or events, then
 	 * remove it from being processed.
@@ -7051,8 +7169,15 @@ static void record_trace(int argc, char **argv,
 				}
 			}
 		}
-		/* sleep till we are woken with Ctrl^C */
-		printf("Hit Ctrl^C to stop recording\n");
+
+		if (do_daemonize) {
+			daemonize_finish();
+			printf("Send SIGINT to pid %d to stop recording\n", getpid());
+		} else {
+			/* sleep till we are woken with Ctrl^C */
+			printf("Hit Ctrl^C to stop recording\n");
+		}
+
 		for_all_instances(instance) {
 			/* If an instance is not tracing individual processes
 			 * or there is an error while waiting for a process to
diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
index ecb7cad..45cb4f0 100644
--- a/tracecmd/trace-usage.c
+++ b/tracecmd/trace-usage.c
@@ -81,6 +81,7 @@ static struct usage_help usage_help[] = {
 		"                        available algorithms can be listed with trace-cmd list -c\n"
 		"          --proxy vsocket to reach the agent. Acts the same as -A (for an agent)\n"
 		"              but will send the proxy connection to the agent.\n"
+		"          --daemonize run trace-cmd in the background as a daemon after recording has started\n"
 	},
 	{
 		"set",
-- 
2.25.1

[PATCH v2 2/4] trace-cmd: export pidfile functions from trace-listen.c

[avidanborisov]

From: Avidan Borisov <avidanborisov@gmail.com>

trace-listen.c has some utility functions for creating and removing
pidfiles, to avoid code duplication we make those functions generic
and export them to the rest of the codebase.

Signed-off-by: Avidan Borisov <avidanborisov@gmail.com>
---
 tracecmd/include/trace-local.h |  4 ++++
 tracecmd/trace-listen.c        | 32 ++++++++++++++------------------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/tracecmd/include/trace-local.h b/tracecmd/include/trace-local.h
index 8f18f6d..33397b1 100644
--- a/tracecmd/include/trace-local.h
+++ b/tracecmd/include/trace-local.h
@@ -461,4 +461,8 @@ static inline bool is_digits(const char *s)
 
 bool trace_tsc2nsec_is_supported(void);
 
+void make_pid_name(char *buf, const char *pidfile_basename);
+void remove_pid_file(const char *pidfile_basename);
+void make_pid_file(const char *pidfile_basename);
+
 #endif /* __TRACE_LOCAL_H */
diff --git a/tracecmd/trace-listen.c b/tracecmd/trace-listen.c
index e95c571..5894a92 100644
--- a/tracecmd/trace-listen.c
+++ b/tracecmd/trace-listen.c
@@ -34,6 +34,8 @@
 
 #define VAR_RUN_DIR		VAR_DIR_Q(VAR_DIR) "/run"
 
+#define LISTEN_PIDFILE		"trace-cmd-net.pid"
+
 static char *default_output_dir = ".";
 static char *output_dir;
 static char *default_output_file = "trace";
@@ -52,7 +54,8 @@ static bool done;
 #define pdie(fmt, ...)					\
 	do {						\
 		tracecmd_plog_error(fmt, ##__VA_ARGS__);\
-		remove_pid_file();			\
+		if (do_daemon)				\
+			remove_pid_file(LISTEN_PIDFILE);\
 		exit(-1);				\
 	} while (0)
 
@@ -126,21 +129,16 @@ static void finish(int sig)
 	done = true;
 }
 
-static void make_pid_name(int mode, char *buf)
+void make_pid_name(char *buf, const char *pidfile_basename)
 {
-	snprintf(buf, PATH_MAX, VAR_RUN_DIR "/trace-cmd-net.pid");
+	snprintf(buf, PATH_MAX, VAR_RUN_DIR "/%s", pidfile_basename);
 }
 
-static void remove_pid_file(void)
+void remove_pid_file(const char *pidfile_basename)
 {
 	char buf[PATH_MAX];
-	int mode = do_daemon;
-
-	if (!do_daemon)
-		return;
-
-	make_pid_name(mode, buf);
 
+	make_pid_name(buf, pidfile_basename);
 	unlink(buf);
 }
 
@@ -991,16 +989,12 @@ static void do_accept_loop(int sfd)
 	clean_up();
 }
 
-static void make_pid_file(void)
+void make_pid_file(const char *pidfile_basename)
 {
 	char buf[PATH_MAX];
-	int mode = do_daemon;
 	int fd;
 
-	if (!do_daemon)
-		return;
-
-	make_pid_name(mode, buf);
+	make_pid_name(buf, pidfile_basename);
 
 	fd = open(buf, O_WRONLY | O_CREAT | O_TRUNC, 0644);
 	if (fd < 0) {
@@ -1075,7 +1069,8 @@ static void do_listen(char *port)
 	if (!tracecmd_get_debug())
 		signal_setup(SIGCHLD, sigstub);
 
-	make_pid_file();
+	if (do_daemon)
+		make_pid_file(LISTEN_PIDFILE);
 
 	if (use_vsock)
 		sfd = get_vsock(port);
@@ -1090,7 +1085,8 @@ static void do_listen(char *port)
 
 	kill_clients();
 
-	remove_pid_file();
+	if (do_daemon)
+		remove_pid_file(LISTEN_PIDFILE);
 }
 
 static void start_daemon(void)
-- 
2.25.1

[PATCH v2 3/4] trace-cmd record: Create a pidfile when using --daemonize

[avidanborisov]

From: Avidan Borisov <avidanborisov@gmail.com>

When used with --daemonize, create a pidfile at
/var/run/trace-cmd-record.pid during the duration
of the recording, and delete it after we're done.

Signed-off-by: Avidan Borisov <avidanborisov@gmail.com>
---
 Documentation/trace-cmd/trace-cmd-record.1.txt |  1 +
 tracecmd/trace-record.c                        | 13 +++++++++++++
 tracecmd/trace-usage.c                         |  4 +++-
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/Documentation/trace-cmd/trace-cmd-record.1.txt b/Documentation/trace-cmd/trace-cmd-record.1.txt
index 366ab46..0c9a914 100644
--- a/Documentation/trace-cmd/trace-cmd-record.1.txt
+++ b/Documentation/trace-cmd/trace-cmd-record.1.txt
@@ -420,6 +420,7 @@ OPTIONS
 
 *--daemonize*
     Run trace-cmd in the background as a daemon after recording has started.
+    Creates a pidfile at /var/run/trace-cmd-record.pid with the pid of trace-cmd during the recording.
 
 EXAMPLES
 --------
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 0049768..69a10bf 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -46,6 +46,8 @@
 #define _STR(x) #x
 #define STR(x) _STR(x)
 
+#define RECORD_PIDFILE	"trace-cmd-record.pid"
+
 #define TRACE_CTRL	"tracing_on"
 #define TRACE		"trace"
 #define AVAILABLE	"available_tracers"
@@ -93,6 +95,8 @@ static bool fork_process;
 
 static bool do_daemonize;
 
+static bool created_pidfile;
+
 /* Max size to let a per cpu file get */
 static int max_kb;
 
@@ -646,6 +650,9 @@ void die(const char *fmt, ...)
 	else
 		ret = -1;
 
+	if (created_pidfile)
+		remove_pid_file(RECORD_PIDFILE);
+
 	kill_threads();
 	va_start(ap, fmt);
 	fprintf(stderr, "  ");
@@ -1737,6 +1744,9 @@ static void daemonize_finish(void)
 
 	if (kill(getppid(), SIGRTMIN) == -1)
 		die("daemonize: kill");
+
+	make_pid_file(RECORD_PIDFILE);
+	created_pidfile = true;
 }
 
 static void trace_or_sleep(enum trace_type type, bool pwait)
@@ -7225,6 +7235,9 @@ static void record_trace(int argc, char **argv,
 
 	destroy_stats();
 	finalize_record_trace(ctx);
+
+	if (created_pidfile)
+		remove_pid_file(RECORD_PIDFILE);
 }
 
 /*
diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
index 45cb4f0..37d576b 100644
--- a/tracecmd/trace-usage.c
+++ b/tracecmd/trace-usage.c
@@ -81,7 +81,9 @@ static struct usage_help usage_help[] = {
 		"                        available algorithms can be listed with trace-cmd list -c\n"
 		"          --proxy vsocket to reach the agent. Acts the same as -A (for an agent)\n"
 		"              but will send the proxy connection to the agent.\n"
-		"          --daemonize run trace-cmd in the background as a daemon after recording has started\n"
+		"          --daemonize run trace-cmd in the background as a daemon after recording has started.\n"
+		"                      creates a pidfile at /var/run/trace-cmd-record.pid with the pid of trace-cmd\n"
+		"                      during the recording.\n"
 	},
 	{
 		"set",
-- 
2.25.1

[PATCH v2 4/4] trace-cmd record: Add --daemonize example to man page

[avidanborisov]

From: Avidan Borisov <avidanborisov@gmail.com>

Add a small usage example of how --daemonize can be used with guest/host tracing for analyzing network activity.

Signed-off-by: Avidan Borisov <avidanborisov@gmail.com>
---
 .../trace-cmd/trace-cmd-record.1.txt          | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/Documentation/trace-cmd/trace-cmd-record.1.txt b/Documentation/trace-cmd/trace-cmd-record.1.txt
index 0c9a914..79ab3d0 100644
--- a/Documentation/trace-cmd/trace-cmd-record.1.txt
+++ b/Documentation/trace-cmd/trace-cmd-record.1.txt
@@ -515,6 +515,35 @@ task: sleep-21611
        => try_to_wake_up (0xffffffff8106340a)
 ----
 
+An example of using --daemonize together with guest/host tracing:
+[source,shell]
+----
+$ sudo trace-cmd record --daemonize -p nop -e 'sched:sched_process_exec' -A guest -p nop -e net &&
+> ping -c 1 10.20.1.2 &&
+> sudo start-stop-daemon --stop --signal INT --retry 20 --pidfile /var/run/trace-cmd-record.pid &&
+> sudo trace-cmd report -i trace.dat -i trace-guest.dat | head
+Negotiated kvm time sync protocol with guest guest
+Send SIGINT to pid 3071371 to stop recording
+PING 10.20.1.2 (10.20.1.2) 56(84) bytes of data.
+64 bytes from 10.20.1.2: icmp_seq=1 ttl=64 time=0.134 ms
+--- 10.20.1.2 ping statistics ---
+1 packets transmitted, 1 received, 0% packet loss, time 0ms
+rtt min/avg/max/mdev = 0.134/0.134/0.134/0.000 ms
+CPU0 data recorded at offset=0x14f000
+    229 bytes in size (4096 uncompressed)
+....
+      trace.dat: cpus=28
+trace-guest.dat: cpus=1
+      trace.dat:           ping-3071450 [013] 1196830.834258: sched_process_exec:     filename=/bin/ping pid=3071450 old_pid=3071450
+trace-guest.dat:           <idle>-0     [000] 1196830.835990: napi_gro_receive_entry: dev=eth1 napi_id=0x2002 queue_mapping=1 skbaddr=0xffff95d051a5c400 vlan_tagged=0 vlan_proto=0x0000 vlan_tci=0x0000 protocol=0x0800 ip_summed=0 hash=0x00000000 l4_hash=0 len=84 data_len=0 truesize=768 mac_header_valid=1 mac_header=-14 nr_frags=0 gso_size=0 gso_type=0
+trace-guest.dat:           <idle>-0     [000] 1196830.835997: napi_gro_receive_exit:  ret=3
+trace-guest.dat:           <idle>-0     [000] 1196830.835998: netif_receive_skb:      dev=eth1 skbaddr=0xffff95d051a5c400x len=84
+trace-guest.dat:           <idle>-0     [000] 1196830.836021: net_dev_queue:          dev=eth1 skbaddr=0xffff95d051a5c700x len=98
+trace-guest.dat:           <idle>-0     [000] 1196830.836024: net_dev_start_xmit:     dev=eth1 queue_mapping=0 skbaddr=0xffff95d051a5c700 vlan_tagged=0 vlan_proto=0x0000 vlan_tci=0x0000 protocol=0x0800 ip_summed=0 len=98 data_len=0 network_offset=14 transport_offset_valid=1 transport_offset=34 tx_flags=0 gso_size=0 gso_segs=0 gso_type=0
+trace-guest.dat:           <idle>-0     [000] 1196830.836069: net_dev_xmit:           dev=eth1 skbaddr=0xffff95d051a5c700 len=98 rc=0
+      trace.dat:           sudo-3071451 [015] 1196830.838262: sched_process_exec:     filename=/usr/bin/sudo pid=3071451 old_pid=3071451
+----
+
 SEE ALSO
 --------
 trace-cmd(1), trace-cmd-report(1), trace-cmd-start(1), trace-cmd-stop(1),
-- 
2.25.1

Re: [PATCH v2 0/4] trace-cmd record: Improvements to --daemonize option

[Steven Rostedt]

On Mon, 26 Jun 2023 12:16:31 +0300
avidanborisov@gmail.com wrote:

> From: Avidan Borisov <avidanborisov@gmail.com>
> 
> Hi Steven,

Thanks Avidan,

I'm currently traveling and then we have two US holidays for Monday and
Tuesday, I'll try to get to it within a week or two.

Also, when sending a v2, please make it a new thread and do not reply
to the previous version. It makes it more visible, otherwise, the
second version gets lost in the thread of the first.

Thanks,

-- Steve


> 
> This is the second version of my patch series for adding the --daemonize option to trace-cmd record, incorporating your feedback.
> 
> Changes since v1:
> - Replaced all instances of "he" with "the user".
> - Fixed the issue with the `break` statement in the switch case.
> - Added a usage example of --daemonize to the man page.
> 
> Let me know if there are any further changes required.
> 
> Thanks,
> Avidan
> 
> Avidan Borisov (4):
>   trace-cmd record: Add --daemonize
>   trace-cmd: export pidfile functions from trace-listen.c
>   trace-cmd record: Create a pidfile when using --daemonize
>   trace-cmd record: Add --daemonize example to man page
> 
>  .../trace-cmd/trace-cmd-record.1.txt          |  33 ++++
>  tracecmd/include/trace-local.h                |   4 +
>  tracecmd/trace-listen.c                       |  32 ++--
>  tracecmd/trace-record.c                       | 142 +++++++++++++++++-
>  tracecmd/trace-usage.c                        |   3 +
>  5 files changed, 194 insertions(+), 20 deletions(-)
> 

Re: [PATCH v2 1/4] trace-cmd record: Add --daemonize

[Steven Rostedt]

On Mon, 26 Jun 2023 12:16:32 +0300
avidanborisov@gmail.com wrote:

> +		if (do_daemonize) {
> +			daemonize_finish();
> +			printf("Send SIGINT to pid %d to stop recording\n", getpid());

I wonder if we should also add SIGTERM, as that's the default signal used by "kill".

It would suck to have a nice trace going and then execute "kill $pid" only
to find out it killed the process and didn't finish processing it.

-- Steve


> +		} else {
> +			/* sleep till we are woken with Ctrl^C */
> +			printf("Hit Ctrl^C to stop recording\n");
> +		}

Re: [PATCH v2 1/4] trace-cmd record: Add --daemonize

[Steven Rostedt]

On Mon, 26 Jun 2023 12:16:32 +0300
avidanborisov@gmail.com wrote:

> +static void daemonize_start(void)
> +{
> +	int devnull;
> +	int status;
> +	int pid;
> +	int rc;

To keep consistent with the rest of the code, use "ret" and not "rc".

> +
> +	pid = fork();
> +	if (pid == -1)
> +		die("daemonize: fork failed");
> +
> +	if (pid == 0) { /* child */
> +		/*
> +		 * We keep stdout and stderr open to allow the user to
> +		 * see output and errors after the daemonization (the user can
> +		 * choose to supress it with >/dev/null if the user wants).
> +		 *
> +		 * No reason to keep stdin open (it might interfere with the
> +		 * shell), we redirect it to /dev/null.
> +		 */
> +		devnull = open("/dev/null", O_RDONLY);
> +		if (devnull == -1)
> +			die("daemonize: open /dev/null failed");
> +
> +		if (devnull > 0) {
> +			if (dup2(devnull, 0) == -1)
> +				die("daemonize: dup2");
> +			close(0);
> +		}
> +
> +		return;
> +
> +		/*
> +		 * The child returns to back to the caller, but the parent waits until
> +		 * SIGRTMIN is received from the child (by calling daemonize_finish()),
> +		 * or the child exits for some reason (usually an indication of
> +		 * an error), which ever comes first.
> +		 *
> +		 * Then the parent exits (with the status code of the child,
> +		 * if it finished early, or with 0 if SIGRTMIN was received),
> +		 * which causes the child (and its entire process tree) to be
> +		 * inherited by init.
> +		 *
> +		 * Note that until the child calls daemonize_finish(), it still has
> +		 * the same session id as the parent, so it can die together with
> +		 * the parent before daemonization finished (purposefully, since the
> +		 * user might send a quick Ctrl^C to cancel the command, and we don't
> +		 * want background processes staying alive in that case)
> +		 */
> +	} else { /* parent */
> +		struct sigaction sa = {
> +			/* disable SA_RESTART, to allow waitpid() to be interrupted by SIGRTMIN */
> +			.sa_flags = 0,
> +			.sa_handler = daemonize_set_child_detached
> +		};
> +
> +		if (sigemptyset(&sa.sa_mask) == -1)
> +			die("daemonize: sigemptyset failed");
> +
> +		if (sigaddset(&sa.sa_mask, SIGRTMIN) == -1)
> +			die("daemonize: sigaddset failed");
> +
> +		if (sigprocmask(SIG_UNBLOCK, &sa.sa_mask, NULL) == -1)
> +			die("daemonize: sigprocmask failed");
> +
> +		if (sigaction(SIGRTMIN, &sa, NULL) == -1)
> +			die("daemonize: sigaction failed");
> +
> +		do {
> +			rc = waitpid(pid, &status, 0);
> +		} while (!child_detached && ((rc == -1) && (errno == EINTR)));

And usually it's a bit more robust to check for less than zero, instead
of -1, not a biggy, just my preference, as I code in the kernel, and errors
are not always -1, but some other negative number (-EINTR for instance).
Also, you don't need all those parenthesis.

		} while (!child_detached && ret < 0 && errno == EINTR);

-- Steve

		

> +
> +		if (child_detached)
> +			exit(0);
> +		else if (rc == pid)
> +			exit(WIFEXITED(status));
> +		else
> +			die("daemonize: waitpid failed");
> +
> +		__builtin_unreachable();
> +	}
> +}
> +
> +static void daemonize_finish(void)
> +{
> +	/*
> +	 * setsid() will also set the sid to be the pgid to all currently
> +	 * running threads in the process group (such as the tsync thread).
> +	 */
> +	if (setsid() == -1)
> +		die("daemonize: setsid");
> +
> +	if (kill(getppid(), SIGRTMIN) == -1)
> +		die("daemonize: kill");
> +}
> +
>  static void trace_or_sleep(enum trace_type type, bool pwait)
>  {
>  	int i;
> @@ -1748,6 +1853,9 @@ static void run_cmd(enum trace_type type, const char *user, int argc, char **arg
>  
>  		execute_program(argc, argv);
>  	}
> +
> +	if (do_daemonize)
> +		daemonize_finish();
>  	if (fork_process)
>  		exit(0);
>  	if (do_ptrace) {
> @@ -5790,6 +5898,7 @@ enum {
>  	OPT_proxy		= 261,
>  	OPT_temp		= 262,
>  	OPT_notimeout		= 264,
> +	OPT_daemonize		= 265,
>  };
>  
>  void trace_stop(int argc, char **argv)
> @@ -6217,6 +6326,7 @@ static void parse_record_options(int argc,
>  			{"file-version", required_argument, NULL, OPT_file_ver},
>  			{"proxy", required_argument, NULL, OPT_proxy},
>  			{"temp", required_argument, NULL, OPT_temp},
> +			{"daemonize", no_argument, NULL, OPT_daemonize},
>  			{NULL, 0, NULL, 0}
>  		};
>  
> @@ -6690,6 +6800,11 @@ static void parse_record_options(int argc,
>  				die("--fork option used for 'start' command only");
>  			fork_process = true;
>  			break;
> +		case OPT_daemonize:
> +			if (!IS_RECORD(ctx))
> +				die("--daemonize option used for 'record' command only");
> +			do_daemonize = true;
> +			break;
>  		case OPT_tsc2nsec:
>  			ret = get_tsc_nsec(&ctx->tsc2nsec.shift,
>  					   &ctx->tsc2nsec.mult);
> @@ -6933,6 +7048,9 @@ static void record_trace(int argc, char **argv,
>  	struct buffer_instance *instance;
>  	struct filter_pids *pid;
>  
> +	if (do_daemonize)
> +		daemonize_start();
> +
>  	/*
>  	 * If top_instance doesn't have any plugins or events, then
>  	 * remove it from being processed.
> @@ -7051,8 +7169,15 @@ static void record_trace(int argc, char **argv,
>  				}
>  			}
>  		}
> -		/* sleep till we are woken with Ctrl^C */
> -		printf("Hit Ctrl^C to stop recording\n");
> +
> +		if (do_daemonize) {
> +			daemonize_finish();
> +			printf("Send SIGINT to pid %d to stop recording\n", getpid());
> +		} else {
> +			/* sleep till we are woken with Ctrl^C */
> +			printf("Hit Ctrl^C to stop recording\n");
> +		}
> +
>  		for_all_instances(instance) {
>  			/* If an instance is not tracing individual processes
>  			 * or there is an error while waiting for a process to
> diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
> index ecb7cad..45cb4f0 100644
> --- a/tracecmd/trace-usage.c
> +++ b/tracecmd/trace-usage.c
> @@ -81,6 +81,7 @@ static struct usage_help usage_help[] = {
>  		"                        available algorithms can be listed with trace-cmd list -c\n"
>  		"          --proxy vsocket to reach the agent. Acts the same as -A (for an agent)\n"
>  		"              but will send the proxy connection to the agent.\n"
> +		"          --daemonize run trace-cmd in the background as a daemon after recording has started\n"
>  	},
>  	{
>  		"set",