[RFC 1/2] testpmd: make f_quit flag volatile

[RFC 1/2] testpmd: make f_quit flag volatile

[Stephen Hemminger]

Since f_quit is set in a signal handler it needs
to be marked as volatile. Otherwise, compler is allowed
to optimize away access to it.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test-pmd/testpmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 5b0f0838dcc8..815dd6dab4e3 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -231,7 +231,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.
  */
-uint8_t f_quit;
+static volatile uint8_t f_quit;
 uint8_t cl_quit; /* Quit testpmd from cmdline. */
 
 /*
-- 
2.35.1

[RFC 2/2] testpmd: cleanup cleanly from signal

[Stephen Hemminger]

The original behavior of testpmd was to kill itself when
it received a SIGINT or SIGTERM. This makes it hard to use
testpmd in test automation where forwarding loop is started
and then stopped via SIGTERM.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test-pmd/testpmd.c | 76 +++++++++++++++++++++++++-----------------
 1 file changed, 46 insertions(+), 30 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 815dd6dab4e3..8c19ded1655e 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -15,6 +15,7 @@
 #include <sys/types.h>
 #include <errno.h>
 #include <stdbool.h>
+#include <poll.h>
 
 #include <sys/queue.h>
 #include <sys/stat.h>
@@ -4243,25 +4244,37 @@ print_stats(void)
 static void
 signal_handler(int signum)
 {
-	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
+	fprintf(stderr, "\nSignal %d %s received, preparing to exit...\n",
+		signum, strsignal(signum));
+
+	/* Set flag to indicate the force termination. */
+	f_quit = 1;
+}
+
+static int
+wait_for_input(void)
+{
+	struct pollfd pfd = {
+		.fd = STDIN_FILENO,
+		.events =  POLLIN,
+	};
+	char c;
+
+	printf("Press enter to exit\n");
+	for (;;) {
+		if (f_quit)
+			return 0;
+
+		if (poll(&pfd, 1, -1) < 0) {
+			if (errno == EINTR)
+				continue;
+			return -1;
+		}
+
+		if (read(STDIN_FILENO, &c, 1) < 0)
+			return -1;
+
+		return 0;
 	}
 }
 
@@ -4441,11 +4454,6 @@ main(int argc, char** argv)
 	} 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) {
@@ -4468,15 +4476,23 @@ main(int argc, char** argv)
 				prev_time = cur_time;
 				rte_delay_us_sleep(US_PER_S);
 			}
+		} else {
+			if (wait_for_input() < 0)
+				return 1;
 		}
-
-		printf("Press enter to exit\n");
-		rc = read(0, &c, 1);
-		pmd_test_exit();
-		if (rc < 0)
-			return 1;
+		stop_packet_forwarding();
+		force_quit();
 	}
 
+#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,
-- 
2.35.1

Re: [RFC 2/2] testpmd: cleanup cleanly from signal

[Andrew Rybchenko]

On 10/14/22 20:23, Stephen Hemminger wrote:
> The original behavior of testpmd was to kill itself when
> it received a SIGINT or SIGTERM. This makes it hard to use
> testpmd in test automation where forwarding loop is started
> and then stopped via SIGTERM.

Can automatic stop it using SIGINT?

> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

[snip]

Re: [RFC 2/2] testpmd: cleanup cleanly from signal

[Stephen Hemminger]

On Sun, 6 Nov 2022 13:50:36 +0300
Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> wrote:

> On 10/14/22 20:23, Stephen Hemminger wrote:
> > The original behavior of testpmd was to kill itself when
> > it received a SIGINT or SIGTERM. This makes it hard to use
> > testpmd in test automation where forwarding loop is started
> > and then stopped via SIGTERM.  
> 
> Can automatic stop it using SIGINT?

Doesn't matter the testpmd code has same bug in SIGINT and SIGTERM.

The fundamental problem is that regular signals in Unix model are delivered
to the process and any thread may receive them.

In the case of testpmd, the signal may arrive inside some driver which
is doing some operations with hardware and even holding a lock.
Then signal_handler() is called. This code does operations that
may interact with driver and other parts of the system.
It is a broken way to handle this. 

There are two ways to handle this problem. One way would be to block
the signals and use signalfd() and epoll() to handle them. This would
be more difficult and invasive in the testpmd logic.

The simpler way is to just set a flag and do the cleanup in the
main loop when the flag is detected.

[PATCH v2] testpmd: cleanup cleanly from signal

[Stephen Hemminger]

Do clean shutdown of testpmd when a signal is received.
Instead of having testpmd commit suicide when a signal
is received, do a clean shutdown. This fixes problem
where a signal could be received in the middle of a PMD
and then the signal handler would call PMD's close routine
which could cause a deadlock.

Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v2 - use sigaction() instead of poll() to handle interrupted read

 app/test-pmd/testpmd.c | 58 ++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 28 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index cf5942d0c422..29b17d7d6d82 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -15,6 +15,7 @@
 #include <sys/types.h>
 #include <errno.h>
 #include <stdbool.h>
+#include <poll.h>
 
 #include <sys/queue.h>
 #include <sys/stat.h>
@@ -4251,26 +4252,11 @@ print_stats(void)
 static void
 signal_handler(int signum)
 {
-	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
-	}
+	fprintf(stderr, "\nSignal %d %s received, preparing to exit...\n",
+		signum, strsignal(signum));
+
+	/* Set flag to indicate the force termination. */
+	f_quit = 1;
 }
 
 int
@@ -4280,9 +4266,14 @@ main(int argc, char** argv)
 	portid_t port_id;
 	uint16_t count;
 	int ret;
+	struct sigaction sa = {
+		.sa_handler = signal_handler,
+		.sa_flags = 0,
+	};
 
-	signal(SIGINT, signal_handler);
-	signal(SIGTERM, signal_handler);
+	/* Want read() to be interrupted not restarted */
+	sigaction(SIGINT, &sa, NULL);
+	sigaction(SIGTERM, &sa, NULL);
 
 	testpmd_logtype = rte_log_register("testpmd");
 	if (testpmd_logtype < 0)
@@ -4476,15 +4467,26 @@ main(int argc, char** argv)
 				prev_time = cur_time;
 				rte_delay_us_sleep(US_PER_S);
 			}
+		} else {
+			printf("Press enter to exit\n");
+			rc = read(0, &c, 1);
+			if (rc < 0 && errno != EINTR)
+				fprintf(stderr, "Read stdin failed: %s\n",
+					strerror(errno));
 		}
-
-		printf("Press enter to exit\n");
-		rc = read(0, &c, 1);
-		pmd_test_exit();
-		if (rc < 0)
-			return 1;
+		stop_packet_forwarding();
+		force_quit();
 	}
 
+#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,
-- 
2.35.1

[PATCH v3] testpmd: cleanup cleanly from signal

[Stephen Hemminger]

Do clean shutdown of testpmd when a signal is received.
Instead of having testpmd commit suicide when a signal
is received, do a clean shutdown. This fixes problem
where a signal could be received in the middle of a PMD
and then the signal handler would call PMD's close routine
which could cause a deadlock.

Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v3 - drop unnecessary poll.h that causes windows build failure

 app/test-pmd/testpmd.c | 57 +++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 28 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index cf5942d0c422..341788f5bc84 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -4251,26 +4251,11 @@ print_stats(void)
 static void
 signal_handler(int signum)
 {
-	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
-	}
+	fprintf(stderr, "\nSignal %d %s received, preparing to exit...\n",
+		signum, strsignal(signum));
+
+	/* Set flag to indicate the force termination. */
+	f_quit = 1;
 }
 
 int
@@ -4280,9 +4265,14 @@ main(int argc, char** argv)
 	portid_t port_id;
 	uint16_t count;
 	int ret;
+	struct sigaction sa = {
+		.sa_handler = signal_handler,
+		.sa_flags = 0,
+	};
 
-	signal(SIGINT, signal_handler);
-	signal(SIGTERM, signal_handler);
+	/* Want read() to be interrupted not restarted */
+	sigaction(SIGINT, &sa, NULL);
+	sigaction(SIGTERM, &sa, NULL);
 
 	testpmd_logtype = rte_log_register("testpmd");
 	if (testpmd_logtype < 0)
@@ -4476,15 +4466,26 @@ main(int argc, char** argv)
 				prev_time = cur_time;
 				rte_delay_us_sleep(US_PER_S);
 			}
+		} else {
+			printf("Press enter to exit\n");
+			rc = read(0, &c, 1);
+			if (rc < 0 && errno != EINTR)
+				fprintf(stderr, "Read stdin failed: %s\n",
+					strerror(errno));
 		}
-
-		printf("Press enter to exit\n");
-		rc = read(0, &c, 1);
-		pmd_test_exit();
-		if (rc < 0)
-			return 1;
+		stop_packet_forwarding();
+		force_quit();
 	}
 
+#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,
-- 
2.35.1

[PATCH v4] 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 problem where a signal could be received
in the middle of a PMD and then the signal handler would call
PMD's close routine which could cause a deadlock.

Added benefit is it gets rid of Windows specific code.

Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v4 - use select() because that is available on Windows; and other
     functions poll() and sigaction() are not.

 app/test-pmd/testpmd.c | 63 +++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 29 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index cf5942d0c422..274e96cac2d4 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -12,6 +12,7 @@
 #ifndef RTE_EXEC_ENV_WINDOWS
 #include <sys/mman.h>
 #endif
+#include <sys/select.h>
 #include <sys/types.h>
 #include <errno.h>
 #include <stdbool.h>
@@ -4251,26 +4252,11 @@ print_stats(void)
 static void
 signal_handler(int signum)
 {
-	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
-	}
+	fprintf(stderr, "\nSignal %d %s received, preparing to exit...\n",
+		signum, strsignal(signum));
+
+	/* Set flag to indicate the force termination. */
+	f_quit = 1;
 }
 
 int
