[PATCH v2 1/3] app/testpmd: cleanup cleanly from signal

[PATCH v2 1/3] app/testpmd: cleanup cleanly from signal

[Stephen Hemminger]

Do a clean shutdown of testpmd when a signal is received; instead of
having testpmd kill itself.  This fixes the problem where a signal could
be received in the middle of a PMD and then the signal handler would
call PMD's close routine leading to locking problems.

The cmdline structure no longer needs to be global it can
just be local to the prompt() function.

An added benefit is it gets rid of some Windows specific code.

(cherry picked from commit 0fd1386c30c3ad9365d7fdd2829bf7cb2e1b9dff)

Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
---
 app/test-pmd/cmdline.c | 31 +++++++----------
 app/test-pmd/testpmd.c | 77 ++++++++++++++++++++----------------------
 app/test-pmd/testpmd.h |  1 +
 3 files changed, 49 insertions(+), 60 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 43857c8008..eeca7171a7 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -68,8 +68,6 @@
 #include "cmdline_tm.h"
 #include "bpf_cmd.h"
 
-static struct cmdline *testpmd_cl;
-
 static void cmd_reconfig_device_queue(portid_t id, uint8_t dev, uint8_t queue);
 
 /* *** Help command with introduction. *** */
@@ -18045,31 +18043,26 @@ cmdline_read_from_file(const char *filename)
 void
 prompt(void)
 {
-	int ret;
+	struct cmdline *cl;
+
 	/* initialize non-constant commands */
 	cmd_set_fwd_mode_init();
 	cmd_set_fwd_retry_mode_init();
 
-	testpmd_cl = cmdline_stdin_new(main_ctx, "testpmd> ");
-	if (testpmd_cl == NULL)
+	cl = cmdline_stdin_new(main_ctx, "testpmd> ");
+	if (cl == NULL)
 		return;
 
-	ret = atexit(prompt_exit);
-	if (ret != 0)
-		fprintf(stderr, "Cannot set exit function for cmdline\n");
+	/* loop until signal or quit command */
+	while (f_quit == 0 && cl_quit == 0) {
+		int status = cmdline_poll(cl);
 
-	cmdline_interact(testpmd_cl);
-	if (ret != 0)
-		cmdline_stdin_exit(testpmd_cl);
-}
-
-void
-prompt_exit(void)
-{
-	if (testpmd_cl != NULL) {
-		cmdline_quit(testpmd_cl);
-		cmdline_stdin_exit(testpmd_cl);
+		if (status < 0 || status == RDLINE_EXITED)
+			break;
 	}
+
+	cmdline_quit(cl);
+	cmdline_stdin_exit(cl);
 }
 
 static void
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 5f34641e90..57e7fa0637 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -11,6 +11,7 @@
 #include <fcntl.h>
 #ifndef RTE_EXEC_ENV_WINDOWS
 #include <sys/mman.h>
+#include <sys/select.h>
 #endif
 #include <sys/types.h>
 #include <errno.h>
@@ -228,7 +229,7 @@ unsigned int xstats_display_num; /**< Size of extended statistics to show */
  * In container, it cannot terminate the process which running with 'stats-period'
  * option. Set flag to exit stats period loop after received SIGINT/SIGTERM.
  */
-static volatile uint8_t f_quit;
+volatile uint8_t f_quit;
 uint8_t cl_quit; /* Quit testpmd from cmdline. */
 
 /*
@@ -4300,13 +4301,6 @@ init_port(void)
 	memset(txring_numa, NUMA_NO_CONFIG, RTE_MAX_ETHPORTS);
 }
 
-static void
-force_quit(void)
-{
-	pmd_test_exit();
-	prompt_exit();
-}
-
 static void
 print_stats(void)
 {
@@ -4325,28 +4319,9 @@ print_stats(void)
 }
 
 static void
-signal_handler(int signum)
+signal_handler(int signum __rte_unused)
 {
-	if (signum == SIGINT || signum == SIGTERM) {
-		fprintf(stderr, "\nSignal %d received, preparing to exit...\n",
-			signum);
-#ifdef RTE_LIB_PDUMP
-		/* uninitialize packet capture framework */
-		rte_pdump_uninit();
-#endif
-#ifdef RTE_LIB_LATENCYSTATS
-		if (latencystats_enabled != 0)
-			rte_latencystats_uninit();
-#endif
-		force_quit();
-		/* Set flag to indicate the force termination. */
-		f_quit = 1;
-		/* exit with the expected status */
-#ifndef RTE_EXEC_ENV_WINDOWS
-		signal(signum, SIG_DFL);
-		kill(getpid(), signum);
-#endif
-	}
+	f_quit = 1;
 }
 
 int
