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

[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