@@ -4449,9 +4435,6 @@ main(int argc, char** argv)
 	} else
 #endif
 	{
-		char c;
-		int rc;
-
 		f_quit = 0;
 
 		printf("No commandline core given, start packet forwarding\n");
@@ -4476,15 +4459,37 @@ 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");
-		rc = read(0, &c, 1);
-		pmd_test_exit();
-		if (rc < 0)
-			return 1;
+			printf("Press enter to exit\n");
+
+			FD_ZERO(&fds);
+			FD_SET(0, &fds);
+
+			if (select(1, &fds, NULL, NULL, NULL) <= 0) {
+				fprintf(stderr, "Select failed: %s\n",
+					strerror(errno));
+			} else if (read(0, &c, 1) <= 0) {
+				fprintf(stderr,
+					"Read stdin failed: %s\n",
+					strerror(errno));
+			}
+		}
+		stop_packet_forwarding();
+		force_quit();
 	}
 
+#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,
-- 
2.35.1

Re: [PATCH v4] testpmd: cleanup cleanly from signal

[Mattias Rönnblom]

On 2022-11-09 05:10, Stephen Hemminger wrote:
> Do a clean shutdown of testpmd when a signal is received;
> instead of having testpmd kill itself.
> This fixes problem where a signal could be received
> in the middle of a PMD and then the signal handler would call
> PMD's close routine which could cause a deadlock.
> 
> Added benefit is it gets rid of Windows specific code.
> 
> Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v4 - use select() because that is available on Windows; and other
>       functions poll() and sigaction() are not.
> 
>   app/test-pmd/testpmd.c | 63 +++++++++++++++++++++++-------------------
>   1 file changed, 34 insertions(+), 29 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index cf5942d0c422..274e96cac2d4 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -12,6 +12,7 @@
>   #ifndef RTE_EXEC_ENV_WINDOWS
>   #include <sys/mman.h>
>   #endif
> +#include <sys/select.h>
>   #include <sys/types.h>
>   #include <errno.h>
>   #include <stdbool.h>
> @@ -4251,26 +4252,11 @@ print_stats(void)
>   static void
>   signal_handler(int signum)
>   {
> -	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
> -	}
> +	fprintf(stderr, "\nSignal %d %s received, preparing to exit...\n",
> +		signum, strsignal(signum));

fprintf() is not async signal safe, and neither is strsignal().

This is not a regression introduced by this patch, but I thought it 
might be worth fixing.

> +
> +	/* Set flag to indicate the force termination. */
> +	f_quit = 1;
>   }
>   
>   int
> @@ -4449,9 +4435,6 @@ main(int argc, char** argv)
>   	} else
>   #endif
>   	{
> -		char c;
> -		int rc;
> -
>   		f_quit = 0;
>   
>   		printf("No commandline core given, start packet forwarding\n");
> @@ -4476,15 +4459,37 @@ 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");
> -		rc = read(0, &c, 1);
> -		pmd_test_exit();
> -		if (rc < 0)
> -			return 1;
> +			printf("Press enter to exit\n");
> +
> +			FD_ZERO(&fds);
> +			FD_SET(0, &fds);
> +
> +			if (select(1, &fds, NULL, NULL, NULL) <= 0) {
> +				fprintf(stderr, "Select failed: %s\n",
> +					strerror(errno));

Why is select() needed? Wouldn't a blocking read suffice? Or getchar().

> +			} else if (read(0, &c, 1) <= 0) {
> +				fprintf(stderr,
> +					"Read stdin failed: %s\n",
> +					strerror(errno));
> +			}
> +		}
> +		stop_packet_forwarding();
> +		force_quit();
>   	}
>   
> +#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,

Re: [PATCH v4] testpmd: cleanup cleanly from signal

[Stephen Hemminger]

On Wed, 9 Nov 2022 22:46:55 +0100
Mattias Rönnblom <hofors@lysator.liu.se> wrote:

> On 2022-11-09 05:10, Stephen Hemminger wrote:
> > Do a clean shutdown of testpmd when a signal is received;
> > instead of having testpmd kill itself.
> > This fixes problem where a signal could be received
> > in the middle of a PMD and then the signal handler would call
> > PMD's close routine which could cause a deadlock.
> > 
> > Added benefit is it gets rid of Windows specific code.
> > 
> > Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> > v4 - use select() because that is available on Windows; and other
> >       functions poll() and sigaction() are not.
> > 
> >   app/test-pmd/testpmd.c | 63 +++++++++++++++++++++++-------------------
> >   1 file changed, 34 insertions(+), 29 deletions(-)
> > 
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> > index cf5942d0c422..274e96cac2d4 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -12,6 +12,7 @@
> >   #ifndef RTE_EXEC_ENV_WINDOWS
> >   #include <sys/mman.h>
> >   #endif
> > +#include <sys/select.h>
> >   #include <sys/types.h>
> >   #include <errno.h>
> >   #include <stdbool.h>
> > @@ -4251,26 +4252,11 @@ print_stats(void)
> >   static void
> >   signal_handler(int signum)
> >   {
> > -	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
> > -	}
> > +	fprintf(stderr, "\nSignal %d %s received, preparing to exit...\n",
> > +		signum, strsignal(signum));  
> 
> fprintf() is not async signal safe, and neither is strsignal().
> 
> This is not a regression introduced by this patch, but I thought it 
> might be worth fixing.
> 
> > +
> > +	/* Set flag to indicate the force termination. */
> > +	f_quit = 1;
> >   }
> >   
> >   int
> > @@ -4449,9 +4435,6 @@ main(int argc, char** argv)
> >   	} else
> >   #endif
> >   	{
> > -		char c;
> > -		int rc;
> > -
> >   		f_quit = 0;
> >   
> >   		printf("No commandline core given, start packet forwarding\n");
> > @@ -4476,15 +4459,37 @@ 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");
> > -		rc = read(0, &c, 1);
> > -		pmd_test_exit();
> > -		if (rc < 0)
> > -			return 1;
> > +			printf("Press enter to exit\n");
> > +
> > +			FD_ZERO(&fds);
> > +			FD_SET(0, &fds);
> > +
> > +			if (select(1, &fds, NULL, NULL, NULL) <= 0) {
> > +				fprintf(stderr, "Select failed: %s\n",
> > +					strerror(errno));  
> 
> Why is select() needed? Wouldn't a blocking read suffice? Or getchar().

On Linux, signal set SA_RESTART so a simple read is not interrupted.
One option was to use sigaction() which allows controlling flags, but that
won't work on Windows.  Using select() works on both.

Re: [PATCH v4] testpmd: cleanup cleanly from signal

[Mattias Rönnblom]

On 2022-11-09 23:53, Stephen Hemminger wrote:
> On Wed, 9 Nov 2022 22:46:55 +0100
> Mattias Rönnblom <hofors@lysator.liu.se> wrote:
> 
>> On 2022-11-09 05:10, Stephen Hemminger wrote:
>>> Do a clean shutdown of testpmd when a signal is received;
>>> instead of having testpmd kill itself.
>>> This fixes problem where a signal could be received
>>> in the middle of a PMD and then the signal handler would call
>>> PMD's close routine which could cause a deadlock.
>>>
>>> Added benefit is it gets rid of Windows specific code.
>>>
>>> Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>> ---
>>> v4 - use select() because that is available on Windows; and other
>>>        functions poll() and sigaction() are not.
>>>
>>>    app/test-pmd/testpmd.c | 63 +++++++++++++++++++++++-------------------
>>>    1 file changed, 34 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>>> index cf5942d0c422..274e96cac2d4 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -12,6 +12,7 @@
>>>    #ifndef RTE_EXEC_ENV_WINDOWS
>>>    #include <sys/mman.h>
>>>    #endif
>>> +#include <sys/select.h>
>>>    #include <sys/types.h>
>>>    #include <errno.h>
>>>    #include <stdbool.h>
>>> @@ -4251,26 +4252,11 @@ print_stats(void)
>>>    static void
>>>    signal_handler(int signum)
>>>    {
>>> -	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
>>> -	}
>>> +	fprintf(stderr, "\nSignal %d %s received, preparing to exit...\n",
>>> +		signum, strsignal(signum));
>>
>> fprintf() is not async signal safe, and neither is strsignal().
>>
>> This is not a regression introduced by this patch, but I thought it
>> might be worth fixing.
>>
>>> +
>>> +	/* Set flag to indicate the force termination. */
>>> +	f_quit = 1;
>>>    }
>>>    
>>>    int
>>> @@ -4449,9 +4435,6 @@ main(int argc, char** argv)
>>>    	} else
>>>    #endif
>>>    	{
>>> -		char c;
>>> -		int rc;
>>> -
>>>    		f_quit = 0;
>>>    
>>>    		printf("No commandline core given, start packet forwarding\n");
>>> @@ -4476,15 +4459,37 @@ 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");
>>> -		rc = read(0, &c, 1);
>>> -		pmd_test_exit();
>>> -		if (rc < 0)
>>> -			return 1;
>>> +			printf("Press enter to exit\n");
>>> +
>>> +			FD_ZERO(&fds);
>>> +			FD_SET(0, &fds);
>>> +
>>> +			if (select(1, &fds, NULL, NULL, NULL) <= 0) {
>>> +				fprintf(stderr, "Select failed: %s\n",
>>> +					strerror(errno));
>>
>> Why is select() needed? Wouldn't a blocking read suffice? Or getchar().
> 
> On Linux, signal set SA_RESTART so a simple read is not interrupted.
> One option was to use sigaction() which allows controlling flags, but that
> won't work on Windows.  Using select() works on both.
> 

OK, so select() is used because a signal might interrupt read() on Windows?

while (read(0, &c, 1) == -1 && errno == EINTR)
         ;

Would that work?

(select() won't return 0 since you don't have a timeout.)

Re: [PATCH v4] testpmd: cleanup cleanly from signal

[Stephen Hemminger]

On Thu, 10 Nov 2022 08:50:40 +0100
Mattias Rönnblom <hofors@lysator.liu.se> wrote:

> >>
> >> Why is select() needed? Wouldn't a blocking read suffice? Or getchar().  
> > 
> > On Linux, signal set SA_RESTART so a simple read is not interrupted.
> > One option was to use sigaction() which allows controlling flags, but that
> > won't work on Windows.  Using select() works on both.
> >   
> 
> OK, so select() is used because a signal might interrupt read() on Windows?
> 
> while (read(0, &c, 1) == -1 && errno == EINTR)
>          ;
> 
> Would that work?

Try it. On Linux the read never gets interrupted.

Re: [PATCH v4] testpmd: cleanup cleanly from signal

[Mattias Rönnblom]

On 2022-11-10 17:14, Stephen Hemminger wrote:
> On Thu, 10 Nov 2022 08:50:40 +0100
> Mattias Rönnblom <hofors@lysator.liu.se> wrote:
> 
>>>>
>>>> Why is select() needed? Wouldn't a blocking read suffice? Or getchar().
>>>
>>> On Linux, signal set SA_RESTART so a simple read is not interrupted.
>>> One option was to use sigaction() which allows controlling flags, but that
>>> won't work on Windows.  Using select() works on both.
>>>    
>>
>> OK, so select() is used because a signal might interrupt read() on Windows?
>>
>> while (read(0, &c, 1) == -1 && errno == EINTR)
>>           ;
>>
>> Would that work?
> 
> Try it. On Linux the read never gets interrupted.

I had no doubts about that, but I misunderstood the code and thought 
that was the required behavior.

[PATCH v5] 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 problem where a signal could be received
in the middle of a PMD and then the signal handler would call
PMD's close routine which could cause a deadlock.

Added benefit is it gets rid of Windows specific code.

Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v5 - fix build on Windows.
     Windows has select() function but the prototype is in winsock.h
     not sys/select.h

 app/test-pmd/testpmd.c | 63 +++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 29 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index cf5942d0c422..be8fa7f0a0b1 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>
@@ -4251,26 +4252,11 @@ print_stats(void)
 static void
 signal_handler(int signum)
 {
-	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
-	}
+	fprintf(stderr, "\nSignal %d received, preparing to exit...\n",
+		signum);
+
+	/* Set flag to indicate the force termination. */
+	f_quit = 1;
 }
 
 int
@@ -4449,9 +4435,6 @@ main(int argc, char** argv)
 	} else
 #endif
 	{
-		char c;
-		int rc;
-
 		f_quit = 0;
 
 		printf("No commandline core given, start packet forwarding\n");
@@ -4476,15 +4459,37 @@ 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");
-		rc = read(0, &c, 1);
-		pmd_test_exit();
-		if (rc < 0)
-			return 1;
+			printf("Press enter to exit\n");
+
+			FD_ZERO(&fds);
+			FD_SET(0, &fds);
+
+			if (select(1, &fds, NULL, NULL, NULL) <= 0) {
+				fprintf(stderr, "Select failed: %s\n",
+					strerror(errno));
+			} else if (read(0, &c, 1) <= 0) {
+				fprintf(stderr,
+					"Read stdin failed: %s\n",
+					strerror(errno));
+			}
+		}
+		stop_packet_forwarding();
+		force_quit();
 	}
 