@@ -4522,15 +4497,9 @@ main(int argc, char** argv)
 			start_packet_forwarding(0);
 		}
 		prompt();
-		pmd_test_exit();
 	} else
 #endif
 	{
-		char c;
-		int rc;
-
-		f_quit = 0;
-
 		printf("No commandline core given, start packet forwarding\n");
 		start_packet_forwarding(tx_first);
 		if (stats_period != 0) {
@@ -4553,15 +4522,41 @@ main(int argc, char** argv)
 				prev_time = cur_time;
 				rte_delay_us_sleep(US_PER_S);
 			}
-		}
+		} else {
+			char c;
+			fd_set fds;
+
+			printf("Press enter to exit\n");
 
-		printf("Press enter to exit\n");
-		rc = read(0, &c, 1);
-		pmd_test_exit();
-		if (rc < 0)
-			return 1;
+			FD_ZERO(&fds);
+			FD_SET(0, &fds);
+
+			/* wait for signal or enter */
+			ret = select(1, &fds, NULL, NULL, NULL);
+			if (ret < 0 && errno != EINTR)
+				rte_exit(EXIT_FAILURE,
+					 "Select failed: %s\n",
+					 strerror(errno));
+
+			/* if got enter then consume it */
+			if (ret == 1 && read(0, &c, 1) < 0)
+				rte_exit(EXIT_FAILURE,
+					 "Read failed: %s\n",
+					 strerror(errno));
+		}
 	}
 
+	pmd_test_exit();
+
+#ifdef RTE_LIB_PDUMP
+	/* uninitialize packet capture framework */
+	rte_pdump_uninit();
+#endif
+#ifdef RTE_LIB_LATENCYSTATS
+	if (latencystats_enabled != 0)
+		rte_latencystats_uninit();
+#endif
+
 	ret = rte_eal_cleanup();
 	if (ret != 0)
 		rte_exit(EXIT_FAILURE,
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index e53320e630..e3a322402e 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -33,6 +33,7 @@
 #define RTE_PORT_HANDLING       (uint16_t)3
 
 extern uint8_t cl_quit;
+extern volatile uint8_t f_quit;
 
 /*
  * It is used to allocate the memory for hash key.
-- 
2.45.2

[PATCH v2 2/3] app/testpmd: fix early exit from signal

[Stephen Hemminger]

Other signals may occur causing read to get interrupted.
Loop until quit flag is set by signal, a character is entered,
or end of file. This fixes bug where testpmd would exit early
because of signal used by TAP device.

Bugzilla ID: 1305
Fixes: 0fd1386c30c3 ("app/testpmd: cleanup cleanly from signal")
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
(cherry picked from commit a996cd04aeeaeca88e6313174101a1229349fb47)
---
 app/test-pmd/testpmd.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 57e7fa0637..bade1feba4 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -11,7 +11,6 @@
 #include <fcntl.h>
 #ifndef RTE_EXEC_ENV_WINDOWS
 #include <sys/mman.h>
-#include <sys/select.h>
 #endif
 #include <sys/types.h>
 #include <errno.h>
@@ -4524,25 +4523,17 @@ main(int argc, char** argv)
 			}
 		} else {
 			char c;
-			fd_set fds;
 
 			printf("Press enter to exit\n");
-
-			FD_ZERO(&fds);
-			FD_SET(0, &fds);
-
-			/* wait for signal or enter */
-			ret = select(1, &fds, NULL, NULL, NULL);
-			if (ret < 0 && errno != EINTR)
-				rte_exit(EXIT_FAILURE,
-					 "Select failed: %s\n",
-					 strerror(errno));
-
-			/* if got enter then consume it */
-			if (ret == 1 && read(0, &c, 1) < 0)
-				rte_exit(EXIT_FAILURE,
-					 "Read failed: %s\n",
+			while (f_quit == 0) {
+				/* end-of-file or any character exits loop */
+				if (read(0, &c, 1) >= 0)
+					break;
+				if (errno == EINTR)
+					continue;
+				rte_exit(EXIT_FAILURE, "Read failed: %s\n",
 					 strerror(errno));
+			}
 		}
 	}
 
