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 1B2A1A0542; Fri, 11 Nov 2022 09:05:27 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F3BB340143; Fri, 11 Nov 2022 09:05:26 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id DCCFA40141 for ; Fri, 11 Nov 2022 09:05:25 +0100 (CET) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 3EB877D; Fri, 11 Nov 2022 11:05:25 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 3EB877D DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1668153925; bh=qNf7gigiE3lxielnUGHY01TVNo6GWR3oo3Kc2iswnWo=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=H8fFzmheFXnKjr0DapJ/wVlYufSIQ22/o2YpiAXkXRqsAaWNHlIzJPyn3aBVTm0Qs KOLZfH2exNu1BVYeXDNVlB+IB49xJuNnLrT9UcZwu3ETNWIXLWyOP//Z9GKvk+qaEo cIrXQuhbLEegfWOno7iWeVxMUFlFo3Bsbk47Sxvk= Message-ID: <39978e88-a231-9ef4-197e-ee1948d9c8ef@oktetlabs.ru> Date: Fri, 11 Nov 2022 11:05:24 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.0 Subject: Re: [PATCH v6] testpmd: cleanup cleanly from signal Content-Language: en-US To: Stephen Hemminger , dev@dpdk.org Cc: Aman Singh , Yuying Zhang , Phil Yang , Jianbo Liu References: <20221014172328.185219-2-stephen@networkplumber.org> <20221110165359.343010-1-stephen@networkplumber.org> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <20221110165359.343010-1-stephen@networkplumber.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 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 > --- > 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 > #ifndef RTE_EXEC_ENV_WINDOWS > #include > +#include > #endif > #include > #include > @@ -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,