+#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,
-- 
2.35.1

Re: [PATCH v5] testpmd: cleanup cleanly from signal

[Andrew Rybchenko]

On 11/9/22 20:29, Stephen Hemminger wrote:
> Do a clean shutdown of testpmd when a signal is received;
> instead of having testpmd kill itself.
> This fixes problem where a signal could be received
> in the middle of a PMD and then the signal handler would call
> PMD's close routine which could cause a deadlock.
> 
> Added benefit is it gets rid of Windows specific code.
> 
> Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")

Cc to stable?

> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

with one nit below:

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

[snip]

> @@ -4476,15 +4459,37 @@ 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");
> -		rc = read(0, &c, 1);
> -		pmd_test_exit();
> -		if (rc < 0)
> -			return 1;
> +			printf("Press enter to exit\n");
> +
> +			FD_ZERO(&fds);
> +			FD_SET(0, &fds);
> +
> +			if (select(1, &fds, NULL, NULL, NULL) <= 0) {
> +				fprintf(stderr, "Select failed: %s\n",
> +					strerror(errno));
> +			} else if (read(0, &c, 1) <= 0) {
> +				fprintf(stderr,
> +					"Read stdin failed: %s\n",
> +					strerror(errno));
> +			}
> +		}
> +		stop_packet_forwarding();

force_quit() calls stop_packet_forwarding() if test_done is 0.
So, there is no difference in test_done == 0 case.
If test_done is not zero, stop_packet_forwarding() just logs
"Packet forwarding not started" and does nothing. So, the
difference is only in error message. Is it intentional?

> +		force_quit();
>   	}
>   
> +#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,

Re: [PATCH v5] testpmd: cleanup cleanly from signal

[Stephen Hemminger]

> > +		stop_packet_forwarding();  
> 
> force_quit() calls stop_packet_forwarding() if test_done is 0.
> So, there is no difference in test_done == 0 case.
> If test_done is not zero, stop_packet_forwarding() just logs
> "Packet forwarding not started" and does nothing. So, the
> difference is only in error message. Is it intentional?
> 
> > +		force_quit();
> >   	}

Will fix in new version, it was a logic error.

[PATCH v6] 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.

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

Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v5 - drop unnecessary print in signal handler.
     don't cleanup twice
     don't print message when select() is interrupted.

 app/test-pmd/testpmd.c | 72 ++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 37 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index cf5942d0c422..62d87f758ac8 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>
@@ -4224,13 +4225,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)
 {
@@ -4249,28 +4243,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
@@ -4449,9 +4424,6 @@ main(int argc, char** argv)
 	} else
 #endif
 	{
-		char c;
-		int rc;
-
 		f_quit = 0;
 
 		printf("No commandline core given, start packet forwarding\n");
@@ -4476,15 +4448,41 @@ main(int argc, char** argv)
 				prev_time = cur_time;
 				rte_delay_us_sleep(US_PER_S);
 			}
-		}
+		} else {
+			char c;
+			fd_set fds;
+			int rc;
 
-		printf("Press enter to exit\n");
-		rc = read(0, &c, 1);
-		pmd_test_exit();
-		if (rc < 0)
-			return 1;
+			printf("Press enter to exit\n");
+
+			FD_ZERO(&fds);
+			FD_SET(0, &fds);
+
+			rc = select(1, &fds, NULL, NULL, NULL);
+			if (rc < 0 && errno != EINTR) {
+				fprintf(stderr, "Select failed: %s\n",
+					strerror(errno));
+				return 1;
+			}
+			if (rc > 0)
+				rc = read(0, &c, 1);
+
+			pmd_test_exit();
+			if (rc < 0)
+				return 1;
+			prompt_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,
-- 
2.35.1

Re: [PATCH v6] testpmd: cleanup cleanly from signal

[Andrew Rybchenko]

On 11/10/22 19:53, Stephen Hemminger wrote:
> 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.
> 
> An added benefit is it gets rid of some Windows specific code.
> 
> Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v5 - drop unnecessary print in signal handler.
>       don't cleanup twice
>       don't print message when select() is interrupted.
> 
>   app/test-pmd/testpmd.c | 72 ++++++++++++++++++++----------------------
>   1 file changed, 35 insertions(+), 37 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index cf5942d0c422..62d87f758ac8 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>
> @@ -4224,13 +4225,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)
>   {
> @@ -4249,28 +4243,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
> @@ -4449,9 +4424,6 @@ main(int argc, char** argv)
>   	} else
>   #endif
>   	{
> -		char c;
> -		int rc;
> -
>   		f_quit = 0;
>   
>   		printf("No commandline core given, start packet forwarding\n");
> @@ -4476,15 +4448,41 @@ main(int argc, char** argv)
>   				prev_time = cur_time;
>   				rte_delay_us_sleep(US_PER_S);
>   			}
> -		}
> +		} else {
> +			char c;
> +			fd_set fds;
> +			int rc;
>   
> -		printf("Press enter to exit\n");
> -		rc = read(0, &c, 1);
> -		pmd_test_exit();
> -		if (rc < 0)
> -			return 1;
> +			printf("Press enter to exit\n");
> +
> +			FD_ZERO(&fds);
> +			FD_SET(0, &fds);
> +
> +			rc = select(1, &fds, NULL, NULL, NULL);
> +			if (rc < 0 && errno != EINTR) {
> +				fprintf(stderr, "Select failed: %s\n",
> +					strerror(errno));
> +				return 1;
> +			}
> +			if (rc > 0)
> +				rc = read(0, &c, 1);
> +
> +			pmd_test_exit();

It looks wrong to skip pmd_test_exit() in periodic stats
case (if body above). Earlier it happened in signal handler.
IMHO, pmd_test_exit() should be done just before pdump
uninit below (outside if/else bodies (and removed from
interactive == 1 branch above).

> +			if (rc < 0)
> +				return 1;

It took some time for me to understand what's happening here.
Finally I came to conclusion that it just preserve previous
behaviour to return with failure code immediately if read
fails. Except addition of pmd_test_exit() above.
I think it is a wrong behaviour to skip all cleanup which
is done below, but I agree that it is a separate issue to
fix.

> +			prompt_exit();

prompt_exit() is registered as atexit() callback and if I'm not
mistakes will be called anyway.

> +		}
>   	}
>   
> +#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,

Re: [PATCH v6] testpmd: cleanup cleanly from signal

[Stephen Hemminger]

On Fri, 11 Nov 2022 11:05:24 +0300
Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> wrote:

> It looks wrong to skip pmd_test_exit() in periodic stats
> case (if body above). Earlier it happened in signal handler.
> IMHO, pmd_test_exit() should be done just before pdump
> uninit below (outside if/else bodies (and removed from
> interactive == 1 branch above).
> 
> > +			if (rc < 0)
> > +				return 1;  
> 
> It took some time for me to understand what's happening here.
> Finally I came to conclusion that it just preserve previous
> behaviour to return with failure code immediately if read
> fails. Except addition of pmd_test_exit() above.
> I think it is a wrong behaviour to skip all cleanup which
> is done below, but I agree that it is a separate issue to
> fix.
> 
> > +			prompt_exit();  
> 
> prompt_exit() is registered as atexit() callback and if I'm not
> mistakes will be called anyway.


Good catch.
Trying to unwind the intentions of earlier code here.

Next version should be cleaner.

[PATCH v7] 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.

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

Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v5 - fix windows build
v6 - drop unneeded prprint in signal handler
v7 - cleanup logic around exiting, use rte_exit() for read/select errors

 app/test-pmd/testpmd.c | 67 +++++++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 36 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index cf5942d0c422..c012f9edbc99 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>
@@ -4224,13 +4225,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)
 {
@@ -4249,28 +4243,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
@@ -4449,9 +4424,6 @@ main(int argc, char** argv)
 	} else
 #endif
 	{
-		char c;
-		int rc;
-
 		f_quit = 0;
 
 		printf("No commandline core given, start packet forwarding\n");
@@ -4476,15 +4448,38 @@ 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");
+
+			FD_ZERO(&fds);
+			FD_SET(STDIN_FILENO, &fds);
 
-		printf("Press enter to exit\n");
-		rc = read(0, &c, 1);
+			ret = select(1, &fds, NULL, NULL, NULL);
+			if (ret < 0 && errno != EINTR)
+				rte_exit(EXIT_FAILURE,
+					 "Select failed: %s\n",
+					 strerror(errno));
+
+			if (ret == 1 && read(STDIN_FILENO, &c, 1) < 0)
+				rte_exit(EXIT_FAILURE,
+					 "Read failed: %s\n",
+					 strerror(errno));
+		}
 		pmd_test_exit();