-- 
2.45.2

[PATCH v2 3/3] app/testpmd: fix interactive mode on Windows

[Stephen Hemminger]

The cmdline_poll() function is broken and was not fully tested,
go back to using cmdline_interact().

Instead, use sigaction() to cancel read character on Unix OS's
and a new helper to cancel I/O on Windows.

Bugzilla ID: 1180
Fixes: 0fd1386c30c3 ("app/testpmd: cleanup cleanly from signal")
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
(cherry picked from commit f1d0993e034e39968a2c80a8561b46c260c27487)
---
 app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
 app/test-pmd/testpmd.c           | 11 +++++++++++
 lib/cmdline/cmdline.c            |  1 +
 lib/cmdline/cmdline_os_unix.c    |  6 ++++++
 lib/cmdline/cmdline_os_windows.c | 14 ++++++++++++++
 lib/cmdline/cmdline_private.h    |  5 ++++-
 6 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index eeca7171a7..781b7ce0db 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -70,6 +70,8 @@
 
 static void cmd_reconfig_device_queue(portid_t id, uint8_t dev, uint8_t queue);
 
+static struct cmdline *testpmd_cl;
+
 /* *** Help command with introduction. *** */
 struct cmd_help_brief_result {
 	cmdline_fixed_string_t help;
@@ -18039,30 +18041,29 @@ cmdline_read_from_file(const char *filename)
 	printf("Read CLI commands from %s\n", filename);
 }
 
+void
+prompt_exit(void)
+{
+	cmdline_quit(testpmd_cl);
+}
+
 /* prompt function, called from main on MAIN lcore */
 void
 prompt(void)
 {
-	struct cmdline *cl;
-
 	/* initialize non-constant commands */
 	cmd_set_fwd_mode_init();
 	cmd_set_fwd_retry_mode_init();
 
-	cl = cmdline_stdin_new(main_ctx, "testpmd> ");
-	if (cl == NULL)
+	testpmd_cl = cmdline_stdin_new(main_ctx, "testpmd> ");
+	if (testpmd_cl == NULL) {
+		fprintf(stderr,
+			"Failed to create stdin based cmdline context\n");
 		return;
-
-	/* loop until signal or quit command */
-	while (f_quit == 0 && cl_quit == 0) {
-		int status = cmdline_poll(cl);
-
-		if (status < 0 || status == RDLINE_EXITED)
-			break;
 	}
 
-	cmdline_quit(cl);
-	cmdline_stdin_exit(cl);
+	cmdline_interact(testpmd_cl);
+	cmdline_stdin_exit(testpmd_cl);
 }
 
 static void
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index bade1feba4..fbfc090d68 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -4321,6 +4321,7 @@ static void
 signal_handler(int signum __rte_unused)
 {
 	f_quit = 1;
+	prompt_exit();
 }
 
 int
@@ -4331,8 +4332,18 @@ main(int argc, char** argv)
 	uint16_t count;
 	int ret;
 
+#ifdef RTE_EXEC_ENV_WINDOWS
 	signal(SIGINT, signal_handler);
 	signal(SIGTERM, signal_handler);
+#else
+	/* Want read() not to be restarted on signal */
+	struct sigaction action = {
+		.sa_handler = signal_handler,
+	};
+
+	sigaction(SIGINT, &action, NULL);
+	sigaction(SIGTERM, &action, NULL);
+#endif
 
 	testpmd_logtype = rte_log_register("testpmd");
 	if (testpmd_logtype < 0)
diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c
index 5600f012c2..d0601d69f4 100644
--- a/lib/cmdline/cmdline.c
+++ b/lib/cmdline/cmdline.c
@@ -175,6 +175,7 @@ cmdline_quit(struct cmdline *cl)
 {
 	if (!cl)
 		return;
+	cmdline_cancel(cl);
 	rdline_quit(&cl->rdl);
 }
 
diff --git a/lib/cmdline/cmdline_os_unix.c b/lib/cmdline/cmdline_os_unix.c
index 64a945a34f..9a4ec4e334 100644
--- a/lib/cmdline/cmdline_os_unix.c
+++ b/lib/cmdline/cmdline_os_unix.c
@@ -51,3 +51,9 @@ cmdline_vdprintf(int fd, const char *format, va_list op)
 {
 	return vdprintf(fd, format, op);
 }
+
+/* This function is not needed on Linux, instead use sigaction() */
+void
+cmdline_cancel(__rte_unused struct cmdline *cl)
+{
+}
diff --git a/lib/cmdline/cmdline_os_windows.c b/lib/cmdline/cmdline_os_windows.c
index 73ed9ba290..80863bfc8a 100644
--- a/lib/cmdline/cmdline_os_windows.c
+++ b/lib/cmdline/cmdline_os_windows.c
@@ -203,3 +203,17 @@ cmdline_vdprintf(int fd, const char *format, va_list op)
 
 	return ret;
 }
