[PATCH 0/2] dumpcap,pdump handle cleanup signals

[PATCH 0/2] dumpcap,pdump handle cleanup signals

[Stephen Hemminger]

The dumpcap and pdump commands need to handle more signals.

Stephen Hemminger (2):
  app/dumpcap: handle SIGTERM and SIGHUP
  app/pdump: handle SIGTERM and SIGHUP

 app/dumpcap/main.c | 14 ++++++++++++--
 app/pdump/main.c   | 21 +++++++++++++++------
 2 files changed, 27 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH 1/2] app/dumpcap: handle SIGTERM and SIGHUP

[Stephen Hemminger]

If application is killed (SIGTERM), or the console session
ends (SIGHUP) or the write to the output file fails (SIGPIPE)
then do cleanup before exiting.

This also makes DPDK dumpcap behave more like Wireshark dumpcap.

Suggested-by: Isaac Boukris <iboukris@gmail.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/dumpcap/main.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index d57db0589a3f..76c747511444 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -939,6 +939,11 @@ int main(int argc, char **argv)
 {
 	struct rte_ring *r;
 	struct rte_mempool *mp;
+	struct sigaction action = {
+		.sa_flags = SA_RESTART,
+		.sa_handler = signal_handler,
+	};
+	struct sigaction origaction;
 	dumpcap_out_t out;
 	char *p;
 
@@ -964,8 +969,13 @@ int main(int argc, char **argv)
 
 	compile_filters();
 
-	signal(SIGINT, signal_handler);
-	signal(SIGPIPE, SIG_IGN);
+	sigemptyset(&action.sa_mask);
+	sigaction(SIGTERM, &action, NULL);
+	sigaction(SIGINT, &action, NULL);
+	sigaction(SIGPIPE, &action, NULL);
+	sigaction(SIGHUP, NULL, &origaction);
+	if (origaction.sa_handler == SIG_DFL)
+		sigaction(SIGHUP, &action, NULL);
 
 	enable_primary_monitor();
 
-- 
2.43.0

[PATCH 2/2] app/pdump: handle SIGTERM and SIGHUP

[Stephen Hemminger]

Like dumpcap, the pdump process should cleanup if process
terminates due to being killed or hangup.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/pdump/main.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index a9205e130bb1..3592f8a865ad 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -571,11 +571,9 @@ disable_primary_monitor(void)
 }
 
 static void
-signal_handler(int sig_num)
+signal_handler(int sig_num __rte_unused)
 {
-	if (sig_num == SIGINT) {
-		quit_signal = 1;
-	}
+	quit_signal = 1;
 }
 
 static inline int
@@ -975,6 +973,11 @@ enable_primary_monitor(void)
 int
 main(int argc, char **argv)
 {
+	struct sigaction action = {
+		.sa_flags = SA_RESTART,
+		.sa_handler = signal_handler,
+	};
+	struct sigaction origaction;
 	int diag;
 	int ret;
 	int i;
@@ -983,8 +986,14 @@ main(int argc, char **argv)
 	char mp_flag[] = "--proc-type=secondary";
 	char *argp[argc + 2];
 
-	/* catch ctrl-c so we can print on exit */
-	signal(SIGINT, signal_handler);
+	/* catch ctrl-c so we can cleanup on exit */
+	sigemptyset(&action.sa_mask);
+	sigaction(SIGTERM, &action, NULL);
+	sigaction(SIGINT, &action, NULL);
+	sigaction(SIGPIPE, &action, NULL);
+	sigaction(SIGHUP, NULL, &origaction);
+	if (origaction.sa_handler == SIG_DFL)
+		sigaction(SIGHUP, &action, NULL);
 
 	argp[0] = argv[0];
 	argp[1] = n_flag;
-- 
2.43.0

RE: [PATCH 2/2] app/pdump: handle SIGTERM and SIGHUP

[Pattan, Reshma]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> +	sigaction(SIGHUP, NULL, &origaction);
 
Why do we need this ?  Can't be this direct sigaction(SIGHUP, &action, NULL)  without a below if code? 
Can you please explain how this works. 

> +	if (origaction.sa_handler == SIG_DFL)
> +		sigaction(SIGHUP, &action, NULL);
> 


Other changes looks ok to me.

Regards,
Reshma

Re: [PATCH 2/2] app/pdump: handle SIGTERM and SIGHUP

[Stephen Hemminger]

On Tue, 27 Feb 2024 09:59:37 +0000
"Pattan, Reshma" <reshma.pattan@intel.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > +	sigaction(SIGHUP, NULL, &origaction);  
>  
> Why do we need this ?  Can't be this direct sigaction(SIGHUP, &action, NULL)  without a below if code? 
> Can you please explain how this works. 
> 
> > +	if (origaction.sa_handler == SIG_DFL)
> > +		sigaction(SIGHUP, &action, NULL);
> >   
> 


If sighup is being ignored already (for example being run by nohup)
then the program should keep ignoring SIGHUP.

This is the method used in tshark and dumpcap utilities in wireshark
and it made sense.

[PATCH v2 0/2] Fix pdump and dumpcap leaks on SIGTERM

[Stephen Hemminger]

Fix bug where dumpcap and pdump programs would leave resources
if killed with SIGTERM (or if file gets full).

v2 - rebase and rewording

Stephen Hemminger (2):
  app/dumpcap: handle SIGTERM and SIGHUP
  app/pdump: handle SIGTERM and SIGHUP

 app/dumpcap/main.c | 14 ++++++++++++--
 app/pdump/main.c   | 21 +++++++++++++++------
 2 files changed, 27 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] app/dumpcap: handle SIGTERM and SIGHUP