-		if (rc < 0)
-			return 1;
 	}
 
+#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,
-- 
2.35.1

[PATCH v8] 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.

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

Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v8 - fix build on Windows (again)
     Windows doesn't have FILENO_STDIN

 app/test-pmd/testpmd.c | 67 +++++++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 36 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index aa7ea29f15ba..b4fb6a2bcbcf 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>
@@ -4224,13 +4225,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)
 {
@@ -4249,28 +4243,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
@@ -4449,9 +4424,6 @@ main(int argc, char** argv)
 	} else
 #endif
 	{
-		char c;
-		int rc;
-
 		f_quit = 0;
 
 		printf("No commandline core given, start packet forwarding\n");
@@ -4476,15 +4448,38 @@ 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");
+
+			FD_ZERO(&fds);
+			FD_SET(0, &fds);
 
-		printf("Press enter to exit\n");
-		rc = read(0, &c, 1);
+			ret = select(1, &fds, NULL, NULL, NULL);
+			if (ret < 0 && errno != EINTR)
+				rte_exit(EXIT_FAILURE,
+					 "Select failed: %s\n",
+					 strerror(errno));
+
+			if (ret == 1 && read(0, &c, 1) < 0)
+				rte_exit(EXIT_FAILURE,
+					 "Read failed: %s\n",
+					 strerror(errno));
+		}
 		pmd_test_exit();
-		if (rc < 0)
-			return 1;
 	}
 
+#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,
-- 
2.35.1

Re: [PATCH v8] testpmd: cleanup cleanly from signal

[Ferruh Yigit]

On 11/12/2022 5:28 PM, Stephen Hemminger wrote:
> 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.
> 
> An added benefit is it gets rid of some Windows specific code.
> 
> Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v8 - fix build on Windows (again)
>      Windows doesn't have FILENO_STDIN
> 
>  app/test-pmd/testpmd.c | 67 +++++++++++++++++++-----------------------
>  1 file changed, 31 insertions(+), 36 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index aa7ea29f15ba..b4fb6a2bcbcf 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>
> @@ -4224,13 +4225,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)
>  {
> @@ -4249,28 +4243,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;
>  }

Signal handler used for interactive mode too, that is why
'force_quit();' is still needed in signal handler.

Perhaps different signal handlers can be set for interactive and
non-interactive modes.

>  
>  int
> @@ -4449,9 +4424,6 @@ main(int argc, char** argv)
>  	} else
>  #endif
>  	{
> -		char c;
> -		int rc;
> -
>  		f_quit = 0;
>  
>  		printf("No commandline core given, start packet forwarding\n");
> @@ -4476,15 +4448,38 @@ 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");
> +
> +			FD_ZERO(&fds);
> +			FD_SET(0, &fds);
>  
> -		printf("Press enter to exit\n");
> -		rc = read(0, &c, 1);
> +			ret = select(1, &fds, NULL, NULL, NULL);
> +			if (ret < 0 && errno != EINTR)
> +				rte_exit(EXIT_FAILURE,
> +					 "Select failed: %s\n",
> +					 strerror(errno));
> +
> +			if (ret == 1 && read(0, &c, 1) < 0)
> +				rte_exit(EXIT_FAILURE,
> +					 "Read failed: %s\n",
> +					 strerror(errno));
> +		}
>  		pmd_test_exit();
> -		if (rc < 0)
> -			return 1;
>  	}
>  
> +#ifdef RTE_LIB_PDUMP
> +	/* uninitialize packet capture framework */
> +	rte_pdump_uninit();
> +#endif
> +#ifdef RTE_LIB_LATENCYSTATS
> +	if (latencystats_enabled != 0)
> +		rte_latencystats_uninit();
> +#endif

+1 to move these functions here, but signal handler for the interactive
mode also needs these functions.

> +
>  	ret = rte_eal_cleanup();
>  	if (ret != 0)
>  		rte_exit(EXIT_FAILURE,

[PATCH v9] 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.

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

Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v9 - also handle the interactive case.
     use cmdine_poll() rather than signals and atexit

 app/test-pmd/cmdline.c | 28 +++++----------
 app/test-pmd/testpmd.c | 77 ++++++++++++++++++++----------------------
 app/test-pmd/testpmd.h |  1 +
 3 files changed, 46 insertions(+), 60 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index b32dc8bfd445..fbe9ad694dee 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -65,7 +65,6 @@
 #include "cmdline_tm.h"
 #include "bpf_cmd.h"
 
-static struct cmdline *testpmd_cl;
 static cmdline_parse_ctx_t *main_ctx;
 static TAILQ_HEAD(, testpmd_driver_commands) driver_commands_head =
 	TAILQ_HEAD_INITIALIZER(driver_commands_head);
@@ -12921,28 +12920,19 @@ cmdline_read_from_file(const char *filename)
 void
 prompt(void)
 {
-	int ret;
+	struct cmdline *cl;
 
-	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");
-
-	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);
+	while (f_quit == 0 && cl_quit == 0) {
+		if (cmdline_poll(cl) < 0)
+			break;
 	}
+
+	cmdline_quit(cl);
+	cmdline_stdin_exit(cl);
 }
 
 void
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 134d79a55547..d01b6105b7de 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>
@@ -231,7 +232,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. */
 
 /*
@@ -4315,13 +4316,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)
 {
@@ -4340,28 +4334,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
@@ -4536,15 +4511,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) {
@@ -4567,15 +4536,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 7d24d25970d2..022210a7a964 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -34,6 +34,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.39.0

Re: [PATCH v9] testpmd: cleanup cleanly from signal

[Ferruh Yigit]

On 1/25/2023 6:32 PM, Stephen Hemminger wrote:
> 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.
> 
> An added benefit is it gets rid of some Windows specific code.
> 
> Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Patch looks good to me, but './devtools/test-null.sh' hangs with change.

It is possible the fix by updating './devtools/test-null.sh', but my
concern is if this behavior change hit more consumers other than this
internal tool, and I am for keeping previous behavior.

'./devtools/test-null.sh' sends 'stop' command to testpmd but that seems
not really what makes testpmd quit, because following change still works
with original testpmd code:
 -(sleep 1 && echo stop) |
 +(sleep 1 && echo help) |
Somehow testpmd gets Ctrl-D or equivalent with above command.

And it seems because of 'cmdline_interact()' and 'cmdline_poll()'
difference behavior changes with above command.

It is possible to add something like  'cmdline_interact_one()' (non loop
version of 'cmdline_interact()'), but not really sure that is correct
way to fix the issue.


> ---
> v9 - also handle the interactive case.
>      use cmdine_poll() rather than signals and atexit
> 
>  app/test-pmd/cmdline.c | 28 +++++----------
>  app/test-pmd/testpmd.c | 77 ++++++++++++++++++++----------------------
>  app/test-pmd/testpmd.h |  1 +
>  3 files changed, 46 insertions(+), 60 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index b32dc8bfd445..fbe9ad694dee 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -65,7 +65,6 @@
>  #include "cmdline_tm.h"
>  #include "bpf_cmd.h"
>  
> -static struct cmdline *testpmd_cl;
>  static cmdline_parse_ctx_t *main_ctx;
>  static TAILQ_HEAD(, testpmd_driver_commands) driver_commands_head =
>  	TAILQ_HEAD_INITIALIZER(driver_commands_head);
> @@ -12921,28 +12920,19 @@ cmdline_read_from_file(const char *filename)
>  void
>  prompt(void)
>  {
> -	int ret;
> +	struct cmdline *cl;
>  
> -	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");
> -
> -	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);
> +	while (f_quit == 0 && cl_quit == 0) {
> +		if (cmdline_poll(cl) < 0)
> +			break;
>  	}
> +
> +	cmdline_quit(cl);
> +	cmdline_stdin_exit(cl);
>  }
>  
>  void
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 134d79a55547..d01b6105b7de 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>
> @@ -231,7 +232,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. */
>  
>  /*
> @@ -4315,13 +4316,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)
>  {
> @@ -4340,28 +4334,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
> @@ -4536,15 +4511,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) {
> @@ -4567,15 +4536,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 7d24d25970d2..022210a7a964 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -34,6 +34,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.

Re: [PATCH v9] testpmd: cleanup cleanly from signal

[Stephen Hemminger]

On Mon, 30 Jan 2023 18:48:25 +0000
Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 1/25/2023 6:32 PM, Stephen Hemminger wrote:
> > 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.
> > 
> > An added benefit is it gets rid of some Windows specific code.
> > 
> > Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>  
> 
> Patch looks good to me, but './devtools/test-null.sh' hangs with change.
> 
> It is possible the fix by updating './devtools/test-null.sh', but my
> concern is if this behavior change hit more consumers other than this
> internal tool, and I am for keeping previous behavior.
> 
> './devtools/test-null.sh' sends 'stop' command to testpmd but that seems
> not really what makes testpmd quit, because following change still works
> with original testpmd code:
>  -(sleep 1 && echo stop) |
>  +(sleep 1 && echo help) |
> Somehow testpmd gets Ctrl-D or equivalent with above command.
> 
> And it seems because of 'cmdline_interact()' and 'cmdline_poll()'
> difference behavior changes with above command.
> 
> It is possible to add something like  'cmdline_interact_one()' (non loop
> version of 'cmdline_interact()'), but not really sure that is correct
> way to fix the issue.
> 

Thanks for the review.
Fixed the end-of-file handling in v10 of the patch.
The cmdline_poll() function doesn't handle EOF correctly.

The function cmdline_poll() documentation is problematic since it
references that the return value is an enum, but the enum is actually
private and not exported by the library!

Re: [RFC 1/2] testpmd: make f_quit flag volatile

[Andrew Rybchenko]

On 10/14/22 20:23, Stephen Hemminger wrote:
> Since f_quit is set in a signal handler it needs
> to be marked as volatile. Otherwise, compler is allowed

compler -> compiler

> to optimize away access to it.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

I need non-RFC version to apply it.

[PATCH v2] testpmd: make f_quit flag volatile

[Stephen Hemminger]

Since f_quit is set in a signal handler it needs to be marked
volatile.  Otherwise, compiler is allowed to optimize the loop because
it can assume the value never changes. The flag can also be made local
to the file it is used in.

Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v2 - not RFC and add fixes line

 app/test-pmd/testpmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index aa7ea29f15ba..cf5942d0c422 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -231,7 +231,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.
  */