+
+void
+cmdline_cancel(struct cmdline *cl)
+{
+	if (!cl)
+		return;
+
+	/* force the outstanding read on console to exit */
+	if (cl->oldterm.is_console_input) {
+		HANDLE handle = (HANDLE)_get_osfhandle(cl->s_in);
+
+		CancelIoEx(handle, NULL);
+	}
+}
diff --git a/lib/cmdline/cmdline_private.h b/lib/cmdline/cmdline_private.h
index a3271c7693..86a46cdea6 100644
--- a/lib/cmdline/cmdline_private.h
+++ b/lib/cmdline/cmdline_private.h
@@ -24,7 +24,7 @@
 #define RDLINE_HISTORY_MAX_LINE 64
 
 struct rdline {
-	enum rdline_status status;
+	volatile enum rdline_status status;
 	/* rdline bufs */
 	struct cirbuf left;
 	struct cirbuf right;
@@ -90,6 +90,9 @@ int cmdline_poll_char(struct cmdline *cl);
 /* Read one character from input. */
 ssize_t cmdline_read_char(struct cmdline *cl, char *c);
 
+/* Force current cmdline read to unblock. */
+void cmdline_cancel(struct cmdline *cl);
+
 /* vdprintf(3) */
 __rte_format_printf(2, 0)
 int cmdline_vdprintf(int fd, const char *format, va_list op);
-- 
2.45.2

Re: [PATCH v2 0/3] Backport of testpmd signal fixes

[Kevin Traynor]

On 02/09/2024 20:47, Stephen Hemminger wrote:
> Backport some of the signal handling changes for testpmd.
> 
> v2 - rebase on current 21.11 branch
> 
> Stephen Hemminger (3):
>   app/testpmd: cleanup cleanly from signal
>   app/testpmd: fix early exit from signal
>   app/testpmd: fix interactive mode on Windows
> 
>  app/test-pmd/cmdline.c           | 32 ++++++-------
>  app/test-pmd/testpmd.c           | 79 +++++++++++++++-----------------
>  app/test-pmd/testpmd.h           |  1 +
>  lib/cmdline/cmdline.c            |  1 +
>  lib/cmdline/cmdline_os_unix.c    |  6 +++
>  lib/cmdline/cmdline_os_windows.c | 14 ++++++
>  lib/cmdline/cmdline_private.h    |  5 +-
>  7 files changed, 77 insertions(+), 61 deletions(-)
> 

Thanks Stephen. Pushed to 21.11 LTS branch.

Kevin.