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 <step...@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,