-uint8_t f_quit;
+static volatile uint8_t f_quit;
 uint8_t cl_quit; /* Quit testpmd from cmdline. */
 
 /*
-- 
2.35.1

RE: [PATCH v2] testpmd: make f_quit flag volatile

[Ruifeng Wang]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, November 9, 2022 2:08 AM
> To: dev@dpdk.org
> Cc: Phil Yang <Phil.Yang@arm.com>; Stephen Hemminger <stephen@networkplumber.org>
> Subject: [PATCH v2] testpmd: make f_quit flag volatile
> 
> Since f_quit is set in a signal handler it needs to be marked volatile.  Otherwise,
> compiler is allowed to optimize the loop because it can assume the value never changes.
> The flag can also be made local to the file it is used in.
> 
> Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v2 - not RFC and add fixes line
> 
>  app/test-pmd/testpmd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> aa7ea29f15ba..cf5942d0c422 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -231,7 +231,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.
>   */
> -uint8_t f_quit;
> +static volatile uint8_t f_quit;
>  uint8_t cl_quit; /* Quit testpmd from cmdline. */
> 
>  /*
> --
> 2.35.1
Thanks for the change.
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>

Re: [PATCH v2] testpmd: make f_quit flag volatile

[Andrew Rybchenko]

On 11/9/22 13:11, Ruifeng Wang wrote:
>> -----Original Message-----
>> From: Stephen Hemminger <stephen@networkplumber.org>
>> Sent: Wednesday, November 9, 2022 2:08 AM
>> To: dev@dpdk.org
>> Cc: Phil Yang <Phil.Yang@arm.com>; Stephen Hemminger <stephen@networkplumber.org>
>> Subject: [PATCH v2] testpmd: make f_quit flag volatile
>>
>> Since f_quit is set in a signal handler it needs to be marked volatile.  Otherwise,
>> compiler is allowed to optimize the loop because it can assume the value never changes.
>> The flag can also be made local to the file it is used in.
>>
>> Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>> ---
>> v2 - not RFC and add fixes line
>>
>>   app/test-pmd/testpmd.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>> aa7ea29f15ba..cf5942d0c422 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -231,7 +231,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.
>>    */
>> -uint8_t f_quit;
>> +static volatile uint8_t f_quit;
>>   uint8_t cl_quit; /* Quit testpmd from cmdline. */
>>
>>   /*
>> --
>> 2.35.1
> Thanks for the change.
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> 

Applied to dpdk-next-net/main, thanks.

[PATCH v10 0/2] testpmd: handle signals safely

[Stephen Hemminger]

Fixing the signal handling in testpmd requires also
fixing EOF handling in cmdline_poll()

Stephen Hemminger (2):
  cmdline: handle EOF in cmdline_poll
  testpmd: cleanup cleanly from signal

 app/test-pmd/cmdline.c | 30 ++++++----------
 app/test-pmd/testpmd.c | 77 ++++++++++++++++++++----------------------
 app/test-pmd/testpmd.h |  1 +
 lib/cmdline/cmdline.c  |  2 +-
 4 files changed, 49 insertions(+), 61 deletions(-)

-- 
2.39.0

[PATCH v10 1/2] cmdline: handle EOF in cmdline_poll

[Stephen Hemminger]

If end of file is reached on input, then cmdline_read_char()
will return 0. The problem is that cmdline_poll() was not checking
for this and would continue and not return the status.

Fixes: 9251cd97a6be ("cmdline: add internal wrappers for character input")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/cmdline/cmdline.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c
index e1009ba4c413..de41406d61e0 100644
--- a/lib/cmdline/cmdline.c
+++ b/lib/cmdline/cmdline.c
@@ -194,7 +194,7 @@ cmdline_poll(struct cmdline *cl)
 	else if (status > 0) {
 		c = -1;
 		read_status = cmdline_read_char(cl, &c);
-		if (read_status < 0)
+		if (read_status <= 0)
 			return read_status;
 
 		status = cmdline_in(cl, &c, 1);
-- 
2.39.0

Re: [PATCH v10 1/2] cmdline: handle EOF in cmdline_poll

[Ferruh Yigit]

On 1/30/2023 8:09 PM, Stephen Hemminger wrote:
> If end of file is reached on input, then cmdline_read_char()
> will return 0. The problem is that cmdline_poll() was not checking
> for this and would continue and not return the status.
> 
> Fixes: 9251cd97a6be ("cmdline: add internal wrappers for character input")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/cmdline/cmdline.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c
> index e1009ba4c413..de41406d61e0 100644
> --- a/lib/cmdline/cmdline.c
> +++ b/lib/cmdline/cmdline.c
> @@ -194,7 +194,7 @@ cmdline_poll(struct cmdline *cl)
>  	else if (status > 0) {
>  		c = -1;
>  		read_status = cmdline_read_char(cl, &c);
> -		if (read_status < 0)
> +		if (read_status <= 0)
>  			return read_status;

According API doc it will be wrong to return '0', which imply 'RDLINE_INIT'.

But function may return any negative value on error, what about to get
eof as and error case:

if (read_status < 0)
	return read_status;
else if (read_status == 0)
	return -EIO;

With this 'cmdline_poll()' can be used in testpmd as it is used in v9 of
this patch:
while (f_quit == 0 && cl_quit == 0) {
	if (cmdline_poll(cl) < 0)
		break;
}



But still I guess this is an ABI break because of API behavior change.

Re: [PATCH v10 1/2] cmdline: handle EOF in cmdline_poll

[Stephen Hemminger]

On Mon, 30 Jan 2023 22:12:42 +0000
Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 1/30/2023 8:09 PM, Stephen Hemminger wrote:
> > If end of file is reached on input, then cmdline_read_char()
> > will return 0. The problem is that cmdline_poll() was not checking
> > for this and would continue and not return the status.
> > 
> > Fixes: 9251cd97a6be ("cmdline: add internal wrappers for character input")
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >  lib/cmdline/cmdline.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c
> > index e1009ba4c413..de41406d61e0 100644
> > --- a/lib/cmdline/cmdline.c
> > +++ b/lib/cmdline/cmdline.c
> > @@ -194,7 +194,7 @@ cmdline_poll(struct cmdline *cl)
> >  	else if (status > 0) {
> >  		c = -1;
> >  		read_status = cmdline_read_char(cl, &c);
> > -		if (read_status < 0)
> > +		if (read_status <= 0)
> >  			return read_status;  
> 
> According API doc it will be wrong to return '0', which imply 'RDLINE_INIT'.

The API doc is a mess. It says function returns things enum that is only
defined in cmdline_private.h. Therefore no application could safely depend
on it.

End of File is not an error in most API's.

[PATCH v10 2/2] 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.

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

Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test-pmd/cmdline.c | 30 ++++++----------
 app/test-pmd/testpmd.c | 77 ++++++++++++++++++++----------------------
 app/test-pmd/testpmd.h |  1 +
 3 files changed, 48 insertions(+), 60 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index b32dc8bfd445..1f73056eb19a 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -65,7 +65,6 @@
 #include "cmdline_tm.h"
 #include "bpf_cmd.h"
 
-static struct cmdline *testpmd_cl;
 static cmdline_parse_ctx_t *main_ctx;
 static TAILQ_HEAD(, testpmd_driver_commands) driver_commands_head =
 	TAILQ_HEAD_INITIALIZER(driver_commands_head);
@@ -12921,28 +12920,21 @@ cmdline_read_from_file(const char *filename)
 void
 prompt(void)
 {
-	int ret;
+	struct cmdline *cl;
 
-	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");
-
-	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);
+	/* loop until signal or quit command */
+	while (f_quit == 0 && cl_quit == 0) {
+		/* exit loop on eof or read error */
+		if (cmdline_poll(cl) <= 0)
+			break;
 	}