[Stephen Hemminger]

If the dumpcap is killed it does not cleanup which leaves
the ring buffer and the memory pool behind.
Until resource are exhausted devices will continue to fill the ring.

Fix by having the application handle SIGTERM, SIGHUP, and SIGPIPE.
These are the same signals handled in same way by the wireshark
dumpcap application.

Fixes: cbb44143be74 ("app/dumpcap: add new packet capture application")

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/dumpcap/main.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index b25b95e2fc..ba91ca94d0 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -936,6 +936,11 @@ int main(int argc, char **argv)
 {
 	struct rte_ring *r;
 	struct rte_mempool *mp;
+	struct sigaction action = {
+		.sa_flags = SA_RESTART,
+		.sa_handler = signal_handler,
+	};
+	struct sigaction origaction;
 	dumpcap_out_t out;
 	char *p;
 
@@ -961,8 +966,13 @@ int main(int argc, char **argv)
 
 	compile_filters();
 
-	signal(SIGINT, signal_handler);
-	signal(SIGPIPE, SIG_IGN);
+	sigemptyset(&action.sa_mask);
+	sigaction(SIGTERM, &action, NULL);
+	sigaction(SIGINT, &action, NULL);
+	sigaction(SIGPIPE, &action, NULL);
+	sigaction(SIGHUP, NULL, &origaction);
+	if (origaction.sa_handler == SIG_DFL)
+		sigaction(SIGHUP, &action, NULL);
 
 	enable_primary_monitor();
 
-- 
2.43.0

[PATCH v2 2/2] app/pdump: handle SIGTERM and SIGHUP

[Stephen Hemminger]

The pdump application will leak ring and memory pool if killed
by SIGTERM.  Like dumpcap, the pdump process should cleanup
if process terminates due to being killed or hangup.

Fixes: caa7028276b8 ("app/pdump: add tool for packet capturing")

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/pdump/main.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index 3bdbb8dc78..fa85859703 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -571,11 +571,9 @@ disable_primary_monitor(void)
 }
 
 static void
-signal_handler(int sig_num)
+signal_handler(int sig_num __rte_unused)
 {
-	if (sig_num == SIGINT) {
-		quit_signal = 1;
-	}
+	quit_signal = 1;
 }
 
 static inline int
@@ -975,6 +973,11 @@ enable_primary_monitor(void)
 int
 main(int argc, char **argv)
 {
+	struct sigaction action = {
+		.sa_flags = SA_RESTART,
+		.sa_handler = signal_handler,
+	};
+	struct sigaction origaction;
 	int diag;
 	int ret;
 	int i;
@@ -983,8 +986,14 @@ main(int argc, char **argv)
 	char mp_flag[] = "--proc-type=secondary";
 	char *argp[argc + 2];
 
-	/* catch ctrl-c so we can print on exit */
-	signal(SIGINT, signal_handler);
+	/* catch ctrl-c so we can cleanup on exit */
+	sigemptyset(&action.sa_mask);
+	sigaction(SIGTERM, &action, NULL);
+	sigaction(SIGINT, &action, NULL);
+	sigaction(SIGPIPE, &action, NULL);
+	sigaction(SIGHUP, NULL, &origaction);
+	if (origaction.sa_handler == SIG_DFL)
+		sigaction(SIGHUP, &action, NULL);
 
 	argp[0] = argv[0];
 	argp[1] = n_flag;
-- 
2.43.0

[PATCH v3 0/3] dumpcap and pdump patches for 24.07

[Stephen Hemminger]

Fix bug where dumpcap and pdump programs would leave resources
if killed with SIGTERM (or if file gets full).

Fix bug where dumpcap would always run on CPU 0 only.

v3 - combine and rebase these patches

Stephen Hemminger (3):
  app/dumpcap: handle SIGTERM and SIGHUP
  app/pdump: handle SIGTERM and SIGHUP
  dumpcap: add lcores option

 app/dumpcap/main.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
 app/pdump/main.c   | 21 +++++++++++++++------
 2 files changed, 58 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH v3 1/3] app/dumpcap: handle SIGTERM and SIGHUP

