From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 94119A04FD; Thu, 10 Nov 2022 08:50:44 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 893D340156; Thu, 10 Nov 2022 08:50:44 +0100 (CET) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 521FF40151 for ; Thu, 10 Nov 2022 08:50:43 +0100 (CET) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 142261B0BF for ; Thu, 10 Nov 2022 08:50:43 +0100 (CET) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 12F401B1AB; Thu, 10 Nov 2022 08:50:43 +0100 (CET) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-1.5 required=5.0 tests=ALL_TRUSTED, AWL, NICE_REPLY_A autolearn=disabled version=3.4.6 X-Spam-Score: -1.5 Received: from [192.168.1.59] (h-62-63-215-114.A163.priv.bahnhof.se [62.63.215.114]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 5B94D1AFB7; Thu, 10 Nov 2022 08:50:41 +0100 (CET) Message-ID: Date: Thu, 10 Nov 2022 08:50:40 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.2 Subject: Re: [PATCH v4] testpmd: cleanup cleanly from signal To: Stephen Hemminger Cc: dev@dpdk.org, phil.yang@arm.com References: <20221014172328.185219-2-stephen@networkplumber.org> <20221109041046.199840-1-stephen@networkplumber.org> <097a2759-f958-84ed-0ddf-4b23eb1eee04@lysator.liu.se> <20221109145323.7ac2a1b4@hermes.local> Content-Language: en-US From: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= In-Reply-To: <20221109145323.7ac2a1b4@hermes.local> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 2022-11-09 23:53, Stephen Hemminger wrote: > On Wed, 9 Nov 2022 22:46:55 +0100 > Mattias Rönnblom 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 >>> --- >>> 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 >>> #endif >>> +#include >>> #include >>> #include >>> #include >>> @@ -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.)