+
+	cmdline_quit(cl);
+	cmdline_stdin_exit(cl);
 }
 
 void
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 134d79a55547..d01b6105b7de 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>
@@ -231,7 +232,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. */
 
 /*
@@ -4315,13 +4316,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)
 {
@@ -4340,28 +4334,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
@@ -4536,15 +4511,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) {
@@ -4567,15 +4536,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 7d24d25970d2..022210a7a964 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -34,6 +34,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.39.0

Re: [PATCH v10 2/2] testpmd: cleanup cleanly from signal

[Ferruh Yigit]

On 1/30/2023 8:09 PM, Stephen Hemminger wrote:
> 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.
> 
> An added benefit is it gets rid of some Windows specific code.
> 
> Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

If cmdline commit is agreed on, testpmd changes looks good to me.

Re: [PATCH v10 0/2] testpmd: handle signals safely

[Ferruh Yigit]

On 1/30/2023 8:09 PM, Stephen Hemminger wrote:
> Fixing the signal handling in testpmd requires also
> fixing EOF handling in cmdline_poll()
> 
> Stephen Hemminger (2):
>   cmdline: handle EOF in cmdline_poll
>   testpmd: cleanup cleanly from signal
> 

I confirm this solves the EOF issue, but please check comment on the
cmdline library patch (1/2).

[PATCH v11 0/3] Fix cmdline_poll and testpmd signal handling

[Stephen Hemminger]

This patchset keeps uncovering bad practices in the cmdline library
around end of file and signal handling.

Stephen Hemminger (3):
  cmdline: make rdline status not private
  cmdline: handle EOF in cmdline_poll
  testpmd: cleanup cleanly from signal

 app/test-pmd/cmdline.c        | 29 +++++--------
 app/test-pmd/testpmd.c        | 77 ++++++++++++++++-------------------
 app/test-pmd/testpmd.h        |  1 +
 lib/cmdline/cmdline.c         | 11 +++--
 lib/cmdline/cmdline.h         |  6 +++
 lib/cmdline/cmdline_private.h |  6 ---
 6 files changed, 62 insertions(+), 68 deletions(-)

-- 
2.39.0

[PATCH v11 1/3] cmdline: make rdline status not private

[Stephen Hemminger]

The function cmdline_poll() returns values from rdline_status enum
but that was moved to being defined only in cmdline_private.h.

For proper use of the API the return value needs to be visible
to callers. This was not a problem before because cmdline_poll()
was not used anywhere.

Fixes: f8f8dc289095 ("cmdline: make struct rdline opaque")
Cc: dmitry.kozliuk@gmail.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/cmdline/cmdline.h         | 6 ++++++
 lib/cmdline/cmdline_private.h | 6 ------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/cmdline/cmdline.h b/lib/cmdline/cmdline.h
index 96674dfda224..b14355ef5121 100644
--- a/lib/cmdline/cmdline.h
+++ b/lib/cmdline/cmdline.h
@@ -23,6 +23,12 @@
 extern "C" {
 #endif
 
+enum rdline_status {
+	RDLINE_INIT,
+	RDLINE_RUNNING,
+	RDLINE_EXITED
+};
+
 struct cmdline;
 
 struct cmdline *cmdline_new(cmdline_parse_ctx_t *ctx, const char *prompt, int s_in, int s_out);
diff --git a/lib/cmdline/cmdline_private.h b/lib/cmdline/cmdline_private.h
index c2e906d8de6d..a3271c76934a 100644
--- a/lib/cmdline/cmdline_private.h
+++ b/lib/cmdline/cmdline_private.h
@@ -23,12 +23,6 @@
 #define RDLINE_HISTORY_BUF_SIZE BUFSIZ
 #define RDLINE_HISTORY_MAX_LINE 64
 
-enum rdline_status {
-	RDLINE_INIT,
-	RDLINE_RUNNING,
-	RDLINE_EXITED
-};
-
 struct rdline {
 	enum rdline_status status;
 	/* rdline bufs */
-- 
2.39.0

Re: [PATCH v11 1/3] cmdline: make rdline status not private

[fengchengwen]

Acked-by: Chengwen Feng <fengchengwen@huawei.com>

On 2023/2/4 3:14, Stephen Hemminger wrote:
> The function cmdline_poll() returns values from rdline_status enum
> but that was moved to being defined only in cmdline_private.h.
> 
> For proper use of the API the return value needs to be visible
> to callers. This was not a problem before because cmdline_poll()
> was not used anywhere.
> 
> Fixes: f8f8dc289095 ("cmdline: make struct rdline opaque")
> Cc: dmitry.kozliuk@gmail.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---

...

[PATCH v11 2/3] cmdline: handle EOF in cmdline_poll

[Stephen Hemminger]

If end of file is reached on input, then cmdline_poll() will
return 1 (ie file has something); and then the cmdline_in()
call to read will return 0. With the existing code,
caller has no way to tell that end of file has been reached
 and will retry forever.

A good way to handle this is to make end of file equivalent
to the quit command. Since no more input is possible at that
point.

Fixes: 067855e651d6 ("cmdline: add polling mode")
Cc: pawelx.wodkowski@intel.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
Note: cmdline_poll() should not have been added back
in 2015, looks like there is no users, and no tests for this.

 lib/cmdline/cmdline.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c
index e1009ba4c413..8ad0690d8533 100644
--- a/lib/cmdline/cmdline.c
+++ b/lib/cmdline/cmdline.c
@@ -197,9 +197,14 @@ cmdline_poll(struct cmdline *cl)
 		if (read_status < 0)
 			return read_status;
 
-		status = cmdline_in(cl, &c, 1);
-		if (status < 0 && cl->rdl.status != RDLINE_EXITED)
-			return status;
+		if (read_status == 0) {
+			/* end of file is implicit quit */
+			cmdline_quit(cl);
+		} else {
+			status = cmdline_in(cl, &c, 1);
+			if (status < 0 && cl->rdl.status != RDLINE_EXITED)
+				return status;
+		}
 	}
 
 	return cl->rdl.status;
-- 
2.39.0

[PATCH v11 3/3] 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.

Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test-pmd/cmdline.c | 29 ++++++----------
 app/test-pmd/testpmd.c | 77 ++++++++++++++++++++----------------------
 app/test-pmd/testpmd.h |  1 +
 3 files changed, 48 insertions(+), 59 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index cb8c174020b0..135c5c439bfc 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -66,7 +66,6 @@
 #include "cmdline_tm.h"
 #include "bpf_cmd.h"
 
-static struct cmdline *testpmd_cl;
 static cmdline_parse_ctx_t *main_ctx;
 static TAILQ_HEAD(, testpmd_driver_commands) driver_commands_head =
 	TAILQ_HEAD_INITIALIZER(driver_commands_head);