[Stephen Hemminger]

If the dumpcap is killed it does not cleanup which leaves
the ring buffer and the memory pool behind.
Until resource are exhausted devices will continue to fill the ring.

Fix by having the application handle SIGTERM, SIGHUP, and SIGPIPE.
These are the same signals handled in same way by the wireshark
dumpcap application.

Fixes: cbb44143be74 ("app/dumpcap: add new packet capture application")

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/dumpcap/main.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index b25b95e2fc..ba91ca94d0 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -936,6 +936,11 @@ int main(int argc, char **argv)
 {
 	struct rte_ring *r;
 	struct rte_mempool *mp;
+	struct sigaction action = {
+		.sa_flags = SA_RESTART,
+		.sa_handler = signal_handler,
+	};
+	struct sigaction origaction;
 	dumpcap_out_t out;
 	char *p;
 
@@ -961,8 +966,13 @@ int main(int argc, char **argv)
 
 	compile_filters();
 
-	signal(SIGINT, signal_handler);
-	signal(SIGPIPE, SIG_IGN);
+	sigemptyset(&action.sa_mask);
+	sigaction(SIGTERM, &action, NULL);
+	sigaction(SIGINT, &action, NULL);
+	sigaction(SIGPIPE, &action, NULL);
+	sigaction(SIGHUP, NULL, &origaction);
+	if (origaction.sa_handler == SIG_DFL)
+		sigaction(SIGHUP, &action, NULL);
 
 	enable_primary_monitor();
 
-- 
2.43.0

[PATCH v3 2/3] app/pdump: handle SIGTERM and SIGHUP

[Stephen Hemminger]

The pdump application will leak ring and memory pool if killed
by SIGTERM.  Like dumpcap, the pdump process should cleanup
if process terminates due to being killed or hangup.

Fixes: caa7028276b8 ("app/pdump: add tool for packet capturing")

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/pdump/main.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index 3bdbb8dc78..fa85859703 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -571,11 +571,9 @@ disable_primary_monitor(void)
 }
 
 static void
-signal_handler(int sig_num)
+signal_handler(int sig_num __rte_unused)
 {
-	if (sig_num == SIGINT) {
-		quit_signal = 1;
-	}
+	quit_signal = 1;
 }
 
 static inline int
@@ -975,6 +973,11 @@ enable_primary_monitor(void)
 int
 main(int argc, char **argv)
 {
+	struct sigaction action = {
+		.sa_flags = SA_RESTART,
+		.sa_handler = signal_handler,
+	};
+	struct sigaction origaction;
 	int diag;
 	int ret;
 	int i;
@@ -983,8 +986,14 @@ main(int argc, char **argv)
 	char mp_flag[] = "--proc-type=secondary";
 	char *argp[argc + 2];
 
-	/* catch ctrl-c so we can print on exit */
-	signal(SIGINT, signal_handler);
+	/* catch ctrl-c so we can cleanup on exit */
+	sigemptyset(&action.sa_mask);
+	sigaction(SIGTERM, &action, NULL);
+	sigaction(SIGINT, &action, NULL);
+	sigaction(SIGPIPE, &action, NULL);
+	sigaction(SIGHUP, NULL, &origaction);
+	if (origaction.sa_handler == SIG_DFL)
+		sigaction(SIGHUP, &action, NULL);
 
 	argp[0] = argv[0];
 	argp[1] = n_flag;
-- 
2.43.0

[PATCH v3 3/3] dumpcap: add lcores option

[Stephen Hemminger]

The dumpcap application is reading from ring and writing to
the kernel. By default the EAL init will cause the main thread
to bound to the first lcore (cpu 0). Add a command line option
to select the lcore to use; or if no lcores are specified
then just be a normal process and let the CPU scheduler handle it.
Letting scheduler is likely to work well for process doint
I/O with kernel.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/dumpcap/main.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index ba91ca94d0..cb2a439f79 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -38,6 +38,7 @@
 #include <rte_pdump.h>
 #include <rte_ring.h>
 #include <rte_string_fns.h>
+#include <rte_thread.h>
 #include <rte_time.h>
 #include <rte_version.h>
 
@@ -60,6 +61,7 @@ static const char *tmp_dir = "/tmp";
 static unsigned int ring_size = 2048;
 static const char *capture_comment;
 static const char *file_prefix;
+static const char *lcore_arg;
 static bool dump_bpf;
 static bool show_interfaces;
 static bool print_stats;
@@ -143,6 +145,7 @@ static void usage(void)
 	       "                           (default: /tmp)\n"
 	       "\n"
 	       "Miscellaneous:\n"
+	       "  --lcore=<core>	   cpu core to run on (default: any)\n"
 	       "  --file-prefix=<prefix>   prefix to use for multi-process\n"
 	       "  -q                       don't report packet capture counts\n"
 	       "  -v, --version            print version information and exit\n"
@@ -343,6 +346,7 @@ static void parse_opts(int argc, char **argv)
 		{ "ifdescr",	     required_argument, NULL, 0 },
 		{ "ifname",	     required_argument, NULL, 0 },
 		{ "interface",       required_argument, NULL, 'i' },
+		{ "lcore",	     required_argument, NULL, 0 },
 		{ "list-interfaces", no_argument,       NULL, 'D' },
 		{ "no-promiscuous-mode", no_argument,   NULL, 'p' },
 		{ "output-file",     required_argument, NULL, 'w' },
@@ -369,6 +373,8 @@ static void parse_opts(int argc, char **argv)
 
 			if (!strcmp(longopt, "capture-comment")) {
 				capture_comment = optarg;
+			} else if (!strcmp(longopt, "lcore")) {
+				lcore_arg = optarg;
 			} else if (!strcmp(longopt, "file-prefix")) {
 				file_prefix = optarg;
 			} else if (!strcmp(longopt, "temp-dir")) {
@@ -608,12 +614,16 @@ static void dpdk_init(void)
 		"--log-level", "notice"
 	};
 	int eal_argc = RTE_DIM(args);
+	rte_cpuset_t cpuset = { };
 	char **eal_argv;
 	unsigned int i;
 
 	if (file_prefix != NULL)
 		eal_argc += 2;
 
+	if (lcore_arg != NULL)
+		eal_argc += 2;
+
 	/* DPDK API requires mutable versions of command line arguments. */
 	eal_argv = calloc(eal_argc + 1, sizeof(char *));
 	if (eal_argv == NULL)
@@ -623,6 +633,11 @@ static void dpdk_init(void)
 	for (i = 1; i < RTE_DIM(args); i++)
 		eal_argv[i] = strdup(args[i]);
 
+	if (lcore_arg != NULL) {
+		eal_argv[i++] = strdup("--lcores");
+		eal_argv[i++] = strdup(lcore_arg);
+	}
+
 	if (file_prefix != NULL) {
 		eal_argv[i++] = strdup("--file-prefix");
 		eal_argv[i++] = strdup(file_prefix);
@@ -633,8 +648,24 @@ static void dpdk_init(void)
 			rte_panic("No memory\n");
 	}
 
+	/*
+	 * Need to get the original cpuset, before EAL init changes
+	 * the affinity of this thread (main lcore).
+	 */
+	if (lcore_arg == NULL &&
+	    rte_thread_get_affinity_by_id(rte_thread_self(), &cpuset) != 0)
+		rte_panic("rte_thread_getaffinity failed\n");
+
 	if (rte_eal_init(eal_argc, eal_argv) < 0)
 		rte_exit(EXIT_FAILURE, "EAL init failed: is primary process running?\n");
+
+	/*
+	 * If no lcore argument was specified, then run this program as a normal process
+	 * which can be scheduled on any non-isolated CPU.
+	 */
+	if (lcore_arg == NULL &&
+	    rte_thread_set_affinity_by_id(rte_thread_self(), &cpuset) != 0)
+		rte_exit(EXIT_FAILURE, "Can not restore original cpu affinity\n");
 }
 
 /* Create packet ring shared between callbacks and process */
-- 
2.43.0

Re: [PATCH v3 0/3] dumpcap and pdump patches for 24.07

[Thomas Monjalon]

03/07/2024 17:45, Stephen Hemminger:
> Stephen Hemminger (3):
>   app/dumpcap: handle SIGTERM and SIGHUP
>   app/pdump: handle SIGTERM and SIGHUP
>   dumpcap: add lcores option

Applied, thanks.

Note: I did few edits for space aligned arguments and "CPU" uppercase.