@@ -12936,28 +12935,22 @@ cmdline_read_from_file(const char *filename)
 void
 prompt(void)
 {
-	int ret;
+	struct cmdline *cl;
 
-	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");
-
-	cmdline_interact(testpmd_cl);
-	if (ret != 0)
-		cmdline_stdin_exit(testpmd_cl);
-}
+	/* loop until signal or quit command */
+	while (f_quit == 0 && cl_quit == 0) {
+		int status = cmdline_poll(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);
 }
 
 void
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index e366f81a0f46..60eb9579ded1 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>
@@ -231,7 +232,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. */
 
 /*
@@ -4315,13 +4316,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)
 {
@@ -4340,28 +4334,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
@@ -4541,15 +4516,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) {
@@ -4572,15 +4541,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 7d24d25970d2..022210a7a964 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -34,6 +34,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.39.0

Re: [PATCH v11 3/3] testpmd: cleanup cleanly from signal

[Ferruh Yigit]

On 2/3/2023 7:14 PM, Stephen Hemminger wrote:
> 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.
> 
> Fixes: d9a191a00e81 ("app/testpmd: fix quitting in container")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

Re: [PATCH v11 0/3] Fix cmdline_poll and testpmd signal handling

[Ferruh Yigit]

On 2/3/2023 7:14 PM, Stephen Hemminger wrote:
> This patchset keeps uncovering bad practices in the cmdline library
> around end of file and signal handling.
> 
> Stephen Hemminger (3):
>   cmdline: make rdline status not private
>   cmdline: handle EOF in cmdline_poll
>   testpmd: cleanup cleanly from signal

Tested-by: Ferruh Yigit <ferruh.yigit@amd.com>

Re: [PATCH v11 0/3] Fix cmdline_poll and testpmd signal handling

[Stephen Hemminger]

On Fri,  3 Feb 2023 11:14:06 -0800
Stephen Hemminger <stephen@networkplumber.org> wrote:

> This patchset keeps uncovering bad practices in the cmdline library
> around end of file and signal handling.
> 
> Stephen Hemminger (3):
>   cmdline: make rdline status not private
>   cmdline: handle EOF in cmdline_poll
>   testpmd: cleanup cleanly from signal
> 
>  app/test-pmd/cmdline.c        | 29 +++++--------
>  app/test-pmd/testpmd.c        | 77 ++++++++++++++++-------------------
>  app/test-pmd/testpmd.h        |  1 +
>  lib/cmdline/cmdline.c         | 11 +++--
>  lib/cmdline/cmdline.h         |  6 +++
>  lib/cmdline/cmdline_private.h |  6 ---
>  6 files changed, 62 insertions(+), 68 deletions(-)
> 

Could this please be merged for 23.03?
There are Ack's.
The only CI failure is a bogus performance test failure.

Re: [PATCH v11 0/3] Fix cmdline_poll and testpmd signal handling

[Thomas Monjalon]

19/02/2023 18:53, Stephen Hemminger:
> On Fri,  3 Feb 2023 11:14:06 -0800
> Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> > This patchset keeps uncovering bad practices in the cmdline library
> > around end of file and signal handling.
> > 
> > Stephen Hemminger (3):
> >   cmdline: make rdline status not private
> >   cmdline: handle EOF in cmdline_poll
> >   testpmd: cleanup cleanly from signal
> > 
> >  app/test-pmd/cmdline.c        | 29 +++++--------
> >  app/test-pmd/testpmd.c        | 77 ++++++++++++++++-------------------
> >  app/test-pmd/testpmd.h        |  1 +
> >  lib/cmdline/cmdline.c         | 11 +++--
> >  lib/cmdline/cmdline.h         |  6 +++
> >  lib/cmdline/cmdline_private.h |  6 ---
> >  6 files changed, 62 insertions(+), 68 deletions(-)
> > 
> 
> Could this please be merged for 23.03?
> There are Ack's.
> The only CI failure is a bogus performance test failure.

There was no review from testpmd maintainers.

I've added Cc: stable@dpdk.org.
Applied, thanks.

RE: [PATCH v11 0/3] Fix cmdline_poll and testpmd signal handling

[Tal Shnaiderman]

> Subject: Re: [PATCH v11 0/3] Fix cmdline_poll and testpmd signal handling
> 
> External email: Use caution opening links or attachments
> 
> 
> 19/02/2023 18:53, Stephen Hemminger:
> > On Fri,  3 Feb 2023 11:14:06 -0800
> > Stephen Hemminger <stephen@networkplumber.org> wrote:
> >	
> > > This patchset keeps uncovering bad practices in the cmdline library
> > > around end of file and signal handling.
> > >
> > > Stephen Hemminger (3):
> > >   cmdline: make rdline status not private
> > >   cmdline: handle EOF in cmdline_poll
> > >   testpmd: cleanup cleanly from signal
> > >
> > >  app/test-pmd/cmdline.c        | 29 +++++--------
> > >  app/test-pmd/testpmd.c        | 77 ++++++++++++++++-------------------
> > >  app/test-pmd/testpmd.h        |  1 +
> > >  lib/cmdline/cmdline.c         | 11 +++--
> > >  lib/cmdline/cmdline.h         |  6 +++
> > >  lib/cmdline/cmdline_private.h |  6 ---
> > >  6 files changed, 62 insertions(+), 68 deletions(-)
> > >
> >
> > Could this please be merged for 23.03?
> > There are Ack's.
> > The only CI failure is a bogus performance test failure.
> 
> There was no review from testpmd maintainers.
> 
> I've added Cc: stable@dpdk.org.
> Applied, thanks.
> 
Hi,

Commit "testpmd: cleanup cleanly from signal" from this series breaks TestPMD's interactive mode on Windows.

See https://bugs.dpdk.org/show_bug.cgi?id=1180

RE: [PATCH v11 0/3] Fix cmdline_poll and testpmd signal handling

[Ling, WeiX]

> -----Original Message-----
> From: Tal Shnaiderman <talshn@nvidia.com>
> Sent: Monday, March 13, 2023 1:18 AM
> To: NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
> Stephen Hemminger <stephen@networkplumber.org>; Pier Damouny
> <pdamouny@nvidia.com>
> Cc: dev@dpdk.org; stable@dpdk.org; ferruh.yigit@amd.com; Singh, Aman
> Deep <aman.deep.singh@intel.com>; Zhang, Yuying
> <yuying.zhang@intel.com>; Raslan Darawsheh <rasland@nvidia.com>
> Subject: RE: [PATCH v11 0/3] Fix cmdline_poll and testpmd signal handling
> 
> > Subject: Re: [PATCH v11 0/3] Fix cmdline_poll and testpmd signal
> > handling
> >
> > External email: Use caution opening links or attachments
> >
> >
> > 19/02/2023 18:53, Stephen Hemminger:
> > > On Fri,  3 Feb 2023 11:14:06 -0800
> > > Stephen Hemminger <stephen@networkplumber.org> wrote:
> > >
> > > > This patchset keeps uncovering bad practices in the cmdline
> > > > library around end of file and signal handling.
> > > >
> > > > Stephen Hemminger (3):
> > > >   cmdline: make rdline status not private
> > > >   cmdline: handle EOF in cmdline_poll
> > > >   testpmd: cleanup cleanly from signal
> > > >
> > > >  app/test-pmd/cmdline.c        | 29 +++++--------
> > > >  app/test-pmd/testpmd.c        | 77 ++++++++++++++++-------------------
> > > >  app/test-pmd/testpmd.h        |  1 +
> > > >  lib/cmdline/cmdline.c         | 11 +++--
> > > >  lib/cmdline/cmdline.h         |  6 +++
> > > >  lib/cmdline/cmdline_private.h |  6 ---
> > > >  6 files changed, 62 insertions(+), 68 deletions(-)
> > > >
> > >
> > > Could this please be merged for 23.03?
> > > There are Ack's.
> > > The only CI failure is a bogus performance test failure.
> >
> > There was no review from testpmd maintainers.
> >
> > I've added Cc: stable@dpdk.org.
> > Applied, thanks.
> >
> Hi,
> 
> Commit "testpmd: cleanup cleanly from signal" from this series breaks
> TestPMD's interactive mode on Windows.
> 
> See https://bugs.dpdk.org/show_bug.cgi?id=1180

Hi Stephen,

I found an issue based this commit(0fd1386c: app/testpmd: cleanup cleanly from signal).

The packets can't loop in 2 testpmd after start dpdk-pdump to capture packets Immediately (less than 1 second).

Steps:

1. Bind 1 CBDMA channel to vfio-pci, then start vhost-user as back-end:

x86_64-native-linuxapp-gcc/app/dpdk-testpmd -l 28-36 -n 4 -a 0000:80:04.0 --file-prefix=vhost   \
--vdev 'eth_vhost0,iface=vhost-net0,queues=8,client=1,\
dmas=[txq0@0000:80:04.0;txq1@0000:80:04.0;txq2@0000:80:04.0;txq3@0000:80:04.0;txq4@0000:80:04.0;txq5@0000:80:04.0;rxq2@0000:80:04.0;rxq3@0000:80:04.0;rxq4@0000:80:04.0;rxq5@0000:80:04.0;rxq6@0000:80:04.0;rxq7@0000:80:04.0]'
--iova=va -- -i --nb-cores=4 --rxq=8 --txq=8 --txd=1024 --rxd=1024

2. Start virtio-user as front-end:

x86_64-native-linuxapp-gcc/app/dpdk-testpmd -l 38-42 -n 4  --file-prefix=virtio-user0 --no-pci   \
--vdev=net_virtio_user0,mac=00:11:22:33:44:10,path=./vhost-net0,queues=8,mrg_rxbuf=1,in_order=1,packed_vq=1,server=1 \
-- -i --nb-cores=4 --rxq=8 --txq=8 --txd=1024 --rxd=1024
testpmd>set fwd csum
testpmd>start

3.Start dpdk-pdump to capture packets:

x86_64-native-linuxapp-gcc/app/dpdk-pdump  -v --file-prefix=virtio-user0 -- \
--pdump  'device_id=net_virtio_user0,queue=0,rx-dev=/root/dpdk/pdump-rx-q0.pcap,mbuf-size=8000' --pdump  \
'device_id=net_virtio_user0,queue=1,rx-dev=/root/dpdk/pdump-rx-q1.pcap,mbuf-size=8000'

4.Set forwarding mode and send packets from vhost-user(execute this step must immediately, we use the automation script to do, it can be reproduced, and if I add time.sleep(1) before this step, it works well):

testpmd>set fwd mac
testpmd>set txpkts 64,64,64,2000,2000,2000
testpmd>set burst 1
testpmd>start tx_first 1
testpmd>show port stats 0

And I try to modify the follows code, then re-build DPDK, it works well. Maybe it's not a good method, just for your reference.

diff --git a/lib/cmdline/cmdline_os_unix.c b/lib/cmdline/cmdline_os_unix.c
index 64a945a34f..ede8289244 100644
--- a/lib/cmdline/cmdline_os_unix.c
+++ b/lib/cmdline/cmdline_os_unix.c
@@ -37,7 +37,7 @@ cmdline_poll_char(struct cmdline *cl)
        pfd.events = POLLIN;
        pfd.revents = 0; -       return poll(&pfd, 1, 0);
+       return poll(&pfd, 1, -1);
} ssize_t

Regards,
Wei Ling

Re: [PATCH v11 0/3] Fix cmdline_poll and testpmd signal handling

[Stephen Hemminger]

On Mon, 13 Mar 2023 10:34:55 +0000
"Ling, WeiX" <weix.ling@intel.com> wrote:

> > -----Original Message-----
> > From: Tal Shnaiderman <talshn@nvidia.com>
> > Sent: Monday, March 13, 2023 1:18 AM
> > To: NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
> > Stephen Hemminger <stephen@networkplumber.org>; Pier Damouny
> > <pdamouny@nvidia.com>
> > Cc: dev@dpdk.org; stable@dpdk.org; ferruh.yigit@amd.com; Singh, Aman
> > Deep <aman.deep.singh@intel.com>; Zhang, Yuying
> > <yuying.zhang@intel.com>; Raslan Darawsheh <rasland@nvidia.com>
> > Subject: RE: [PATCH v11 0/3] Fix cmdline_poll and testpmd signal handling
> >   
> > > Subject: Re: [PATCH v11 0/3] Fix cmdline_poll and testpmd signal
> > > handling
> > >
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > 19/02/2023 18:53, Stephen Hemminger:  
> > > > On Fri,  3 Feb 2023 11:14:06 -0800
> > > > Stephen Hemminger <stephen@networkplumber.org> wrote:
> > > >  
> > > > > This patchset keeps uncovering bad practices in the cmdline
> > > > > library around end of file and signal handling.
> > > > >
> > > > > Stephen Hemminger (3):
> > > > >   cmdline: make rdline status not private
> > > > >   cmdline: handle EOF in cmdline_poll
> > > > >   testpmd: cleanup cleanly from signal
> > > > >
> > > > >  app/test-pmd/cmdline.c        | 29 +++++--------
> > > > >  app/test-pmd/testpmd.c        | 77 ++++++++++++++++-------------------
> > > > >  app/test-pmd/testpmd.h        |  1 +
> > > > >  lib/cmdline/cmdline.c         | 11 +++--
> > > > >  lib/cmdline/cmdline.h         |  6 +++
> > > > >  lib/cmdline/cmdline_private.h |  6 ---
> > > > >  6 files changed, 62 insertions(+), 68 deletions(-)
> > > > >  
> > > >
> > > > Could this please be merged for 23.03?
> > > > There are Ack's.
> > > > The only CI failure is a bogus performance test failure.  
> > >
> > > There was no review from testpmd maintainers.
> > >
> > > I've added Cc: stable@dpdk.org.
> > > Applied, thanks.
> > >  
> > Hi,
> > 
> > Commit "testpmd: cleanup cleanly from signal" from this series breaks
> > TestPMD's interactive mode on Windows.
> > 
> > See https://bugs.dpdk.org/show_bug.cgi?id=1180  
> 
> Hi Stephen,
> 
> I found an issue based this commit(0fd1386c: app/testpmd: cleanup cleanly from signal).
> 
> The packets can't loop in 2 testpmd after start dpdk-pdump to capture packets Immediately (less than 1 second).
> 
> Steps:
> 
> 1. Bind 1 CBDMA channel to vfio-pci, then start vhost-user as back-end:
> 
> x86_64-native-linuxapp-gcc/app/dpdk-testpmd -l 28-36 -n 4 -a 0000:80:04.0 --file-prefix=vhost   \
> --vdev 'eth_vhost0,iface=vhost-net0,queues=8,client=1,\
> dmas=[txq0@0000:80:04.0;txq1@0000:80:04.0;txq2@0000:80:04.0;txq3@0000:80:04.0;txq4@0000:80:04.0;txq5@0000:80:04.0;rxq2@0000:80:04.0;rxq3@0000:80:04.0;rxq4@0000:80:04.0;rxq5@0000:80:04.0;rxq6@0000:80:04.0;rxq7@0000:80:04.0]'
> --iova=va -- -i --nb-cores=4 --rxq=8 --txq=8 --txd=1024 --rxd=1024
> 
> 2. Start virtio-user as front-end:
> 
> x86_64-native-linuxapp-gcc/app/dpdk-testpmd -l 38-42 -n 4  --file-prefix=virtio-user0 --no-pci   \
> --vdev=net_virtio_user0,mac=00:11:22:33:44:10,path=./vhost-net0,queues=8,mrg_rxbuf=1,in_order=1,packed_vq=1,server=1 \
> -- -i --nb-cores=4 --rxq=8 --txq=8 --txd=1024 --rxd=1024
> testpmd>set fwd csum
> testpmd>start  
> 
> 3.Start dpdk-pdump to capture packets:
> 
> x86_64-native-linuxapp-gcc/app/dpdk-pdump  -v --file-prefix=virtio-user0 -- \
> --pdump  'device_id=net_virtio_user0,queue=0,rx-dev=/root/dpdk/pdump-rx-q0.pcap,mbuf-size=8000' --pdump  \
> 'device_id=net_virtio_user0,queue=1,rx-dev=/root/dpdk/pdump-rx-q1.pcap,mbuf-size=8000'
> 
> 4.Set forwarding mode and send packets from vhost-user(execute this step must immediately, we use the automation script to do, it can be reproduced, and if I add time.sleep(1) before this step, it works well):
> 
> testpmd>set fwd mac
> testpmd>set txpkts 64,64,64,2000,2000,2000
> testpmd>set burst 1
> testpmd>start tx_first 1
> testpmd>show port stats 0  
> 
> And I try to modify the follows code, then re-build DPDK, it works well. Maybe it's not a good method, just for your reference.
> 
> diff --git a/lib/cmdline/cmdline_os_unix.c b/lib/cmdline/cmdline_os_unix.c
> index 64a945a34f..ede8289244 100644
> --- a/lib/cmdline/cmdline_os_unix.c
> +++ b/lib/cmdline/cmdline_os_unix.c
> @@ -37,7 +37,7 @@ cmdline_poll_char(struct cmdline *cl)
>         pfd.events = POLLIN;
>         pfd.revents = 0; -       return poll(&pfd, 1, 0);
> +       return poll(&pfd, 1, -1);
> } ssize_t


Thanks, cmdline_poll() existed a long time but was never used by any part of DPDK
until now. My preference is to get the old cmdline_read_char() working and just
remove it.

RE: [PATCH v11 0/3] Fix cmdline_poll and testpmd signal handling

[Ling, WeiX]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Monday, March 13, 2023 11:53 PM
> To: Ling, WeiX <weix.ling@intel.com>
> Cc: Tal Shnaiderman <talshn@nvidia.com>; NBU-Contact-Thomas Monjalon
> (EXTERNAL) <thomas@monjalon.net>; Pier Damouny
> <pdamouny@nvidia.com>; dev@dpdk.org; stable@dpdk.org;
> ferruh.yigit@amd.com; Singh, Aman Deep <aman.deep.singh@intel.com>;
> Zhang, Yuying <yuying.zhang@intel.com>; Raslan Darawsheh
> <rasland@nvidia.com>
> Subject: Re: [PATCH v11 0/3] Fix cmdline_poll and testpmd signal handling
> 
> On Mon, 13 Mar 2023 10:34:55 +0000
> "Ling, WeiX" <weix.ling@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Tal Shnaiderman <talshn@nvidia.com>
> > > Sent: Monday, March 13, 2023 1:18 AM
> > > To: NBU-Contact-Thomas Monjalon (EXTERNAL)
> <thomas@monjalon.net>;
> > > Stephen Hemminger <stephen@networkplumber.org>; Pier Damouny
> > > <pdamouny@nvidia.com>
> > > Cc: dev@dpdk.org; stable@dpdk.org; ferruh.yigit@amd.com; Singh,
> Aman
> > > Deep <aman.deep.singh@intel.com>; Zhang, Yuying
> > > <yuying.zhang@intel.com>; Raslan Darawsheh <rasland@nvidia.com>
> > > Subject: RE: [PATCH v11 0/3] Fix cmdline_poll and testpmd signal
> > > handling
> > >
> > > > Subject: Re: [PATCH v11 0/3] Fix cmdline_poll and testpmd signal
> > > > handling
> > > >
> > > > External email: Use caution opening links or attachments
> > > >
> > > >
> > > > 19/02/2023 18:53, Stephen Hemminger:
> > > > > On Fri,  3 Feb 2023 11:14:06 -0800 Stephen Hemminger
> > > > > <stephen@networkplumber.org> wrote:
> > > > >
> > > > > > This patchset keeps uncovering bad practices in the cmdline
> > > > > > library around end of file and signal handling.
> > > > > >
> > > > > > Stephen Hemminger (3):
> > > > > >   cmdline: make rdline status not private
> > > > > >   cmdline: handle EOF in cmdline_poll
> > > > > >   testpmd: cleanup cleanly from signal
> > > > > >
> > > > > >  app/test-pmd/cmdline.c        | 29 +++++--------
> > > > > >  app/test-pmd/testpmd.c        | 77 ++++++++++++++++---------------
> ----
> > > > > >  app/test-pmd/testpmd.h        |  1 +
> > > > > >  lib/cmdline/cmdline.c         | 11 +++--
> > > > > >  lib/cmdline/cmdline.h         |  6 +++
> > > > > >  lib/cmdline/cmdline_private.h |  6 ---
> > > > > >  6 files changed, 62 insertions(+), 68 deletions(-)
> > > > > >
> > > > >
> > > > > Could this please be merged for 23.03?
> > > > > There are Ack's.
> > > > > The only CI failure is a bogus performance test failure.
> > > >
> > > > There was no review from testpmd maintainers.
> > > >
> > > > I've added Cc: stable@dpdk.org.
> > > > Applied, thanks.
> > > >
> > > Hi,
> > >
> > > Commit "testpmd: cleanup cleanly from signal" from this series
> > > breaks TestPMD's interactive mode on Windows.
> > >
> > > See https://bugs.dpdk.org/show_bug.cgi?id=1180
> >
> > Hi Stephen,
> >
> > I found an issue based this commit(0fd1386c: app/testpmd: cleanup cleanly
> from signal).
> >
> > The packets can't loop in 2 testpmd after start dpdk-pdump to capture
> packets Immediately (less than 1 second).
> >
> > Steps:
> >
> > 1. Bind 1 CBDMA channel to vfio-pci, then start vhost-user as back-end:
> >
> > x86_64-native-linuxapp-gcc/app/dpdk-testpmd -l 28-36 -n 4 -a 0000:80:04.0
> --file-prefix=vhost   \
> > --vdev 'eth_vhost0,iface=vhost-net0,queues=8,client=1,\
> >
> dmas=[txq0@0000:80:04.0;txq1@0000:80:04.0;txq2@0000:80:04.0;txq3@000
> 0:80:04.0;txq4@0000:80:04.0;txq5@0000:80:04.0;rxq2@0000:80:04.0;rxq3@00
> 00:80:04.0;rxq4@0000:80:04.0;rxq5@0000:80:04.0;rxq6@0000:80:04.0;rxq7@0
> 000:80:04.0]'
> > --iova=va -- -i --nb-cores=4 --rxq=8 --txq=8 --txd=1024 --rxd=1024
> >
> > 2. Start virtio-user as front-end:
> >
> > x86_64-native-linuxapp-gcc/app/dpdk-testpmd -l 38-42 -n 4  --file-
> prefix=virtio-user0 --no-pci   \
> > --vdev=net_virtio_user0,mac=00:11:22:33:44:10,path=./vhost-net0,queues
> > =8,mrg_rxbuf=1,in_order=1,packed_vq=1,server=1 \
> > -- -i --nb-cores=4 --rxq=8 --txq=8 --txd=1024 --rxd=1024
> > testpmd>set fwd csum
> > testpmd>start
> >
> > 3.Start dpdk-pdump to capture packets:
> >
> > x86_64-native-linuxapp-gcc/app/dpdk-pdump  -v
> > --file-prefix=virtio-user0 -- \ --pdump
> > 'device_id=net_virtio_user0,queue=0,rx-dev=/root/dpdk/pdump-rx-
> q0.pcap,mbuf-size=8000' --pdump  \
> 'device_id=net_virtio_user0,queue=1,rx-dev=/root/dpdk/pdump-rx-
> q1.pcap,mbuf-size=8000'
> >
> > 4.Set forwarding mode and send packets from vhost-user(execute this
> step must immediately, we use the automation script to do, it can be
> reproduced, and if I add time.sleep(1) before this step, it works well):
> >
> > testpmd>set fwd mac
> > testpmd>set txpkts 64,64,64,2000,2000,2000 set burst 1 start tx_first
> > testpmd>1 show port stats 0
> >
> > And I try to modify the follows code, then re-build DPDK, it works well.
> Maybe it's not a good method, just for your reference.
> >
> > diff --git a/lib/cmdline/cmdline_os_unix.c
> > b/lib/cmdline/cmdline_os_unix.c index 64a945a34f..ede8289244 100644
> > --- a/lib/cmdline/cmdline_os_unix.c
> > +++ b/lib/cmdline/cmdline_os_unix.c
> > @@ -37,7 +37,7 @@ cmdline_poll_char(struct cmdline *cl)
> >         pfd.events = POLLIN;
> >         pfd.revents = 0; -       return poll(&pfd, 1, 0);
> > +       return poll(&pfd, 1, -1);
> > } ssize_t
> 
> 
> Thanks, cmdline_poll() existed a long time but was never used by any part of
> DPDK until now. My preference is to get the old cmdline_read_char()
> working and just remove it.

Hi Stephen,

I have submit a Bugzilla bug: https://bugs.dpdk.org/show_bug.cgi?id=1181 to track this issue.

And I have verified based on DPDK23.03-rc2(baf13c3135) with this you new patch

(https://patches.dpdk.org/project/dpdk/patch/20230313213831.80071-1-stephen@networkplumber.org/) PASSED.

OS: Ubuntu 22.04.1 LTS/Linux 5.15.45-051545-generic

Regards,
Wei Ling