[PATCH v2 0/3] Handle SIGINT and SIGTERM in DPDK examples

[PATCH v2 0/3] Handle SIGINT and SIGTERM in DPDK examples

[Zhihong Wang]

This patch handles SIGINT and SIGTERM in testpmd, l2fwd, and l3fwd, make sure all ports are properly stopped and closed.
For virtual ports, the stop and close function may deal with resource cleanup, such as socket files unlinking.

--------------
Changes in v2:

1. Make sure graceful exit for all running phases

2. Make sure program exits with the right status

Zhihong Wang (3):
  app/test-pmd: Handle SIGINT and SIGTERM in testpmd
  examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd
  examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

 app/test-pmd/cmdline.c |  19 ++++++---
 app/test-pmd/testpmd.c |  38 ++++++++++++++---
 app/test-pmd/testpmd.h |   1 +
 examples/l2fwd/main.c  |  60 +++++++++++++++++++++++++++
 examples/l3fwd/main.c  | 110 ++++++++++++++++++++++++++++++++++++++++---------
 5 files changed, 196 insertions(+), 32 deletions(-)

-- 
2.5.0

[PATCH v2 1/3] app/test-pmd: Handle SIGINT and SIGTERM in testpmd

[Zhihong Wang]

Handle SIGINT and SIGTERM in testpmd.

Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
---
 app/test-pmd/cmdline.c | 19 +++++++++++++------
 app/test-pmd/testpmd.c | 38 ++++++++++++++++++++++++++++++++------
 app/test-pmd/testpmd.h |  1 +
 3 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 73298c9..4ff1739 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -90,6 +90,8 @@
 
 #include "testpmd.h"
 
+static struct cmdline *testpmd_cl;
+
 static void cmd_reconfig_device_queue(portid_t id, uint8_t dev, uint8_t queue);
 
 #ifdef RTE_NIC_BYPASS
@@ -9778,17 +9780,22 @@ cmdline_parse_ctx_t main_ctx[] = {
 void
 prompt(void)
 {
-	struct cmdline *cl;
-
 	/* initialize non-constant commands */
 	cmd_set_fwd_mode_init();
 
-	cl = cmdline_stdin_new(main_ctx, "testpmd> ");
-	if (cl == NULL) {
+	testpmd_cl = cmdline_stdin_new(main_ctx, "testpmd> ");
+	if (testpmd_cl == NULL) {
 		return;
 	}
-	cmdline_interact(cl);
-	cmdline_stdin_exit(cl);
+	cmdline_interact(testpmd_cl);
+	cmdline_stdin_exit(testpmd_cl);
+}
+
+void
+prompt_exit(void)
+{
+	if (testpmd_cl != NULL)
+		cmdline_quit(testpmd_cl);
 }
 
 static void
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 98ae46d..cb38d56 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1570,13 +1570,16 @@ pmd_test_exit(void)
 	if (test_done == 0)
 		stop_packet_forwarding();
 
-	FOREACH_PORT(pt_id, ports) {
-		printf("Stopping port %d...", pt_id);
-		fflush(stdout);
-		rte_eth_dev_close(pt_id);
-		printf("done\n");
+	if (ports != NULL) {
+		FOREACH_PORT(pt_id, ports) {
+			printf("Stopping port %d...", pt_id);
+			fflush(stdout);
+			rte_eth_dev_stop(pt_id);
+			rte_eth_dev_close(pt_id);
+			printf(" Done\n");
+		}
 	}
-	printf("bye...\n");
+	printf("Bye...\n");
 }
 
 typedef void (*cmd_func_t)(void);
@@ -1984,12 +1987,34 @@ init_port(void)
 		ports[pid].enabled = 1;
 }
 
+static void
+force_quit(void)
+{
+	pmd_test_exit();
+	prompt_exit();
+}
+
+static void
+sigint_handler(__rte_unused int signum)
+{
+	if (signum == SIGINT || signum == SIGTERM) {
+		printf("\nSignal %d received, preparing to exit...\n",
+				signum);
+		force_quit();
+		signal(signum, SIG_DFL);
+		kill(getpid(), signum);
+	}
+}
+
 int
 main(int argc, char** argv)
 {
 	int  diag;
 	uint8_t port_id;
 
+	signal(SIGINT, sigint_handler);
+	signal(SIGTERM, sigint_handler);
+
 	diag = rte_eal_init(argc, argv);
 	if (diag < 0)
 		rte_panic("Cannot init EAL\n");
@@ -2041,6 +2066,7 @@ main(int argc, char** argv)
 		start_packet_forwarding(0);
 		printf("Press enter to exit\n");
 		rc = read(0, &c, 1);
+		pmd_test_exit();
 		if (rc < 0)
 			return 1;
 	}
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index ee7de98..7ffc17b 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -462,6 +462,7 @@ unsigned int parse_item_list(char* str, const char* item_name,
 			unsigned int *parsed_items, int check_unique_values);
 void launch_args_parse(int argc, char** argv);
 void prompt(void);
+void prompt_exit(void);
 void nic_stats_display(portid_t port_id);
 void nic_stats_clear(portid_t port_id);
 void nic_xstats_display(portid_t port_id);
-- 
2.5.0

[PATCH v2 2/3] examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd

[Zhihong Wang]

Handle SIGINT and SIGTERM in l2fwd.

Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
---
 examples/l2fwd/main.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/examples/l2fwd/main.c b/examples/l2fwd/main.c
index 720fd5a..75899dd 100644
--- a/examples/l2fwd/main.c
+++ b/examples/l2fwd/main.c
@@ -44,6 +44,8 @@
 #include <ctype.h>
 #include <errno.h>
 #include <getopt.h>
+#include <signal.h>
+#include <unistd.h>
 
 #include <rte_common.h>
 #include <rte_log.h>
@@ -69,6 +71,9 @@
 #include <rte_mempool.h>
 #include <rte_mbuf.h>
 
+static int force_quit = -1;
+static int signo_quit = -1;
+
 #define RTE_LOGTYPE_L2FWD RTE_LOGTYPE_USER1
 
 #define NB_MBUF   8192
@@ -284,6 +289,8 @@ l2fwd_main_loop(void)
 	}
 
 	while (1) {
+		if (unlikely(force_quit != 0))
+			break;
 
 		cur_tsc = rte_rdtsc();
 
@@ -534,6 +541,45 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
 	}
 }
 
+static void
+stop_ports(void)
+{
+	unsigned portid, nb_ports;
+
+	nb_ports = rte_eth_dev_count();
+	for (portid = 0; portid < nb_ports; portid++) {
+		if ((l2fwd_enabled_port_mask & (1 << portid)) == 0) {
+			continue;
+		}
+		printf("Stopping port %d...", portid);
+		rte_eth_dev_stop(portid);
+		rte_eth_dev_close(portid);
+		printf(" Done\n");
+	}
+}
+
+static void
+signal_handler(__rte_unused int signum)
+{
+	if (signum == SIGINT || signum == SIGTERM) {
+		printf("\nSignal %d received, preparing to exit...\n",
+				signum);
+		if (force_quit < 0) {
+			printf("Forwarding not started yet...\n");
+			/* stop ports */
+			stop_ports();
+			printf("Bye...\n");
+			/* inform if there's a caller */
+			signal(signum, SIG_DFL);
+			kill(getpid(), signum);
+		} else {
+			printf("Forwarding started already...\n");
+			signo_quit = signum;
+			force_quit = 1;
+		}
+	}
+}
+
 int
 main(int argc, char **argv)
 {
@@ -546,6 +592,9 @@ main(int argc, char **argv)
 	unsigned lcore_id, rx_lcore_id;
 	unsigned nb_ports_in_mask = 0;
 
+	signal(SIGINT, signal_handler);
+	signal(SIGTERM, signal_handler);
+
 	/* init EAL */
 	ret = rte_eal_init(argc, argv);
 	if (ret < 0)
@@ -697,11 +746,22 @@ main(int argc, char **argv)
 	check_all_ports_link_status(nb_ports, l2fwd_enabled_port_mask);
 
 	/* launch per-lcore init on every lcore */
+	force_quit = 0;
 	rte_eal_mp_remote_launch(l2fwd_launch_one_lcore, NULL, CALL_MASTER);
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
 		if (rte_eal_wait_lcore(lcore_id) < 0)
 			return -1;
 	}
 
+	printf("Stopping forwarding... Done\n");
+	/* stop ports */
+	stop_ports();
+	printf("Bye...\n");
+	/* inform if there's a caller */
+	if (force_quit != 0) {
+		signal(signo_quit, SIG_DFL);
+		kill(getpid(), signo_quit);
+	}
+
 	return 0;
 }
-- 
2.5.0

[PATCH v2 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

[Zhihong Wang]

Handle SIGINT and SIGTERM in l3fwd.

Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
---
 examples/l3fwd/main.c | 110 +++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 90 insertions(+), 20 deletions(-)

diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
index 5b0c2dd..b9f3232 100644
--- a/examples/l3fwd/main.c
+++ b/examples/l3fwd/main.c
@@ -41,6 +41,8 @@
 #include <stdarg.h>
 #include <errno.h>
 #include <getopt.h>
+#include <signal.h>
+#include <unistd.h>
 
 #include <rte_common.h>
 #include <rte_vect.h>
@@ -75,6 +77,9 @@
 #include <cmdline_parse.h>
 #include <cmdline_parse_etheraddr.h>
 
+static int force_quit = -1;
+static int signo_quit = -1;
+
 #define APP_LOOKUP_EXACT_MATCH          0
 #define APP_LOOKUP_LPM                  1
 #define DO_RFC_1812_CHECKS
@@ -1554,6 +1559,8 @@ main_loop(__attribute__((unused)) void *dummy)
 	}
 
 	while (1) {
+		if (unlikely(force_quit != 0))
+			break;
 
 		cur_tsc = rte_rdtsc();
 
@@ -2559,6 +2566,74 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
 	}
 }
 
+static void
+start_ports(void)
+{
+	unsigned portid, nb_ports;
+	int ret;
+
+	nb_ports = rte_eth_dev_count();
+	for (portid = 0; portid < nb_ports; portid++) {
+		if ((enabled_port_mask & (1 << portid)) == 0) {
+			continue;
+		}
+		printf("Starting port %d...", portid);
+		ret = rte_eth_dev_start(portid);
+		if (ret < 0)
+			rte_exit(EXIT_FAILURE,
+					"rte_eth_dev_start: err=%d, port=%d\n",
+					ret, portid);
+		/*
+		 * If enabled, put device in promiscuous mode.
+		 * This allows IO forwarding mode to forward packets
+		 * to itself through 2 cross-connected  ports of the
+		 * target machine.
+		 */
+		if (promiscuous_on)
+			rte_eth_promiscuous_enable(portid);
+		printf(" Done\n");
+	}
+}
+
+static void
+stop_ports(void)
+{
+	unsigned portid, nb_ports;
+
+	nb_ports = rte_eth_dev_count();
+	for (portid = 0; portid < nb_ports; portid++) {
+		if ((enabled_port_mask & (1 << portid)) == 0) {
+			continue;
+		}
+		printf("Stopping port %d...", portid);
+		rte_eth_dev_stop(portid);
+		rte_eth_dev_close(portid);
+		printf(" Done\n");
+	}
+}
+
+static void
+signal_handler(__rte_unused int signum)
+{
+	if (signum == SIGINT || signum == SIGTERM) {
+		printf("\nSignal %d received, preparing to exit...\n",
+				signum);
+		if (force_quit < 0) {
+			printf("Forwarding not started yet...\n");
+			/* stop ports */
+			stop_ports();
+			printf("Bye...\n");
+			/* inform if there's a caller */
+			signal(signum, SIG_DFL);
+			kill(getpid(), signum);
+		} else {
+			printf("Forwarding started already...\n");
+			signo_quit = signum;
+			force_quit = 1;
+		}
+	}
+}
+
 int
 main(int argc, char **argv)
 {
@@ -2572,6 +2647,9 @@ main(int argc, char **argv)
 	uint32_t n_tx_queue, nb_lcores;
 	uint8_t portid, nb_rx_queue, queue, socketid;
 
+	signal(SIGINT, signal_handler);
+	signal(SIGTERM, signal_handler);
+
 	/* init EAL */
 	ret = rte_eal_init(argc, argv);
 	if (ret < 0)
@@ -2711,34 +2789,26 @@ main(int argc, char **argv)
 	printf("\n");
 
 	/* start ports */
-	for (portid = 0; portid < nb_ports; portid++) {
-		if ((enabled_port_mask & (1 << portid)) == 0) {
-			continue;
-		}
-		/* Start device */
-		ret = rte_eth_dev_start(portid);
-		if (ret < 0)
-			rte_exit(EXIT_FAILURE, "rte_eth_dev_start: err=%d, port=%d\n",
-				ret, portid);
-
-		/*
-		 * If enabled, put device in promiscuous mode.
-		 * This allows IO forwarding mode to forward packets
-		 * to itself through 2 cross-connected  ports of the
-		 * target machine.
-		 */
-		if (promiscuous_on)
-			rte_eth_promiscuous_enable(portid);
-	}
-
+	start_ports();
 	check_all_ports_link_status((uint8_t)nb_ports, enabled_port_mask);
 
 	/* launch per-lcore init on every lcore */
+	force_quit = 0;
 	rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER);
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
 		if (rte_eal_wait_lcore(lcore_id) < 0)
 			return -1;
 	}
 
+	printf("Stopping forwarding... Done\n");
+	/* stop ports */
+	stop_ports();
+	printf("Bye...\n");
+	/* inform if there's a caller */
+	if (force_quit != 0) {
+		signal(signo_quit, SIG_DFL);
+		kill(getpid(), signo_quit);
+	}
+
 	return 0;
 }
-- 
2.5.0

Re: [PATCH v2 1/3] app/test-pmd: Handle SIGINT and SIGTERM in testpmd

[Stephen Hemminger]

On Thu, 24 Dec 2015 21:37:10 -0500
Zhihong Wang <zhihong.wang@intel.com> wrote:

> Handle SIGINT and SIGTERM in testpmd.
> 
> Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
> ---
>  app/test-pmd/cmdline.c | 19 +++++++++++++------
>  app/test-pmd/testpmd.c | 38 ++++++++++++++++++++++++++++++++------
>  app/test-pmd/testpmd.h |  1 +
>  3 files changed, 46 insertions(+), 12 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 73298c9..4ff1739 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -90,6 +90,8 @@
>  
>  #include "testpmd.h"
>  
> +static struct cmdline *testpmd_cl;
> +
>  static void cmd_reconfig_device_queue(portid_t id, uint8_t dev, uint8_t queue);
>  
>  #ifdef RTE_NIC_BYPASS
> @@ -9778,17 +9780,22 @@ cmdline_parse_ctx_t main_ctx[] = {
>  void
>  prompt(void)
>  {
> -	struct cmdline *cl;
> -
>  	/* initialize non-constant commands */
>  	cmd_set_fwd_mode_init();
>  
> -	cl = cmdline_stdin_new(main_ctx, "testpmd> ");
> -	if (cl == NULL) {
> +	testpmd_cl = cmdline_stdin_new(main_ctx, "testpmd> ");
> +	if (testpmd_cl == NULL) {
>  		return;
>  	}

Style nit: don't need {} around single statement.

> -	cmdline_interact(cl);
> -	cmdline_stdin_exit(cl);
> +	cmdline_interact(testpmd_cl);
> +	cmdline_stdin_exit(testpmd_cl);
> +}
> +
> +void
> +prompt_exit(void)
> +{
> +	if (testpmd_cl != NULL)
> +		cmdline_quit(testpmd_cl);
>  }
>  
>  static void
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 98ae46d..cb38d56 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -1570,13 +1570,16 @@ pmd_test_exit(void)
>  	if (test_done == 0)
>  		stop_packet_forwarding();
>  
> -	FOREACH_PORT(pt_id, ports) {
> -		printf("Stopping port %d...", pt_id);
> -		fflush(stdout);
> -		rte_eth_dev_close(pt_id);
> -		printf("done\n");
> +	if (ports != NULL) {
> +		FOREACH_PORT(pt_id, ports) {
> +			printf("Stopping port %d...", pt_id);
> +			fflush(stdout);
> +			rte_eth_dev_stop(pt_id);
> +			rte_eth_dev_close(pt_id);
> +			printf(" Done\n");
> +		}
>  	}
> -	printf("bye...\n");
> +	printf("Bye...\n");
>  }
>  
>  typedef void (*cmd_func_t)(void);
> @@ -1984,12 +1987,34 @@ init_port(void)
>  		ports[pid].enabled = 1;
>  }
>  
> +static void
> +force_quit(void)
> +{
> +	pmd_test_exit();
> +	prompt_exit();
> +}
> +
> +static void
> +sigint_handler(__rte_unused int signum)
> +{
> +	if (signum == SIGINT || signum == SIGTERM) {

signmum is used, so don't want __rte_unused

> +		printf("\nSignal %d received, preparing to exit...\n",
> +				signum);
> +		force_quit();
> +		signal(signum, SIG_DFL);
> +		kill(getpid(), signum);
> +	}
> +}
> +
>  int
>  main(int argc, char** argv)
>  {
>  	int  diag;
>  	uint8_t port_id;
>  
> +	signal(SIGINT, sigint_handler);
> +	signal(SIGTERM, sigint_handler);
> +
>  	diag = rte_eal_init(argc, argv);
>  	if (diag < 0)
>  		rte_panic("Cannot init EAL\n");
> @@ -2041,6 +2066,7 @@ main(int argc, char** argv)
>  		start_packet_forwarding(0);
>  		printf("Press enter to exit\n");
>  		rc = read(0, &c, 1);
> +		pmd_test_exit();
>  		if (rc < 0)
>  			return 1;
>  	}
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index ee7de98..7ffc17b 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -462,6 +462,7 @@ unsigned int parse_item_list(char* str, const char* item_name,
>  			unsigned int *parsed_items, int check_unique_values);
>  void launch_args_parse(int argc, char** argv);
>  void prompt(void);
> +void prompt_exit(void);
>  void nic_stats_display(portid_t port_id);
>  void nic_stats_clear(portid_t port_id);
>  void nic_xstats_display(portid_t port_id);

Re: [PATCH v2 2/3] examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd

[Stephen Hemminger]

On Thu, 24 Dec 2015 21:37:11 -0500
Zhihong Wang <zhihong.wang@intel.com> wrote:

> Handle SIGINT and SIGTERM in l2fwd.
> 
> Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
> ---
>  examples/l2fwd/main.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/examples/l2fwd/main.c b/examples/l2fwd/main.c
> index 720fd5a..75899dd 100644
> --- a/examples/l2fwd/main.c
> +++ b/examples/l2fwd/main.c
> @@ -44,6 +44,8 @@
>  #include <ctype.h>
>  #include <errno.h>
>  #include <getopt.h>
> +#include <signal.h>
> +#include <unistd.h>
>  
>  #include <rte_common.h>
>  #include <rte_log.h>
> @@ -69,6 +71,9 @@
>  #include <rte_mempool.h>
>  #include <rte_mbuf.h>
>  
> +static int force_quit = -1;
> +static int signo_quit = -1;

These need to be volatile otherwise you risk compiler optimizing
away your checks.

Also, don't use -1/0 just use 0/1 for boolean or better yet
the definition in <stdbool.h> of bool and true/false.
That way the code can read much nicer.

>  #define RTE_LOGTYPE_L2FWD RTE_LOGTYPE_USER1
>  
>  #define NB_MBUF   8192
> @@ -284,6 +289,8 @@ l2fwd_main_loop(void)
>  	}
>  
>  	while (1) {
> +		if (unlikely(force_quit != 0))
> +			break;

Please maske this a proper while loop instead.

        while (!force_quit) {

>  
>  		cur_tsc = rte_rdtsc();
>  
> @@ -534,6 +541,45 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
>  	}
>  }
>  
> +static void
> +stop_ports(void)
> +{
> +	unsigned portid, nb_ports;
> +
> +	nb_ports = rte_eth_dev_count();
> +	for (portid = 0; portid < nb_ports; portid++) {
> +		if ((l2fwd_enabled_port_mask & (1 << portid)) == 0) {
> +			continue;
> +		}

No need for {} here.

> +		printf("Stopping port %d...", portid);
> +		rte_eth_dev_stop(portid);
> +		rte_eth_dev_close(portid);
> +		printf(" Done\n");
> +	}
> +}
> +
> +static void
> +signal_handler(__rte_unused int signum)
> +{
> +	if (signum == SIGINT || signum == SIGTERM) {

signum is used, dont give __rte_unused attribute.

>  
>  	/* launch per-lcore init on every lcore */
> +	force_quit = 0;

What is gained by having tri-value here. Just initialize it as false.


>  	rte_eal_mp_remote_launch(l2fwd_launch_one_lcore, NULL, CALL_MASTER);
>  	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>  		if (rte_eal_wait_lcore(lcore_id) < 0)
>  			return -1;
>  	}
>  
> +	printf("Stopping forwarding... Done\n");
> +	/* stop ports */
> +	stop_ports();
> +	printf("Bye...\n");
> +	/* inform if there's a caller */
> +	if (force_quit != 0) {
> +		signal(signo_quit, SIG_DFL);
> +		kill(getpid(), signo_quit);

The kill should not be needed.

It would be good if examples cleaned up allocations, that way they
could be used with valgrind for validation of drivers, etc.

Re: [PATCH v2 2/3] examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd

[Wang, Zhihong]

Hi Stephen,

Really appreciate the detailed review!
Please see comments below.


> > +static int force_quit = -1;
> > +static int signo_quit = -1;
> 
> These need to be volatile otherwise you risk compiler optimizing away your
> checks.

Yes. Don't wanna take chances here.

> 
> Also, don't use -1/0 just use 0/1 for boolean or better yet the definition in
> <stdbool.h> of bool and true/false.
> That way the code can read much nicer.

-1 when forwarding not started yet.
Can add a "static bool fwd_started;" to represent this to make it clearer.

> 
> >  #define RTE_LOGTYPE_L2FWD RTE_LOGTYPE_USER1
> >
> >  #define NB_MBUF   8192
> > @@ -284,6 +289,8 @@ l2fwd_main_loop(void)
> >  	}
> >
> >  	while (1) {
> > +		if (unlikely(force_quit != 0))
> > +			break;
> 
> Please maske this a proper while loop instead.

Exactly.

> 
>         while (!force_quit) {
> 
> >
> >  		cur_tsc = rte_rdtsc();
> >
> > @@ -534,6 +541,45 @@ check_all_ports_link_status(uint8_t port_num,
> uint32_t port_mask)
> >  	}
> >  }
> >
> > +static void
> > +stop_ports(void)
> > +{
> > +	unsigned portid, nb_ports;
> > +
> > +	nb_ports = rte_eth_dev_count();
> > +	for (portid = 0; portid < nb_ports; portid++) {
> > +		if ((l2fwd_enabled_port_mask & (1 << portid)) == 0) {
> > +			continue;
> > +		}
> 
> No need for {} here.
> 
> > +		printf("Stopping port %d...", portid);
> > +		rte_eth_dev_stop(portid);
> > +		rte_eth_dev_close(portid);
> > +		printf(" Done\n");
> > +	}
> > +}
> > +
> > +static void
> > +signal_handler(__rte_unused int signum) {
> > +	if (signum == SIGINT || signum == SIGTERM) {
> 
> signum is used, dont give __rte_unused attribute.
> 
> >
> >  	/* launch per-lcore init on every lcore */
> > +	force_quit = 0;
> 
> What is gained by having tri-value here. Just initialize it as false.

As stated above.

> 
> 
> >  	rte_eal_mp_remote_launch(l2fwd_launch_one_lcore, NULL,
> CALL_MASTER);
> >  	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
> >  		if (rte_eal_wait_lcore(lcore_id) < 0)
> >  			return -1;
> >  	}
> >
> > +	printf("Stopping forwarding... Done\n");
> > +	/* stop ports */
> > +	stop_ports();
> > +	printf("Bye...\n");
> > +	/* inform if there's a caller */
> > +	if (force_quit != 0) {
> > +		signal(signo_quit, SIG_DFL);
> > +		kill(getpid(), signo_quit);
> 
> The kill should not be needed.

The purpose is to make the program exit with the killed status.

> 
> It would be good if examples cleaned up allocations, that way they could be used
> with valgrind for validation of drivers, etc.

Re: [PATCH v2 1/3] app/test-pmd: Handle SIGINT and SIGTERM in testpmd

[Wang, Zhihong]

> > -	cl = cmdline_stdin_new(main_ctx, "testpmd> ");
> > -	if (cl == NULL) {
> > +	testpmd_cl = cmdline_stdin_new(main_ctx, "testpmd> ");
> > +	if (testpmd_cl == NULL) {
> >  		return;
> >  	}
> 
> Style nit: don't need {} around single statement.
> 
> > +static void
> > +sigint_handler(__rte_unused int signum) {
> > +	if (signum == SIGINT || signum == SIGTERM) {
> 
> signmum is used, so don't want __rte_unused
> 

Thanks :) Will fix these in the next version.

Re: [PATCH v2 0/3] Handle SIGINT and SIGTERM in DPDK examples

[Qiu, Michael]

On 2015/12/25 17:40, Wang, Zhihong wrote:
> This patch handles SIGINT and SIGTERM in testpmd, l2fwd, and l3fwd, make sure all ports are properly stopped and closed.
> For virtual ports, the stop and close function may deal with resource cleanup, such as socket files unlinking.
>
> --------------
> Changes in v2:
>
> 1. Make sure graceful exit for all running phases
>
> 2. Make sure program exits with the right status
>
> Zhihong Wang (3):
>   app/test-pmd: Handle SIGINT and SIGTERM in testpmd
>   examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd
>   examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd
>
>  app/test-pmd/cmdline.c |  19 ++++++---
>  app/test-pmd/testpmd.c |  38 ++++++++++++++---
>  app/test-pmd/testpmd.h |   1 +
>  examples/l2fwd/main.c  |  60 +++++++++++++++++++++++++++
>  examples/l3fwd/main.c  | 110 ++++++++++++++++++++++++++++++++++++++++---------
>  5 files changed, 196 insertions(+), 32 deletions(-)
>

Next time, you'd better not to top post for V2 :)

Acked-by: Michael Qiu <michael.qiu@intel.com>

Re: [PATCH v2 0/3] Handle SIGINT and SIGTERM in DPDK examples

[Wang, Zhihong]

> -----Original Message-----
> From: Qiu, Michael
> Sent: Monday, December 28, 2015 12:18 PM
> To: Wang, Zhihong <zhihong.wang@intel.com>; dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> stephen@networkplumber.org
> Subject: Re: [PATCH v2 0/3] Handle SIGINT and SIGTERM in DPDK examples
> 
> On 2015/12/25 17:40, Wang, Zhihong wrote:
> > This patch handles SIGINT and SIGTERM in testpmd, l2fwd, and l3fwd, make
> sure all ports are properly stopped and closed.
> > For virtual ports, the stop and close function may deal with resource cleanup,
> such as socket files unlinking.
> >
> > --------------
> > Changes in v2:
> >
> > 1. Make sure graceful exit for all running phases
> >
> > 2. Make sure program exits with the right status
> >
> > Zhihong Wang (3):
> >   app/test-pmd: Handle SIGINT and SIGTERM in testpmd
> >   examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd
> >   examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd
> >
> >  app/test-pmd/cmdline.c |  19 ++++++---
> >  app/test-pmd/testpmd.c |  38 ++++++++++++++---
> >  app/test-pmd/testpmd.h |   1 +
> >  examples/l2fwd/main.c  |  60 +++++++++++++++++++++++++++
> >  examples/l3fwd/main.c  | 110
> ++++++++++++++++++++++++++++++++++++++++---------
> >  5 files changed, 196 insertions(+), 32 deletions(-)
> >
> 
> Next time, you'd better not to top post for V2 :)

Gotcha :)

> 
> Acked-by: Michael Qiu <michael.qiu@intel.com>

[PATCH v3 0/3] Handle SIGINT and SIGTERM in DPDK examples

[Zhihong Wang]

This patch handles SIGINT and SIGTERM in testpmd, l2fwd, and l3fwd, make sure all ports are properly stopped and closed.
For virtual ports, the stop and close function may deal with resource cleanup, such as socket files unlinking.

--------------
Changes in v3:

1. Make sure correct port operations regarding status

2. Small fixes to make the code clearer

--------------
Changes in v2:

1. Make sure graceful exit for all running phases

2. Make sure program exits with the right status

Zhihong Wang (3):
  app/test-pmd: Handle SIGINT and SIGTERM in testpmd
  examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd
  examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

 app/test-pmd/cmdline.c |  20 +++++---
 app/test-pmd/testpmd.c |  39 ++++++++++++---
 app/test-pmd/testpmd.h |   1 +
 examples/l2fwd/main.c  | 123 +++++++++++++++++++++++++++++++++++++---------
 examples/l3fwd/main.c  | 129 ++++++++++++++++++++++++++++++++++++++++---------
 5 files changed, 255 insertions(+), 57 deletions(-)

-- 
2.5.0

[PATCH v3 1/3] app/test-pmd: Handle SIGINT and SIGTERM in testpmd

[Zhihong Wang]

Handle SIGINT and SIGTERM in testpmd.

Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
Acked-by: Michael Qiu <michael.qiu@intel.com>
---
 app/test-pmd/cmdline.c | 20 +++++++++++++-------
 app/test-pmd/testpmd.c | 39 +++++++++++++++++++++++++++++++++------
 app/test-pmd/testpmd.h |  1 +
 3 files changed, 47 insertions(+), 13 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 73298c9..6d28c1b 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -90,6 +90,8 @@
 
 #include "testpmd.h"
 
+static struct cmdline *testpmd_cl;
+
 static void cmd_reconfig_device_queue(portid_t id, uint8_t dev, uint8_t queue);
 
 #ifdef RTE_NIC_BYPASS
@@ -9778,17 +9780,21 @@ cmdline_parse_ctx_t main_ctx[] = {
 void
 prompt(void)
 {
-	struct cmdline *cl;
-
 	/* initialize non-constant commands */
 	cmd_set_fwd_mode_init();
 
-	cl = cmdline_stdin_new(main_ctx, "testpmd> ");
-	if (cl == NULL) {
+	testpmd_cl = cmdline_stdin_new(main_ctx, "testpmd> ");
+	if (testpmd_cl == NULL)
 		return;
-	}
-	cmdline_interact(cl);
-	cmdline_stdin_exit(cl);
+	cmdline_interact(testpmd_cl);
+	cmdline_stdin_exit(testpmd_cl);
+}
+
+void
+prompt_exit(void)
+{
+	if (testpmd_cl != NULL)
+		cmdline_quit(testpmd_cl);
 }
 
 static void
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 98ae46d..1319917 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1570,13 +1570,16 @@ pmd_test_exit(void)
 	if (test_done == 0)
 		stop_packet_forwarding();
 
-	FOREACH_PORT(pt_id, ports) {
-		printf("Stopping port %d...", pt_id);
-		fflush(stdout);
-		rte_eth_dev_close(pt_id);
-		printf("done\n");
+	if (ports != NULL) {
+		no_link_check = 1;
+		FOREACH_PORT(pt_id, ports) {
+			printf("\nShutting down port %d...\n", pt_id);
+			fflush(stdout);
+			stop_port(pt_id);
+			close_port(pt_id);
+		}
 	}
-	printf("bye...\n");
+	printf("\nBye...\n");
 }
 
 typedef void (*cmd_func_t)(void);
@@ -1984,12 +1987,35 @@ init_port(void)
 		ports[pid].enabled = 1;
 }
 
+static void
+force_quit(void)
+{
+	pmd_test_exit();
+	prompt_exit();
+}
+
+static void
+signal_handler(int signum)
+{
+	if (signum == SIGINT || signum == SIGTERM) {
+		printf("\nSignal %d received, preparing to exit...\n",
+				signum);
+		force_quit();
+		/* exit with the expected status */
+		signal(signum, SIG_DFL);
+		kill(getpid(), signum);
+	}
+}
+
 int
 main(int argc, char** argv)
 {
 	int  diag;
 	uint8_t port_id;
 
+	signal(SIGINT, signal_handler);
+	signal(SIGTERM, signal_handler);
+
 	diag = rte_eal_init(argc, argv);
 	if (diag < 0)
 		rte_panic("Cannot init EAL\n");
@@ -2041,6 +2067,7 @@ main(int argc, char** argv)
 		start_packet_forwarding(0);
 		printf("Press enter to exit\n");
 		rc = read(0, &c, 1);
+		pmd_test_exit();
 		if (rc < 0)
 			return 1;
 	}
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index ee7de98..7ffc17b 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -462,6 +462,7 @@ unsigned int parse_item_list(char* str, const char* item_name,
 			unsigned int *parsed_items, int check_unique_values);
 void launch_args_parse(int argc, char** argv);
 void prompt(void);
+void prompt_exit(void);
 void nic_stats_display(portid_t port_id);
 void nic_stats_clear(portid_t port_id);
 void nic_xstats_display(portid_t port_id);
-- 
2.5.0

[PATCH v3 2/3] examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd

[Zhihong Wang]

Handle SIGINT and SIGTERM in l2fwd.

Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
Acked-by: Michael Qiu <michael.qiu@intel.com>
---
 examples/l2fwd/main.c | 123 +++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 101 insertions(+), 22 deletions(-)

diff --git a/examples/l2fwd/main.c b/examples/l2fwd/main.c
index 720fd5a..ecd5d2b 100644
--- a/examples/l2fwd/main.c
+++ b/examples/l2fwd/main.c
@@ -44,6 +44,9 @@
 #include <ctype.h>
 #include <errno.h>
 #include <getopt.h>
+#include <signal.h>
+#include <unistd.h>
+#include <stdbool.h>
 
 #include <rte_common.h>
 #include <rte_log.h>
@@ -69,6 +72,10 @@
 #include <rte_mempool.h>
 #include <rte_mbuf.h>
 
+static volatile bool port_started;
+static volatile bool force_quit;
+static volatile int signo_quit;
+
 #define RTE_LOGTYPE_L2FWD RTE_LOGTYPE_USER1
 
 #define NB_MBUF   8192
@@ -283,8 +290,7 @@ l2fwd_main_loop(void)
 			portid);
 	}
 
-	while (1) {
-
+	while (!force_quit) {
 		cur_tsc = rte_rdtsc();
 
 		/*
@@ -491,8 +497,12 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
 	printf("\nChecking link status");
 	fflush(stdout);
 	for (count = 0; count <= MAX_CHECK_TIME; count++) {
+		if (force_quit)
+			return;
 		all_ports_up = 1;
 		for (portid = 0; portid < port_num; portid++) {
+			if (force_quit)
+				return;
 			if ((port_mask & (1 << portid)) == 0)
 				continue;
 			memset(&link, 0, sizeof(link));
@@ -534,18 +544,85 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
 	}
 }
 
+static uint8_t
+start_ports(void)
+{
+	unsigned portid, nb_ports, avail_ports;
+	int ret;
+
+	nb_ports = rte_eth_dev_count();
+	avail_ports = 0;
+	for (portid = 0; portid < nb_ports; portid++) {
+		if ((l2fwd_enabled_port_mask & (1 << portid)) == 0)
+			continue;
+		avail_ports++;
+		port_started = true;
+		printf("Starting port %d...", portid);
+		ret = rte_eth_dev_start(portid);
+		if (ret < 0)
+			rte_exit(EXIT_FAILURE,
+					"rte_eth_dev_start:err=%d, port=%u\n",
+				  ret, (unsigned) portid);
+		rte_eth_promiscuous_enable(portid);
+		printf(" Done\n");
+	}
+
+	return avail_ports;
+}
+
+static void
+stop_ports(void)
+{
+	unsigned portid, nb_ports;
+
+	nb_ports = rte_eth_dev_count();
+	for (portid = 0; portid < nb_ports; portid++) {
+		if ((l2fwd_enabled_port_mask & (1 << portid)) == 0)
+			continue;
+		printf("Stopping port %d...", portid);
+		rte_eth_dev_stop(portid);
+		rte_eth_dev_close(portid);
+		printf(" Done\n");
+	}
+	port_started = false;
+}
+
+static void
+signal_handler(int signum)
+{
+	if (signum == SIGINT || signum == SIGTERM) {
+		printf("\nSignal %d received, preparing to exit...\n",
+				signum);
+		if (port_started) {
+			printf("Ports started already...\n");
+			signo_quit = signum;
+			force_quit = true;
+		} else {
+			printf("Ports not started yet...\n");
+			printf("Bye...\n");
+			/* exit with the expected status */
+			signal(signum, SIG_DFL);
+			kill(getpid(), signum);
+		}
+	}
+}
+
 int
 main(int argc, char **argv)
 {
 	struct lcore_queue_conf *qconf;
 	struct rte_eth_dev_info dev_info;
 	int ret;
-	uint8_t nb_ports;
-	uint8_t nb_ports_available;
+	uint8_t nb_ports, avail_ports;
 	uint8_t portid, last_port;
 	unsigned lcore_id, rx_lcore_id;
 	unsigned nb_ports_in_mask = 0;
 
+	port_started = false;
+	force_quit = false;
+	signal(SIGINT, signal_handler);
+	signal(SIGTERM, signal_handler);
+
 	/* init EAL */
 	ret = rte_eal_init(argc, argv);
 	if (ret < 0)
@@ -627,14 +704,11 @@ main(int argc, char **argv)
 		printf("Lcore %u: RX port %u\n", rx_lcore_id, (unsigned) portid);
 	}
 
-	nb_ports_available = nb_ports;
-
 	/* Initialise each port */
 	for (portid = 0; portid < nb_ports; portid++) {
 		/* skip ports that are not enabled */
 		if ((l2fwd_enabled_port_mask & (1 << portid)) == 0) {
 			printf("Skipping disabled port %u\n", (unsigned) portid);
-			nb_ports_available--;
 			continue;
 		}
 		/* init port */
@@ -666,16 +740,6 @@ main(int argc, char **argv)
 			rte_exit(EXIT_FAILURE, "rte_eth_tx_queue_setup:err=%d, port=%u\n",
 				ret, (unsigned) portid);
 
-		/* Start device */
-		ret = rte_eth_dev_start(portid);
-		if (ret < 0)
-			rte_exit(EXIT_FAILURE, "rte_eth_dev_start:err=%d, port=%u\n",
-				  ret, (unsigned) portid);
-
-		printf("done: \n");
-
-		rte_eth_promiscuous_enable(portid);
-
 		printf("Port %u, MAC address: %02X:%02X:%02X:%02X:%02X:%02X\n\n",
 				(unsigned) portid,
 				l2fwd_ports_eth_addr[portid].addr_bytes[0],
@@ -689,19 +753,34 @@ main(int argc, char **argv)
 		memset(&port_statistics, 0, sizeof(port_statistics));
 	}
 
-	if (!nb_ports_available) {
+	/* start ports */
+	avail_ports = start_ports();
+
+	if (!avail_ports) {
 		rte_exit(EXIT_FAILURE,
-			"All available ports are disabled. Please set portmask.\n");
+			"All ports are disabled, please check portmask...\n");
 	}
 
 	check_all_ports_link_status(nb_ports, l2fwd_enabled_port_mask);
 
+	ret = 0;
 	/* launch per-lcore init on every lcore */
 	rte_eal_mp_remote_launch(l2fwd_launch_one_lcore, NULL, CALL_MASTER);
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
-		if (rte_eal_wait_lcore(lcore_id) < 0)
-			return -1;
+		if (rte_eal_wait_lcore(lcore_id) < 0) {
+			ret = -1;
+			break;
+		}
 	}
 
-	return 0;
+	/* stop ports */
+	stop_ports();
+	printf("Bye...\n");
+	/* exit with the expected status */
+	if (force_quit) {
+		signal(signo_quit, SIG_DFL);
+		kill(getpid(), signo_quit);
+	}
+
+	return ret;
 }
-- 
2.5.0

[PATCH v3 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

[Zhihong Wang]

Handle SIGINT and SIGTERM in l3fwd.

Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
Acked-by: Michael Qiu <michael.qiu@intel.com>
---
 examples/l3fwd/main.c | 129 +++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 107 insertions(+), 22 deletions(-)

diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
index 5b0c2dd..c766cf5 100644
--- a/examples/l3fwd/main.c
+++ b/examples/l3fwd/main.c
@@ -41,6 +41,9 @@
 #include <stdarg.h>
 #include <errno.h>
 #include <getopt.h>
+#include <signal.h>
+#include <unistd.h>
+#include <stdbool.h>
 
 #include <rte_common.h>
 #include <rte_vect.h>
@@ -75,6 +78,10 @@
 #include <cmdline_parse.h>
 #include <cmdline_parse_etheraddr.h>
 
+static volatile bool port_started;
+static volatile bool force_quit;
+static volatile int signo_quit;
+
 #define APP_LOOKUP_EXACT_MATCH          0
 #define APP_LOOKUP_LPM                  1
 #define DO_RFC_1812_CHECKS
@@ -1553,8 +1560,7 @@ main_loop(__attribute__((unused)) void *dummy)
 			portid, queueid);
 	}
 
-	while (1) {
-
+	while (!force_quit) {
 		cur_tsc = rte_rdtsc();
 
 		/*
@@ -2516,8 +2522,12 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
 	printf("\nChecking link status");
 	fflush(stdout);
 	for (count = 0; count <= MAX_CHECK_TIME; count++) {
+		if (force_quit)
+			return;
 		all_ports_up = 1;
 		for (portid = 0; portid < port_num; portid++) {
+			if (force_quit)
+				return;
 			if ((port_mask & (1 << portid)) == 0)
 				continue;
 			memset(&link, 0, sizeof(link));
@@ -2559,6 +2569,76 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
 	}
 }
 
+static uint8_t
+start_ports(void)
+{
+	unsigned portid, nb_ports, avail_ports;
+	int ret;
+
+	nb_ports = rte_eth_dev_count();
+	avail_ports = 0;
+	for (portid = 0; portid < nb_ports; portid++) {
+		if ((enabled_port_mask & (1 << portid)) == 0)
+			continue;
+		avail_ports++;
+		port_started = true;
+		printf("Starting port %d...", portid);
+		ret = rte_eth_dev_start(portid);
+		if (ret < 0)
+			rte_exit(EXIT_FAILURE,
+					"rte_eth_dev_start: err=%d, port=%d\n",
+					ret, portid);
+		/*
+		 * If enabled, put device in promiscuous mode.
+		 * This allows IO forwarding mode to forward packets
+		 * to itself through 2 cross-connected  ports of the
+		 * target machine.
+		 */
+		if (promiscuous_on)
+			rte_eth_promiscuous_enable(portid);
+		printf(" Done\n");
+	}
+
+	return avail_ports;
+}
+
+static void
+stop_ports(void)
+{
+	unsigned portid, nb_ports;
+
+	nb_ports = rte_eth_dev_count();
+	for (portid = 0; portid < nb_ports; portid++) {
+		if ((enabled_port_mask & (1 << portid)) == 0)
+			continue;
+		printf("Stopping port %d...", portid);
+		rte_eth_dev_stop(portid);
+		rte_eth_dev_close(portid);
+		printf(" Done\n");
+	}
+	port_started = false;
+}
+
+static void
+signal_handler(int signum)
+{
+	if (signum == SIGINT || signum == SIGTERM) {
+		printf("\nSignal %d received, preparing to exit...\n",
+				signum);
+		if (port_started) {
+			printf("Ports started already...\n");
+			signo_quit = signum;
+			force_quit = true;
+		} else {
+			printf("Ports not started yet...\n");
+			printf("Bye...\n");
+			/* exit with the expected status */
+			signal(signum, SIG_DFL);
+			kill(getpid(), signum);
+		}
+	}
+}
+
 int
 main(int argc, char **argv)
 {
@@ -2571,6 +2651,12 @@ main(int argc, char **argv)
 	unsigned lcore_id;
 	uint32_t n_tx_queue, nb_lcores;
 	uint8_t portid, nb_rx_queue, queue, socketid;
+	uint8_t avail_ports;
+
+	port_started = false;
+	force_quit = false;
+	signal(SIGINT, signal_handler);
+	signal(SIGTERM, signal_handler);
 
 	/* init EAL */
 	ret = rte_eal_init(argc, argv);
@@ -2711,34 +2797,33 @@ main(int argc, char **argv)
 	printf("\n");
 
 	/* start ports */
-	for (portid = 0; portid < nb_ports; portid++) {
-		if ((enabled_port_mask & (1 << portid)) == 0) {
-			continue;
-		}
-		/* Start device */
-		ret = rte_eth_dev_start(portid);
-		if (ret < 0)
-			rte_exit(EXIT_FAILURE, "rte_eth_dev_start: err=%d, port=%d\n",
-				ret, portid);
+	avail_ports = start_ports();
 
-		/*
-		 * If enabled, put device in promiscuous mode.
-		 * This allows IO forwarding mode to forward packets
-		 * to itself through 2 cross-connected  ports of the
-		 * target machine.
-		 */
-		if (promiscuous_on)
-			rte_eth_promiscuous_enable(portid);
+	if (!avail_ports) {
+		rte_exit(EXIT_FAILURE,
+			"All ports are disabled, please check portmask...\n");
 	}
 
 	check_all_ports_link_status((uint8_t)nb_ports, enabled_port_mask);
 
+	ret = 0;
 	/* launch per-lcore init on every lcore */
 	rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER);
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
-		if (rte_eal_wait_lcore(lcore_id) < 0)
-			return -1;
+		if (rte_eal_wait_lcore(lcore_id) < 0) {
+			ret = -1;
+			break;
+		}
 	}
 
-	return 0;
+	/* stop ports */
+	stop_ports();
+	printf("Bye...\n");
+	/* exit with the expected status */
+	if (force_quit) {
+		signal(signo_quit, SIG_DFL);
+		kill(getpid(), signo_quit);
+	}
+
+	return ret;
 }
-- 
2.5.0

Re: [PATCH v3 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

[Ananyev, Konstantin]

> -----Original Message-----
> From: Wang, Zhihong
> Sent: Tuesday, December 29, 2015 1:21 AM
> To: dev@dpdk.org
> Cc: Ananyev, Konstantin; stephen@networkplumber.org; Qiu, Michael; Wang, Zhihong
> Subject: [PATCH v3 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd
> 
> Handle SIGINT and SIGTERM in l3fwd.
> 
> Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
> Acked-by: Michael Qiu <michael.qiu@intel.com>
> ---
>  examples/l3fwd/main.c | 129 +++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 107 insertions(+), 22 deletions(-)
> 
> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
> index 5b0c2dd..c766cf5 100644
> --- a/examples/l3fwd/main.c
> +++ b/examples/l3fwd/main.c
> @@ -41,6 +41,9 @@
>  #include <stdarg.h>
>  #include <errno.h>
>  #include <getopt.h>
> +#include <signal.h>
> +#include <unistd.h>
> +#include <stdbool.h>
> 
>  #include <rte_common.h>
>  #include <rte_vect.h>
> @@ -75,6 +78,10 @@
>  #include <cmdline_parse.h>
>  #include <cmdline_parse_etheraddr.h>
> 
> +static volatile bool port_started;
> +static volatile bool force_quit;
> +static volatile int signo_quit;
> +
>  #define APP_LOOKUP_EXACT_MATCH          0
>  #define APP_LOOKUP_LPM                  1
>  #define DO_RFC_1812_CHECKS
> @@ -1553,8 +1560,7 @@ main_loop(__attribute__((unused)) void *dummy)
>  			portid, queueid);
>  	}
> 
> -	while (1) {
> -
> +	while (!force_quit) {
>  		cur_tsc = rte_rdtsc();
> 
>  		/*
> @@ -2516,8 +2522,12 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
>  	printf("\nChecking link status");
>  	fflush(stdout);
>  	for (count = 0; count <= MAX_CHECK_TIME; count++) {
> +		if (force_quit)
> +			return;
>  		all_ports_up = 1;
>  		for (portid = 0; portid < port_num; portid++) {
> +			if (force_quit)
> +				return;
>  			if ((port_mask & (1 << portid)) == 0)
>  				continue;
>  			memset(&link, 0, sizeof(link));
> @@ -2559,6 +2569,76 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
>  	}
>  }
> 
> +static uint8_t
> +start_ports(void)
> +{
> +	unsigned portid, nb_ports, avail_ports;
> +	int ret;
> +
> +	nb_ports = rte_eth_dev_count();
> +	avail_ports = 0;
> +	for (portid = 0; portid < nb_ports; portid++) {
> +		if ((enabled_port_mask & (1 << portid)) == 0)
> +			continue;
> +		avail_ports++;
> +		port_started = true;

Why do you need it at each iteration?

> +		printf("Starting port %d...", portid);
> +		ret = rte_eth_dev_start(portid);
> +		if (ret < 0)
> +			rte_exit(EXIT_FAILURE,
> +					"rte_eth_dev_start: err=%d, port=%d\n",
> +					ret, portid);
> +		/*
> +		 * If enabled, put device in promiscuous mode.
> +		 * This allows IO forwarding mode to forward packets
> +		 * to itself through 2 cross-connected  ports of the
> +		 * target machine.
> +		 */
> +		if (promiscuous_on)
> +			rte_eth_promiscuous_enable(portid);
> +		printf(" Done\n");
> +	}
> +
> +	return avail_ports;
> +}
> +
> +static void
> +stop_ports(void)
> +{
> +	unsigned portid, nb_ports;
> +
> +	nb_ports = rte_eth_dev_count();
> +	for (portid = 0; portid < nb_ports; portid++) {
> +		if ((enabled_port_mask & (1 << portid)) == 0)
> +			continue;
> +		printf("Stopping port %d...", portid);
> +		rte_eth_dev_stop(portid);
> +		rte_eth_dev_close(portid);
> +		printf(" Done\n");
> +	}
> +	port_started = false;
> +}
> +
> +static void
> +signal_handler(int signum)
> +{
> +	if (signum == SIGINT || signum == SIGTERM) {
> +		printf("\nSignal %d received, preparing to exit...\n",
> +				signum);
> +		if (port_started) {
> +			printf("Ports started already...\n");
> +			signo_quit = signum;
> +			force_quit = true;
> +		} else {


Hmm, and what if signal_handler() would be executed not in the context of master lcore?
Then there could be a raise condition, and you could end up here, while master lcore
would be in the middle of start_ports()->rte_eth_dev_start().
Probably not a big deal, but why do you need this  if (port_started) {...} else {...} at all?
Why not just:

signal_handler(int signum)
{
	signo_quit = signum;
	force_quit = true;
}
?

Konstantin

> +			printf("Ports not started yet...\n");
> +			printf("Bye...\n");
> +			/* exit with the expected status */
> +			signal(signum, SIG_DFL);
> +			kill(getpid(), signum);
> +		}
> +	}
> +}
> +
>  int
>  main(int argc, char **argv)
>  {
> @@ -2571,6 +2651,12 @@ main(int argc, char **argv)
>  	unsigned lcore_id;
>  	uint32_t n_tx_queue, nb_lcores;
>  	uint8_t portid, nb_rx_queue, queue, socketid;
> +	uint8_t avail_ports;
> +
> +	port_started = false;
> +	force_quit = false;
> +	signal(SIGINT, signal_handler);
> +	signal(SIGTERM, signal_handler);
> 
>  	/* init EAL */
>  	ret = rte_eal_init(argc, argv);
> @@ -2711,34 +2797,33 @@ main(int argc, char **argv)
>  	printf("\n");
> 
>  	/* start ports */
> -	for (portid = 0; portid < nb_ports; portid++) {
> -		if ((enabled_port_mask & (1 << portid)) == 0) {
> -			continue;
> -		}
> -		/* Start device */
> -		ret = rte_eth_dev_start(portid);
> -		if (ret < 0)
> -			rte_exit(EXIT_FAILURE, "rte_eth_dev_start: err=%d, port=%d\n",
> -				ret, portid);
> +	avail_ports = start_ports();
> 
> -		/*
> -		 * If enabled, put device in promiscuous mode.
> -		 * This allows IO forwarding mode to forward packets
> -		 * to itself through 2 cross-connected  ports of the
> -		 * target machine.
> -		 */
> -		if (promiscuous_on)
> -			rte_eth_promiscuous_enable(portid);
> +	if (!avail_ports) {
> +		rte_exit(EXIT_FAILURE,
> +			"All ports are disabled, please check portmask...\n");
>  	}
> 
>  	check_all_ports_link_status((uint8_t)nb_ports, enabled_port_mask);
> 
> +	ret = 0;
>  	/* launch per-lcore init on every lcore */
>  	rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER);
>  	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
> -		if (rte_eal_wait_lcore(lcore_id) < 0)
> -			return -1;
> +		if (rte_eal_wait_lcore(lcore_id) < 0) {
> +			ret = -1;
> +			break;
> +		}
>  	}
> 
> -	return 0;
> +	/* stop ports */
> +	stop_ports();
> +	printf("Bye...\n");
> +	/* exit with the expected status */
> +	if (force_quit) {
> +		signal(signo_quit, SIG_DFL);
> +		kill(getpid(), signo_quit);
> +	}
> +
> +	return ret;
>  }
> --
> 2.5.0

[PATCH v4 0/3] Handle SIGINT and SIGTERM in DPDK examples

[Zhihong Wang]

This patch handles SIGINT and SIGTERM in testpmd, l2fwd, and l3fwd, make sure all ports are properly stopped and closed.
For virtual ports, the stop and close function may deal with resource cleanup, such as socket files unlinking.

--------------
Changes in v4:

1. Add port status control in l2fwd and l3fwd

--------------
Changes in v3:

1. Make sure correct port operations regarding status

2. Small fixes to make the code clearer

--------------
Changes in v2:

1. Make sure graceful exit for all running phases

2. Make sure program exits with the right status

Zhihong Wang (3):
  app/test-pmd: Handle SIGINT and SIGTERM in testpmd
  examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd
  examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

 app/test-pmd/cmdline.c |  20 +++---
 app/test-pmd/testpmd.c |  39 ++++++++++--
 app/test-pmd/testpmd.h |   1 +
 examples/l2fwd/main.c  | 161 ++++++++++++++++++++++++++++++++++++++++-------
 examples/l3fwd/main.c  | 167 ++++++++++++++++++++++++++++++++++++++++++-------
 5 files changed, 331 insertions(+), 57 deletions(-)

-- 
2.5.0

[PATCH v4 1/3] app/test-pmd: Handle SIGINT and SIGTERM in testpmd

[Zhihong Wang]

Handle SIGINT and SIGTERM in testpmd.

Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
Acked-by: Michael Qiu <michael.qiu@intel.com>
---
 app/test-pmd/cmdline.c | 20 +++++++++++++-------
 app/test-pmd/testpmd.c | 39 +++++++++++++++++++++++++++++++++------
 app/test-pmd/testpmd.h |  1 +
 3 files changed, 47 insertions(+), 13 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 73298c9..6d28c1b 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -90,6 +90,8 @@
 
 #include "testpmd.h"
 
+static struct cmdline *testpmd_cl;
+
 static void cmd_reconfig_device_queue(portid_t id, uint8_t dev, uint8_t queue);
 
 #ifdef RTE_NIC_BYPASS
@@ -9778,17 +9780,21 @@ cmdline_parse_ctx_t main_ctx[] = {
 void
 prompt(void)
 {
-	struct cmdline *cl;
-
 	/* initialize non-constant commands */
 	cmd_set_fwd_mode_init();
 
-	cl = cmdline_stdin_new(main_ctx, "testpmd> ");
-	if (cl == NULL) {
+	testpmd_cl = cmdline_stdin_new(main_ctx, "testpmd> ");
+	if (testpmd_cl == NULL)
 		return;
-	}
-	cmdline_interact(cl);
-	cmdline_stdin_exit(cl);
+	cmdline_interact(testpmd_cl);
+	cmdline_stdin_exit(testpmd_cl);
+}
+
+void
+prompt_exit(void)
+{
+	if (testpmd_cl != NULL)
+		cmdline_quit(testpmd_cl);
 }
 
 static void
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 98ae46d..1319917 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1570,13 +1570,16 @@ pmd_test_exit(void)
 	if (test_done == 0)
 		stop_packet_forwarding();
 
-	FOREACH_PORT(pt_id, ports) {
-		printf("Stopping port %d...", pt_id);
-		fflush(stdout);
-		rte_eth_dev_close(pt_id);
-		printf("done\n");
+	if (ports != NULL) {
+		no_link_check = 1;
+		FOREACH_PORT(pt_id, ports) {
+			printf("\nShutting down port %d...\n", pt_id);
+			fflush(stdout);
+			stop_port(pt_id);
+			close_port(pt_id);
+		}
 	}
-	printf("bye...\n");
+	printf("\nBye...\n");
 }
 
 typedef void (*cmd_func_t)(void);
@@ -1984,12 +1987,35 @@ init_port(void)
 		ports[pid].enabled = 1;
 }
 
+static void
+force_quit(void)
+{
+	pmd_test_exit();
+	prompt_exit();
+}
+
+static void
+signal_handler(int signum)
+{
+	if (signum == SIGINT || signum == SIGTERM) {
+		printf("\nSignal %d received, preparing to exit...\n",
+				signum);
+		force_quit();
+		/* exit with the expected status */
+		signal(signum, SIG_DFL);
+		kill(getpid(), signum);
+	}
+}
+
 int
 main(int argc, char** argv)
 {
 	int  diag;
 	uint8_t port_id;
 
+	signal(SIGINT, signal_handler);
+	signal(SIGTERM, signal_handler);
+
 	diag = rte_eal_init(argc, argv);
 	if (diag < 0)
 		rte_panic("Cannot init EAL\n");
@@ -2041,6 +2067,7 @@ main(int argc, char** argv)
 		start_packet_forwarding(0);
 		printf("Press enter to exit\n");
 		rc = read(0, &c, 1);
+		pmd_test_exit();
 		if (rc < 0)
 			return 1;
 	}
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index ee7de98..7ffc17b 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -462,6 +462,7 @@ unsigned int parse_item_list(char* str, const char* item_name,
 			unsigned int *parsed_items, int check_unique_values);
 void launch_args_parse(int argc, char** argv);
 void prompt(void);
+void prompt_exit(void);
 void nic_stats_display(portid_t port_id);
 void nic_stats_clear(portid_t port_id);
 void nic_xstats_display(portid_t port_id);
-- 
2.5.0

[PATCH v4 2/3] examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd

[Zhihong Wang]

Handle SIGINT and SIGTERM in l2fwd.

Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
Acked-by: Michael Qiu <michael.qiu@intel.com>
---
 examples/l2fwd/main.c | 161 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 139 insertions(+), 22 deletions(-)

diff --git a/examples/l2fwd/main.c b/examples/l2fwd/main.c
index 720fd5a..9a6f80b 100644
--- a/examples/l2fwd/main.c
+++ b/examples/l2fwd/main.c
@@ -44,6 +44,9 @@
 #include <ctype.h>
 #include <errno.h>
 #include <getopt.h>
+#include <signal.h>
+#include <unistd.h>
+#include <stdbool.h>
 
 #include <rte_common.h>
 #include <rte_log.h>
@@ -69,6 +72,16 @@
 #include <rte_mempool.h>
 #include <rte_mbuf.h>
 
+#define PORT_IDLE 0
+#define PORT_INIT 1
+#define PORT_WORK 2
+#define PORT_STOP 3
+#define PORT_QUIT 4
+
+static volatile uint32_t port_status;
+static volatile bool force_quit;
+static volatile int signo_quit;
+
 #define RTE_LOGTYPE_L2FWD RTE_LOGTYPE_USER1
 
 #define NB_MBUF   8192
@@ -283,8 +296,7 @@ l2fwd_main_loop(void)
 			portid);
 	}
 
-	while (1) {
-
+	while (!force_quit) {
 		cur_tsc = rte_rdtsc();
 
 		/*
@@ -491,8 +503,12 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
 	printf("\nChecking link status");
 	fflush(stdout);
 	for (count = 0; count <= MAX_CHECK_TIME; count++) {
+		if (force_quit)
+			return;
 		all_ports_up = 1;
 		for (portid = 0; portid < port_num; portid++) {
+			if (force_quit)
+				return;
 			if ((port_mask & (1 << portid)) == 0)
 				continue;
 			memset(&link, 0, sizeof(link));
@@ -534,18 +550,110 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
 	}
 }
 
+static uint8_t
+start_ports(void)
+{
+	unsigned portid, nb_ports, avail_ports;
+	int ret;
+
+	if (rte_atomic32_cmpset(&port_status,
+				PORT_IDLE, PORT_INIT) == 0) {
+		printf("Ports not idle...\n");
+		return 0;
+	}
+
+	nb_ports = rte_eth_dev_count();
+	avail_ports = 0;
+	for (portid = 0; portid < nb_ports; portid++) {
+		if ((l2fwd_enabled_port_mask & (1 << portid)) == 0)
+			continue;
+		avail_ports++;
+		printf("Starting port %d...", portid);
+		ret = rte_eth_dev_start(portid);
+		if (ret < 0)
+			rte_exit(EXIT_FAILURE,
+					"rte_eth_dev_start:err=%d, port=%u\n",
+					ret, (unsigned) portid);
+		rte_eth_promiscuous_enable(portid);
+		printf(" Done\n");
+	}
+
+	if (avail_ports) {
+		if (rte_atomic32_cmpset(&port_status,
+					PORT_INIT, PORT_WORK) == 0)
+			printf("Set port state failed!\n");
+	} else {
+		if (rte_atomic32_cmpset(&port_status,
+					PORT_INIT, PORT_IDLE) == 0)
+			printf("Set port state failed!\n");
+	}
+
+	return avail_ports;
+}
+
+static void
+stop_ports(void)
+{
+	unsigned portid, nb_ports;
+
+	if (rte_atomic32_cmpset(&port_status,
+				PORT_WORK, PORT_STOP) == 0) {
+		printf("Ports not started...\n");
+		return;
+	}
+
+	nb_ports = rte_eth_dev_count();
+	for (portid = 0; portid < nb_ports; portid++) {
+		if ((l2fwd_enabled_port_mask & (1 << portid)) == 0)
+			continue;
+		printf("Stopping port %d...", portid);
+		rte_eth_dev_stop(portid);
+		rte_eth_dev_close(portid);
+		printf(" Done\n");
+	}
+
+	if (rte_atomic32_cmpset(&port_status,
+				PORT_STOP, PORT_IDLE) == 0)
+		printf("Set port state failed!\n");
+}
+
+static void
+signal_handler(int signum)
+{
+	if (signum == SIGINT || signum == SIGTERM) {
+		printf("\nSignal %d received, preparing to exit...\n",
+				signum);
+		if (rte_atomic32_cmpset(&port_status,
+					PORT_IDLE, PORT_QUIT) == 0) {
+			printf("Ports started already...\n");
+			signo_quit = signum;
+			force_quit = true;
+		} else {
+			printf("Ports not started yet...\n");
+			printf("Bye...\n");
+			/* exit with the expected status */
+			signal(signum, SIG_DFL);
+			kill(getpid(), signum);
+		}
+	}
+}
+
 int
 main(int argc, char **argv)
 {
 	struct lcore_queue_conf *qconf;
 	struct rte_eth_dev_info dev_info;
 	int ret;
-	uint8_t nb_ports;
-	uint8_t nb_ports_available;
+	uint8_t nb_ports, avail_ports;
 	uint8_t portid, last_port;
 	unsigned lcore_id, rx_lcore_id;
 	unsigned nb_ports_in_mask = 0;
 
+	port_status = PORT_IDLE;
+	force_quit = false;
+	signal(SIGINT, signal_handler);
+	signal(SIGTERM, signal_handler);
+
 	/* init EAL */
 	ret = rte_eal_init(argc, argv);
 	if (ret < 0)
@@ -627,14 +735,11 @@ main(int argc, char **argv)
 		printf("Lcore %u: RX port %u\n", rx_lcore_id, (unsigned) portid);
 	}
 
-	nb_ports_available = nb_ports;
-
 	/* Initialise each port */
 	for (portid = 0; portid < nb_ports; portid++) {
 		/* skip ports that are not enabled */
 		if ((l2fwd_enabled_port_mask & (1 << portid)) == 0) {
 			printf("Skipping disabled port %u\n", (unsigned) portid);
-			nb_ports_available--;
 			continue;
 		}
 		/* init port */
@@ -666,16 +771,6 @@ main(int argc, char **argv)
 			rte_exit(EXIT_FAILURE, "rte_eth_tx_queue_setup:err=%d, port=%u\n",
 				ret, (unsigned) portid);
 
-		/* Start device */
-		ret = rte_eth_dev_start(portid);
-		if (ret < 0)
-			rte_exit(EXIT_FAILURE, "rte_eth_dev_start:err=%d, port=%u\n",
-				  ret, (unsigned) portid);
-
-		printf("done: \n");
-
-		rte_eth_promiscuous_enable(portid);
-
 		printf("Port %u, MAC address: %02X:%02X:%02X:%02X:%02X:%02X\n\n",
 				(unsigned) portid,
 				l2fwd_ports_eth_addr[portid].addr_bytes[0],
@@ -689,19 +784,41 @@ main(int argc, char **argv)
 		memset(&port_statistics, 0, sizeof(port_statistics));
 	}
 
-	if (!nb_ports_available) {
+	/* start ports */
+	avail_ports = start_ports();
+
+	if (!avail_ports) {
+		/* exit with the expected status */
+		if (force_quit) {
+			printf("All ports disabled...\n");
+			printf("Bye...\n");
+			signal(signo_quit, SIG_DFL);
+			kill(getpid(), signo_quit);
+		}
 		rte_exit(EXIT_FAILURE,
-			"All available ports are disabled. Please set portmask.\n");
+			"All ports are disabled, please check portmask...\n");
 	}
 
 	check_all_ports_link_status(nb_ports, l2fwd_enabled_port_mask);
 
+	ret = 0;
 	/* launch per-lcore init on every lcore */
 	rte_eal_mp_remote_launch(l2fwd_launch_one_lcore, NULL, CALL_MASTER);
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
-		if (rte_eal_wait_lcore(lcore_id) < 0)
-			return -1;
+		if (rte_eal_wait_lcore(lcore_id) < 0) {
+			ret = -1;
+			break;
+		}
 	}
 
-	return 0;
+	/* stop ports */
+	stop_ports();
+	printf("Bye...\n");
+	/* exit with the expected status */
+	if (force_quit) {
+		signal(signo_quit, SIG_DFL);
+		kill(getpid(), signo_quit);
+	}
+
+	return ret;
 }
-- 
2.5.0

[PATCH v4 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

[Zhihong Wang]

Handle SIGINT and SIGTERM in l3fwd.

Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
Acked-by: Michael Qiu <michael.qiu@intel.com>
---
 examples/l3fwd/main.c | 167 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 145 insertions(+), 22 deletions(-)

diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
index 5b0c2dd..f73d2a4 100644
--- a/examples/l3fwd/main.c
+++ b/examples/l3fwd/main.c
@@ -41,6 +41,9 @@
 #include <stdarg.h>
 #include <errno.h>
 #include <getopt.h>
+#include <signal.h>
+#include <unistd.h>
+#include <stdbool.h>
 
 #include <rte_common.h>
 #include <rte_vect.h>
@@ -75,6 +78,16 @@
 #include <cmdline_parse.h>
 #include <cmdline_parse_etheraddr.h>
 
+#define PORT_IDLE 0
+#define PORT_INIT 1
+#define PORT_WORK 2
+#define PORT_STOP 3
+#define PORT_QUIT 4
+
+static volatile uint32_t port_status;
+static volatile bool force_quit;
+static volatile int signo_quit;
+
 #define APP_LOOKUP_EXACT_MATCH          0
 #define APP_LOOKUP_LPM                  1
 #define DO_RFC_1812_CHECKS
@@ -1553,8 +1566,7 @@ main_loop(__attribute__((unused)) void *dummy)
 			portid, queueid);
 	}
 
-	while (1) {
-
+	while (!force_quit) {
 		cur_tsc = rte_rdtsc();
 
 		/*
@@ -2516,8 +2528,12 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
 	printf("\nChecking link status");
 	fflush(stdout);
 	for (count = 0; count <= MAX_CHECK_TIME; count++) {
+		if (force_quit)
+			return;
 		all_ports_up = 1;
 		for (portid = 0; portid < port_num; portid++) {
+			if (force_quit)
+				return;
 			if ((port_mask & (1 << portid)) == 0)
 				continue;
 			memset(&link, 0, sizeof(link));
@@ -2559,6 +2575,101 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
 	}
 }
 
+static uint8_t
+start_ports(void)
+{
+	unsigned portid, nb_ports, avail_ports;
+	int ret;
+
+	if (rte_atomic32_cmpset(&port_status,
+				PORT_IDLE, PORT_INIT) == 0) {
+		printf("Ports not idle...\n");
+		return 0;
+	}
+
+	nb_ports = rte_eth_dev_count();
+	avail_ports = 0;
+	for (portid = 0; portid < nb_ports; portid++) {
+		if ((enabled_port_mask & (1 << portid)) == 0)
+			continue;
+		avail_ports++;
+		printf("Starting port %d...", portid);
+		ret = rte_eth_dev_start(portid);
+		if (ret < 0)
+			rte_exit(EXIT_FAILURE,
+					"rte_eth_dev_start: err=%d, port=%d\n",
+					ret, portid);
+		/*
+		 * If enabled, put device in promiscuous mode.
+		 * This allows IO forwarding mode to forward packets
+		 * to itself through 2 cross-connected  ports of the
+		 * target machine.
+		 */
+		if (promiscuous_on)
+			rte_eth_promiscuous_enable(portid);
+		printf(" Done\n");
+	}
+
+	if (avail_ports) {
+		if (rte_atomic32_cmpset(&port_status,
+					PORT_INIT, PORT_WORK) == 0)
+			printf("Set port state failed!\n");
+	} else {
+		if (rte_atomic32_cmpset(&port_status,
+					PORT_INIT, PORT_IDLE) == 0)
+			printf("Set port state failed!\n");
+	}
+
+	return avail_ports;
+}
+
+static void
+stop_ports(void)
+{
+	unsigned portid, nb_ports;
+
+	if (rte_atomic32_cmpset(&port_status,
+				PORT_WORK, PORT_STOP) == 0) {
+		printf("Ports not started...\n");
+		return;
+	}
+
+	nb_ports = rte_eth_dev_count();
+	for (portid = 0; portid < nb_ports; portid++) {
+		if ((enabled_port_mask & (1 << portid)) == 0)
+			continue;
+		printf("Stopping port %d...", portid);
+		rte_eth_dev_stop(portid);
+		rte_eth_dev_close(portid);
+		printf(" Done\n");
+	}
+
+	if (rte_atomic32_cmpset(&port_status,
+				PORT_STOP, PORT_IDLE) == 0)
+		printf("Set port state failed!\n");
+}
+
+static void
+signal_handler(int signum)
+{
+	if (signum == SIGINT || signum == SIGTERM) {
+		printf("\nSignal %d received, preparing to exit...\n",
+				signum);
+		if (rte_atomic32_cmpset(&port_status,
+					PORT_IDLE, PORT_QUIT) == 0) {
+			printf("Ports started already...\n");
+			signo_quit = signum;
+			force_quit = true;
+		} else {
+			printf("Ports not started yet...\n");
+			printf("Bye...\n");
+			/* exit with the expected status */
+			signal(signum, SIG_DFL);
+			kill(getpid(), signum);
+		}
+	}
+}
+
 int
 main(int argc, char **argv)
 {
@@ -2571,6 +2682,12 @@ main(int argc, char **argv)
 	unsigned lcore_id;
 	uint32_t n_tx_queue, nb_lcores;
 	uint8_t portid, nb_rx_queue, queue, socketid;
+	uint8_t avail_ports;
+
+	port_status = PORT_IDLE;
+	force_quit = false;
+	signal(SIGINT, signal_handler);
+	signal(SIGTERM, signal_handler);
 
 	/* init EAL */
 	ret = rte_eal_init(argc, argv);
@@ -2711,34 +2828,40 @@ main(int argc, char **argv)
 	printf("\n");
 
 	/* start ports */
-	for (portid = 0; portid < nb_ports; portid++) {
-		if ((enabled_port_mask & (1 << portid)) == 0) {
-			continue;
+	avail_ports = start_ports();
+
+	if (!avail_ports) {
+		/* exit with the expected status */
+		if (force_quit) {
+			printf("All ports disabled...\n");
+			printf("Bye...\n");
+			signal(signo_quit, SIG_DFL);
+			kill(getpid(), signo_quit);
 		}
-		/* Start device */
-		ret = rte_eth_dev_start(portid);
-		if (ret < 0)
-			rte_exit(EXIT_FAILURE, "rte_eth_dev_start: err=%d, port=%d\n",
-				ret, portid);
-
-		/*
-		 * If enabled, put device in promiscuous mode.
-		 * This allows IO forwarding mode to forward packets
-		 * to itself through 2 cross-connected  ports of the
-		 * target machine.
-		 */
-		if (promiscuous_on)
-			rte_eth_promiscuous_enable(portid);
+		rte_exit(EXIT_FAILURE,
+			"All ports are disabled, please check portmask...\n");
 	}
 
 	check_all_ports_link_status((uint8_t)nb_ports, enabled_port_mask);
 
+	ret = 0;
 	/* launch per-lcore init on every lcore */
 	rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER);
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
-		if (rte_eal_wait_lcore(lcore_id) < 0)
-			return -1;
+		if (rte_eal_wait_lcore(lcore_id) < 0) {
+			ret = -1;
+			break;
+		}
 	}
 
-	return 0;
+	/* stop ports */
+	stop_ports();
+	printf("Bye...\n");
+	/* exit with the expected status */
+	if (force_quit) {
+		signal(signo_quit, SIG_DFL);
+		kill(getpid(), signo_quit);
+	}
+
+	return ret;
 }
-- 
2.5.0

Re: [PATCH v3 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

[Wang, Zhihong]

> > +static uint8_t
> > +start_ports(void)
> > +{
> > +	unsigned portid, nb_ports, avail_ports;
> > +	int ret;
> > +
> > +	nb_ports = rte_eth_dev_count();
> > +	avail_ports = 0;
> > +	for (portid = 0; portid < nb_ports; portid++) {
> > +		if ((enabled_port_mask & (1 << portid)) == 0)
> > +			continue;
> > +		avail_ports++;
> > +		port_started = true;
> 
> Why do you need it at each iteration?

Only become true when the first enabled port about to started. In case there's no port enabled at all.
In my opinion no need to optimize since it's not performance sensitive and the logic is correct :)


> 
> > +		printf("Starting port %d...", portid);
> > +		ret = rte_eth_dev_start(portid);
> > +		if (ret < 0)
> > +			rte_exit(EXIT_FAILURE,
> > +					"rte_eth_dev_start: err=%d, port=%d\n",
> > +					ret, portid);
> > +		/*
> > +		 * If enabled, put device in promiscuous mode.
> > +		 * This allows IO forwarding mode to forward packets
> > +		 * to itself through 2 cross-connected  ports of the
> > +		 * target machine.
> > +		 */
> > +		if (promiscuous_on)
> > +			rte_eth_promiscuous_enable(portid);
> > +		printf(" Done\n");
> > +	}
> > +
> > +	return avail_ports;
> > +}

[...]

> > +static void
> > +signal_handler(int signum)
> > +{
> > +	if (signum == SIGINT || signum == SIGTERM) {
> > +		printf("\nSignal %d received, preparing to exit...\n",
> > +				signum);
> > +		if (port_started) {
> > +			printf("Ports started already...\n");
> > +			signo_quit = signum;
> > +			force_quit = true;
> > +		} else {
> 
> 
> Hmm, and what if signal_handler() would be executed not in the context of
> master lcore?
> Then there could be a raise condition, and you could end up here, while master
> lcore would be in the middle of start_ports()->rte_eth_dev_start().

Good point! Then we need rte_atomic16_cmpset() to avoid the race condition.


> Probably not a big deal, but why do you need this  if (port_started) {...} else {...}
> at all?
> Why not just:

If no port has been started, then just kill itself.
This is for cases like when you just started it and then want to shut it down, it'll wait a long time for initialization (memory, etc.) before the force_quit signal take effect.


> 
> signal_handler(int signum)
> {
> 	signo_quit = signum;
> 	force_quit = true;
> }
> ?
> 
> Konstantin
> 
> > +			printf("Ports not started yet...\n");
> > +			printf("Bye...\n");
> > +			/* exit with the expected status */
> > +			signal(signum, SIG_DFL);
> > +			kill(getpid(), signum);
> > +		}
> > +	}
> > +}
> > +

Re: [PATCH v3 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

[Ananyev, Konstantin]

> -----Original Message-----
> From: Wang, Zhihong
> Sent: Wednesday, December 30, 2015 3:15 AM
> To: Ananyev, Konstantin; dev@dpdk.org
> Cc: stephen@networkplumber.org; Qiu, Michael
> Subject: RE: [PATCH v3 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd
> 
> > > +static uint8_t
> > > +start_ports(void)
> > > +{
> > > +	unsigned portid, nb_ports, avail_ports;
> > > +	int ret;
> > > +
> > > +	nb_ports = rte_eth_dev_count();
> > > +	avail_ports = 0;
> > > +	for (portid = 0; portid < nb_ports; portid++) {
> > > +		if ((enabled_port_mask & (1 << portid)) == 0)
> > > +			continue;
> > > +		avail_ports++;
> > > +		port_started = true;
> >
> > Why do you need it at each iteration?
> 
> Only become true when the first enabled port about to started. In case there's no port enabled at all.
> In my opinion no need to optimize since it's not performance sensitive and the logic is correct :)
> 
> 
> >
> > > +		printf("Starting port %d...", portid);
> > > +		ret = rte_eth_dev_start(portid);
> > > +		if (ret < 0)
> > > +			rte_exit(EXIT_FAILURE,
> > > +					"rte_eth_dev_start: err=%d, port=%d\n",
> > > +					ret, portid);
> > > +		/*
> > > +		 * If enabled, put device in promiscuous mode.
> > > +		 * This allows IO forwarding mode to forward packets
> > > +		 * to itself through 2 cross-connected  ports of the
> > > +		 * target machine.
> > > +		 */
> > > +		if (promiscuous_on)
> > > +			rte_eth_promiscuous_enable(portid);
> > > +		printf(" Done\n");
> > > +	}
> > > +
> > > +	return avail_ports;
> > > +}
> 
> [...]
> 
> > > +static void
> > > +signal_handler(int signum)
> > > +{
> > > +	if (signum == SIGINT || signum == SIGTERM) {
> > > +		printf("\nSignal %d received, preparing to exit...\n",
> > > +				signum);
> > > +		if (port_started) {
> > > +			printf("Ports started already...\n");
> > > +			signo_quit = signum;
> > > +			force_quit = true;
> > > +		} else {
> >
> >
> > Hmm, and what if signal_handler() would be executed not in the context of
> > master lcore?
> > Then there could be a raise condition, and you could end up here, while master
> > lcore would be in the middle of start_ports()->rte_eth_dev_start().
> 
> Good point! Then we need rte_atomic16_cmpset() to avoid the race condition.
> 
> 
> > Probably not a big deal, but why do you need this  if (port_started) {...} else {...}
> > at all?
> > Why not just:
> 
> If no port has been started, then just kill itself.
> This is for cases like when you just started it and then want to shut it down, it'll wait a long time for initialization (memory, etc.) before
> the force_quit signal take effect.

Do you mean rte_eal_init()?
Then why not to install non-default signal handlers after rte_eal_init()?
Konstantin

> 
> 
> >
> > signal_handler(int signum)
> > {
> > 	signo_quit = signum;
> > 	force_quit = true;
> > }
> > ?
> >
> > Konstantin
> >
> > > +			printf("Ports not started yet...\n");
> > > +			printf("Bye...\n");
> > > +			/* exit with the expected status */
> > > +			signal(signum, SIG_DFL);
> > > +			kill(getpid(), signum);
> > > +		}
> > > +	}
> > > +}
> > > +

Re: [PATCH v4 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

[Ananyev, Konstantin]

> -----Original Message-----
> From: Wang, Zhihong
> Sent: Tuesday, December 29, 2015 11:27 PM
> To: dev@dpdk.org
> Cc: Ananyev, Konstantin; stephen@networkplumber.org; Qiu, Michael; Wang, Zhihong
> Subject: [PATCH v4 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd
> 
> Handle SIGINT and SIGTERM in l3fwd.
> 
> Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
> Acked-by: Michael Qiu <michael.qiu@intel.com>
> ---
>  examples/l3fwd/main.c | 167 +++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 145 insertions(+), 22 deletions(-)
> 
> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
> index 5b0c2dd..f73d2a4 100644
> --- a/examples/l3fwd/main.c
> +++ b/examples/l3fwd/main.c
> @@ -41,6 +41,9 @@
>  #include <stdarg.h>
>  #include <errno.h>
>  #include <getopt.h>
> +#include <signal.h>
> +#include <unistd.h>
> +#include <stdbool.h>
> 
>  #include <rte_common.h>
>  #include <rte_vect.h>
> @@ -75,6 +78,16 @@
>  #include <cmdline_parse.h>
>  #include <cmdline_parse_etheraddr.h>
> 
> +#define PORT_IDLE 0
> +#define PORT_INIT 1
> +#define PORT_WORK 2
> +#define PORT_STOP 3
> +#define PORT_QUIT 4

Seems ok, but over-complicated.
I think all you need is just IDLE, INIT, QUIT.
Konstantin

> +
> +static volatile uint32_t port_status;
> +static volatile bool force_quit;
> +static volatile int signo_quit;
> +
>  #define APP_LOOKUP_EXACT_MATCH          0
>  #define APP_LOOKUP_LPM                  1
>  #define DO_RFC_1812_CHECKS
> @@ -1553,8 +1566,7 @@ main_loop(__attribute__((unused)) void *dummy)
>  			portid, queueid);
>  	}
> 
> -	while (1) {
> -
> +	while (!force_quit) {
>  		cur_tsc = rte_rdtsc();
> 
>  		/*
> @@ -2516,8 +2528,12 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
>  	printf("\nChecking link status");
>  	fflush(stdout);
>  	for (count = 0; count <= MAX_CHECK_TIME; count++) {
> +		if (force_quit)
> +			return;
>  		all_ports_up = 1;
>  		for (portid = 0; portid < port_num; portid++) {
> +			if (force_quit)
> +				return;
>  			if ((port_mask & (1 << portid)) == 0)
>  				continue;
>  			memset(&link, 0, sizeof(link));
> @@ -2559,6 +2575,101 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
>  	}
>  }
> 
> +static uint8_t
> +start_ports(void)
> +{
> +	unsigned portid, nb_ports, avail_ports;
> +	int ret;
> +
> +	if (rte_atomic32_cmpset(&port_status,
> +				PORT_IDLE, PORT_INIT) == 0) {
> +		printf("Ports not idle...\n");
> +		return 0;
> +	}
> +
> +	nb_ports = rte_eth_dev_count();
> +	avail_ports = 0;
> +	for (portid = 0; portid < nb_ports; portid++) {
> +		if ((enabled_port_mask & (1 << portid)) == 0)
> +			continue;
> +		avail_ports++;
> +		printf("Starting port %d...", portid);
> +		ret = rte_eth_dev_start(portid);
> +		if (ret < 0)
> +			rte_exit(EXIT_FAILURE,
> +					"rte_eth_dev_start: err=%d, port=%d\n",
> +					ret, portid);
> +		/*
> +		 * If enabled, put device in promiscuous mode.
> +		 * This allows IO forwarding mode to forward packets
> +		 * to itself through 2 cross-connected  ports of the
> +		 * target machine.
> +		 */
> +		if (promiscuous_on)
> +			rte_eth_promiscuous_enable(portid);
> +		printf(" Done\n");
> +	}
> +
> +	if (avail_ports) {
> +		if (rte_atomic32_cmpset(&port_status,
> +					PORT_INIT, PORT_WORK) == 0)
> +			printf("Set port state failed!\n");
> +	} else {
> +		if (rte_atomic32_cmpset(&port_status,
> +					PORT_INIT, PORT_IDLE) == 0)
> +			printf("Set port state failed!\n");
> +	}
> +
> +	return avail_ports;
> +}
> +
> +static void
> +stop_ports(void)
> +{
> +	unsigned portid, nb_ports;
> +
> +	if (rte_atomic32_cmpset(&port_status,
> +				PORT_WORK, PORT_STOP) == 0) {
> +		printf("Ports not started...\n");
> +		return;
> +	}
> +
> +	nb_ports = rte_eth_dev_count();
> +	for (portid = 0; portid < nb_ports; portid++) {
> +		if ((enabled_port_mask & (1 << portid)) == 0)
> +			continue;
> +		printf("Stopping port %d...", portid);
> +		rte_eth_dev_stop(portid);
> +		rte_eth_dev_close(portid);
> +		printf(" Done\n");
> +	}
> +
> +	if (rte_atomic32_cmpset(&port_status,
> +				PORT_STOP, PORT_IDLE) == 0)
> +		printf("Set port state failed!\n");
> +}
> +
> +static void
> +signal_handler(int signum)
> +{
> +	if (signum == SIGINT || signum == SIGTERM) {
> +		printf("\nSignal %d received, preparing to exit...\n",
> +				signum);
> +		if (rte_atomic32_cmpset(&port_status,
> +					PORT_IDLE, PORT_QUIT) == 0) {
> +			printf("Ports started already...\n");
> +			signo_quit = signum;
> +			force_quit = true;
> +		} else {
> +			printf("Ports not started yet...\n");
> +			printf("Bye...\n");
> +			/* exit with the expected status */
> +			signal(signum, SIG_DFL);
> +			kill(getpid(), signum);
> +		}
> +	}
> +}
> +
>  int
>  main(int argc, char **argv)
>  {
> @@ -2571,6 +2682,12 @@ main(int argc, char **argv)
>  	unsigned lcore_id;
>  	uint32_t n_tx_queue, nb_lcores;
>  	uint8_t portid, nb_rx_queue, queue, socketid;
> +	uint8_t avail_ports;
> +
> +	port_status = PORT_IDLE;
> +	force_quit = false;
> +	signal(SIGINT, signal_handler);
> +	signal(SIGTERM, signal_handler);
> 
>  	/* init EAL */
>  	ret = rte_eal_init(argc, argv);
> @@ -2711,34 +2828,40 @@ main(int argc, char **argv)
>  	printf("\n");
> 
>  	/* start ports */
> -	for (portid = 0; portid < nb_ports; portid++) {
> -		if ((enabled_port_mask & (1 << portid)) == 0) {
> -			continue;
> +	avail_ports = start_ports();
> +
> +	if (!avail_ports) {
> +		/* exit with the expected status */
> +		if (force_quit) {
> +			printf("All ports disabled...\n");
> +			printf("Bye...\n");
> +			signal(signo_quit, SIG_DFL);
> +			kill(getpid(), signo_quit);
>  		}
> -		/* Start device */
> -		ret = rte_eth_dev_start(portid);
> -		if (ret < 0)
> -			rte_exit(EXIT_FAILURE, "rte_eth_dev_start: err=%d, port=%d\n",
> -				ret, portid);
> -
> -		/*
> -		 * If enabled, put device in promiscuous mode.
> -		 * This allows IO forwarding mode to forward packets
> -		 * to itself through 2 cross-connected  ports of the
> -		 * target machine.
> -		 */
> -		if (promiscuous_on)
> -			rte_eth_promiscuous_enable(portid);
> +		rte_exit(EXIT_FAILURE,
> +			"All ports are disabled, please check portmask...\n");
>  	}
> 
>  	check_all_ports_link_status((uint8_t)nb_ports, enabled_port_mask);
> 
> +	ret = 0;
>  	/* launch per-lcore init on every lcore */
>  	rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER);
>  	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
> -		if (rte_eal_wait_lcore(lcore_id) < 0)
> -			return -1;
> +		if (rte_eal_wait_lcore(lcore_id) < 0) {
> +			ret = -1;
> +			break;
> +		}
>  	}
> 
> -	return 0;
> +	/* stop ports */
> +	stop_ports();
> +	printf("Bye...\n");
> +	/* exit with the expected status */
> +	if (force_quit) {
> +		signal(signo_quit, SIG_DFL);
> +		kill(getpid(), signo_quit);
> +	}
> +
> +	return ret;
>  }
> --
> 2.5.0

Re: [PATCH v3 2/3] examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd

[Stephen Hemminger]

On Mon, 28 Dec 2015 20:20:31 -0500
Zhihong Wang <zhihong.wang@intel.com> wrote:

> +	/* exit with the expected status */
> +	if (force_quit) {
> +		signal(signo_quit, SIG_DFL);
> +		kill(getpid(), signo_quit);

This is really not necessary. Most other programs don't do this.

Re: [PATCH v3 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

[Stephen Hemminger]

On Mon, 28 Dec 2015 20:20:32 -0500
Zhihong Wang <zhihong.wang@intel.com> wrote:

> Handle SIGINT and SIGTERM in l3fwd.
> 
> Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
> Acked-by: Michael Qiu <michael.qiu@intel.com>
> ---
>  examples/l3fwd/main.c | 129 +++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 107 insertions(+), 22 deletions(-)
> 
> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
> index 5b0c2dd..c766cf5 100644
> --- a/examples/l3fwd/main.c
> +++ b/examples/l3fwd/main.c
> @@ -41,6 +41,9 @@
>  #include <stdarg.h>
>  #include <errno.h>
>  #include <getopt.h>
> +#include <signal.h>
> +#include <unistd.h>
> +#include <stdbool.h>
>  
>  #include <rte_common.h>
>  #include <rte_vect.h>
> @@ -75,6 +78,10 @@
>  #include <cmdline_parse.h>
>  #include <cmdline_parse_etheraddr.h>
>  
> +static volatile bool port_started;
> +static volatile bool force_quit;
> +static volatile int signo_quit;

I don't think you need the port_started or signo_quit logic.

The port_started logic is racy, and if you write the loops
correctly, is unnecessary.

The signo_quit logic is unnecessary, since exit code of interrupted
program doesn't matter. 

[PATCH v5 0/3] Handle SIGINT and SIGTERM in DPDK examples

[Zhihong Wang]

This patch handles SIGINT and SIGTERM in testpmd, l2fwd, and l3fwd, make sure all ports are properly stopped and closed.
For virtual ports, the stop and close function may deal with resource cleanup, such as socket files unlinking.

--------------
Changes in v5:

1. Get rid of over complicated logic in l2fwd and l3fwd

--------------
Changes in v4:

1. Add port status control in l2fwd and l3fwd

--------------
Changes in v3:

1. Make sure correct port operations regarding status

2. Small fixes to make the code clearer

--------------
Changes in v2:

1. Make sure graceful exit for all running phases

2. Make sure program exits with the right status

Zhihong Wang (3):
  app/test-pmd: Handle SIGINT and SIGTERM in testpmd
  examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd
  examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

 app/test-pmd/cmdline.c | 20 +++++++++++++-------
 app/test-pmd/testpmd.c | 39 +++++++++++++++++++++++++++++++++------
 app/test-pmd/testpmd.h |  1 +
 examples/l2fwd/main.c  | 43 +++++++++++++++++++++++++++++++++++++++----
 examples/l3fwd/main.c  | 46 ++++++++++++++++++++++++++++++++++++++++++----
 5 files changed, 128 insertions(+), 21 deletions(-)

-- 
2.5.0

[PATCH v5 1/3] app/test-pmd: Handle SIGINT and SIGTERM in testpmd

[Zhihong Wang]

Handle SIGINT and SIGTERM in testpmd.

Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
Acked-by: Michael Qiu <michael.qiu@intel.com>
---
 app/test-pmd/cmdline.c | 20 +++++++++++++-------
 app/test-pmd/testpmd.c | 39 +++++++++++++++++++++++++++++++++------
 app/test-pmd/testpmd.h |  1 +
 3 files changed, 47 insertions(+), 13 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 73298c9..6d28c1b 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -90,6 +90,8 @@
 
 #include "testpmd.h"
 
+static struct cmdline *testpmd_cl;
+
 static void cmd_reconfig_device_queue(portid_t id, uint8_t dev, uint8_t queue);
 
 #ifdef RTE_NIC_BYPASS
@@ -9778,17 +9780,21 @@ cmdline_parse_ctx_t main_ctx[] = {
 void
 prompt(void)
 {
-	struct cmdline *cl;
-
 	/* initialize non-constant commands */
 	cmd_set_fwd_mode_init();
 
-	cl = cmdline_stdin_new(main_ctx, "testpmd> ");
-	if (cl == NULL) {
+	testpmd_cl = cmdline_stdin_new(main_ctx, "testpmd> ");
+	if (testpmd_cl == NULL)
 		return;
-	}
-	cmdline_interact(cl);
-	cmdline_stdin_exit(cl);
+	cmdline_interact(testpmd_cl);
+	cmdline_stdin_exit(testpmd_cl);
+}
+
+void
+prompt_exit(void)
+{
+	if (testpmd_cl != NULL)
+		cmdline_quit(testpmd_cl);
 }
 
 static void
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 98ae46d..1319917 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1570,13 +1570,16 @@ pmd_test_exit(void)
 	if (test_done == 0)
 		stop_packet_forwarding();
 
-	FOREACH_PORT(pt_id, ports) {
-		printf("Stopping port %d...", pt_id);
-		fflush(stdout);
-		rte_eth_dev_close(pt_id);
-		printf("done\n");
+	if (ports != NULL) {
+		no_link_check = 1;
+		FOREACH_PORT(pt_id, ports) {
+			printf("\nShutting down port %d...\n", pt_id);
+			fflush(stdout);
+			stop_port(pt_id);
+			close_port(pt_id);
+		}
 	}
-	printf("bye...\n");
+	printf("\nBye...\n");
 }
 
 typedef void (*cmd_func_t)(void);
@@ -1984,12 +1987,35 @@ init_port(void)
 		ports[pid].enabled = 1;
 }
 
+static void
+force_quit(void)
+{
+	pmd_test_exit();
+	prompt_exit();
+}
+
+static void
+signal_handler(int signum)
+{
+	if (signum == SIGINT || signum == SIGTERM) {
+		printf("\nSignal %d received, preparing to exit...\n",
+				signum);
+		force_quit();
+		/* exit with the expected status */
+		signal(signum, SIG_DFL);
+		kill(getpid(), signum);
+	}
+}
+
 int
 main(int argc, char** argv)
 {
 	int  diag;
 	uint8_t port_id;
 
+	signal(SIGINT, signal_handler);
+	signal(SIGTERM, signal_handler);
+
 	diag = rte_eal_init(argc, argv);
 	if (diag < 0)
 		rte_panic("Cannot init EAL\n");
@@ -2041,6 +2067,7 @@ main(int argc, char** argv)
 		start_packet_forwarding(0);
 		printf("Press enter to exit\n");
 		rc = read(0, &c, 1);
+		pmd_test_exit();
 		if (rc < 0)
 			return 1;
 	}
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index ee7de98..7ffc17b 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -462,6 +462,7 @@ unsigned int parse_item_list(char* str, const char* item_name,
 			unsigned int *parsed_items, int check_unique_values);
 void launch_args_parse(int argc, char** argv);
 void prompt(void);
+void prompt_exit(void);
 void nic_stats_display(portid_t port_id);
 void nic_stats_clear(portid_t port_id);
 void nic_xstats_display(portid_t port_id);
-- 
2.5.0

[PATCH v5 2/3] examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd

[Zhihong Wang]

Handle SIGINT and SIGTERM in l2fwd.

Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
Acked-by: Michael Qiu <michael.qiu@intel.com>
---
 examples/l2fwd/main.c | 43 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/examples/l2fwd/main.c b/examples/l2fwd/main.c
index 720fd5a..f35d8a1 100644
--- a/examples/l2fwd/main.c
+++ b/examples/l2fwd/main.c
@@ -44,6 +44,8 @@
 #include <ctype.h>
 #include <errno.h>
 #include <getopt.h>
+#include <signal.h>
+#include <stdbool.h>
 
 #include <rte_common.h>
 #include <rte_log.h>
@@ -69,6 +71,8 @@
 #include <rte_mempool.h>
 #include <rte_mbuf.h>
 
+static volatile bool force_quit;
+
 #define RTE_LOGTYPE_L2FWD RTE_LOGTYPE_USER1
 
 #define NB_MBUF   8192
@@ -283,7 +287,7 @@ l2fwd_main_loop(void)
 			portid);
 	}
 
-	while (1) {
+	while (!force_quit) {
 
 		cur_tsc = rte_rdtsc();
 
@@ -491,8 +495,12 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
 	printf("\nChecking link status");
 	fflush(stdout);
 	for (count = 0; count <= MAX_CHECK_TIME; count++) {
+		if (force_quit)
+			return;
 		all_ports_up = 1;
 		for (portid = 0; portid < port_num; portid++) {
+			if (force_quit)
+				return;
 			if ((port_mask & (1 << portid)) == 0)
 				continue;
 			memset(&link, 0, sizeof(link));
@@ -534,6 +542,16 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
 	}
 }
 
+static void
+signal_handler(int signum)
+{
+	if (signum == SIGINT || signum == SIGTERM) {
+		printf("\n\nSignal %d received, preparing to exit...\n",
+				signum);
+		force_quit = true;
+	}
+}
+
 int
 main(int argc, char **argv)
 {
@@ -553,6 +571,10 @@ main(int argc, char **argv)
 	argc -= ret;
 	argv += ret;
 
+	force_quit = false;
+	signal(SIGINT, signal_handler);
+	signal(SIGTERM, signal_handler);
+
 	/* parse application arguments (after the EAL ones) */
 	ret = l2fwd_parse_args(argc, argv);
 	if (ret < 0)
@@ -696,12 +718,25 @@ main(int argc, char **argv)
 
 	check_all_ports_link_status(nb_ports, l2fwd_enabled_port_mask);
 
+	ret = 0;
 	/* launch per-lcore init on every lcore */
 	rte_eal_mp_remote_launch(l2fwd_launch_one_lcore, NULL, CALL_MASTER);
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
-		if (rte_eal_wait_lcore(lcore_id) < 0)
-			return -1;
+		if (rte_eal_wait_lcore(lcore_id) < 0) {
+			ret = -1;
+			break;
+		}
 	}
 
-	return 0;
+	for (portid = 0; portid < nb_ports; portid++) {
+		if ((l2fwd_enabled_port_mask & (1 << portid)) == 0)
+			continue;
+		printf("Closing port %d...", portid);
+		rte_eth_dev_stop(portid);
+		rte_eth_dev_close(portid);
+		printf(" Done\n");
+	}
+	printf("Bye...\n");
+
+	return ret;
 }
-- 
2.5.0

[PATCH v5 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

[Zhihong Wang]

Handle SIGINT and SIGTERM in l3fwd.

Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
Acked-by: Michael Qiu <michael.qiu@intel.com>
---
 examples/l3fwd/main.c | 46 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 4 deletions(-)

diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
index 5b0c2dd..21a5782 100644
--- a/examples/l3fwd/main.c
+++ b/examples/l3fwd/main.c
@@ -41,6 +41,8 @@
 #include <stdarg.h>
 #include <errno.h>
 #include <getopt.h>
+#include <signal.h>
+#include <stdbool.h>
 
 #include <rte_common.h>
 #include <rte_vect.h>
@@ -75,6 +77,8 @@
 #include <cmdline_parse.h>
 #include <cmdline_parse_etheraddr.h>
 
+static volatile bool force_quit;
+
 #define APP_LOOKUP_EXACT_MATCH          0
 #define APP_LOOKUP_LPM                  1
 #define DO_RFC_1812_CHECKS
@@ -1553,7 +1557,7 @@ main_loop(__attribute__((unused)) void *dummy)
 			portid, queueid);
 	}
 
-	while (1) {
+	while (!force_quit) {
 
 		cur_tsc = rte_rdtsc();
 
@@ -1781,6 +1785,8 @@ main_loop(__attribute__((unused)) void *dummy)
 
 		}
 	}
+
+	return 0;
 }
 
 static int
@@ -2516,8 +2522,12 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
 	printf("\nChecking link status");
 	fflush(stdout);
 	for (count = 0; count <= MAX_CHECK_TIME; count++) {
+		if (force_quit)
+			return;
 		all_ports_up = 1;
 		for (portid = 0; portid < port_num; portid++) {
+			if (force_quit)
+				return;
 			if ((port_mask & (1 << portid)) == 0)
 				continue;
 			memset(&link, 0, sizeof(link));
@@ -2559,6 +2569,16 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
 	}
 }
 
+static void
+signal_handler(int signum)
+{
+	if (signum == SIGINT || signum == SIGTERM) {
+		printf("\n\nSignal %d received, preparing to exit...\n",
+				signum);
+		force_quit = true;
+	}
+}
+
 int
 main(int argc, char **argv)
 {
@@ -2579,6 +2599,10 @@ main(int argc, char **argv)
 	argc -= ret;
 	argv += ret;
 
+	force_quit = false;
+	signal(SIGINT, signal_handler);
+	signal(SIGTERM, signal_handler);
+
 	/* pre-init dst MACs for all ports to 02:00:00:00:00:xx */
 	for (portid = 0; portid < RTE_MAX_ETHPORTS; portid++) {
 		dest_eth_addr[portid] = ETHER_LOCAL_ADMIN_ADDR + ((uint64_t)portid << 40);
@@ -2733,12 +2757,26 @@ main(int argc, char **argv)
 
 	check_all_ports_link_status((uint8_t)nb_ports, enabled_port_mask);
 
+	ret = 0;
 	/* launch per-lcore init on every lcore */
 	rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER);
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
-		if (rte_eal_wait_lcore(lcore_id) < 0)
-			return -1;
+		if (rte_eal_wait_lcore(lcore_id) < 0) {
+			ret = -1;
+			break;
+		}
 	}
 
-	return 0;
+	/* stop ports */
+	for (portid = 0; portid < nb_ports; portid++) {
+		if ((enabled_port_mask & (1 << portid)) == 0)
+			continue;
+		printf("Closing port %d...", portid);
+		rte_eth_dev_stop(portid);
+		rte_eth_dev_close(portid);
+		printf(" Done\n");
+	}
+	printf("Bye...\n");
+
+	return ret;
 }
-- 
2.5.0

Re: [PATCH v4 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

[Wang, Zhihong]

> > +#define PORT_IDLE 0
> > +#define PORT_INIT 1
> > +#define PORT_WORK 2
> > +#define PORT_STOP 3
> > +#define PORT_QUIT 4
> 
> Seems ok, but over-complicated.
> I think all you need is just IDLE, INIT, QUIT.

Yes for l2/l3fwd 3 states are enough.
I implement a full state machine so it can also serve as an example on how to do this in other cases, like where stop might be called before or during init.

> Konstantin

Re: [PATCH v4 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

[Stephen Hemminger]

On Thu, 31 Dec 2015 01:44:20 +0000
"Wang, Zhihong" <zhihong.wang@intel.com> wrote:

> > > +#define PORT_IDLE 0
> > > +#define PORT_INIT 1
> > > +#define PORT_WORK 2
> > > +#define PORT_STOP 3
> > > +#define PORT_QUIT 4  
> > 
> > Seems ok, but over-complicated.
> > I think all you need is just IDLE, INIT, QUIT.  
> 
> Yes for l2/l3fwd 3 states are enough.
> I implement a full state machine so it can also serve as an example on how to do this in other cases, like where stop might be called before or during init.

These are examples, it is better to have as little code as necessary to get the
job done. That makes the example clearer.  Adding extra unnecessary complexity
just makes it harder to understand.

Re: [PATCH v3 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

[Wang, Zhihong]

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Wednesday, December 30, 2015 7:30 PM
> To: Wang, Zhihong <zhihong.wang@intel.com>; dev@dpdk.org
> Cc: stephen@networkplumber.org; Qiu, Michael <michael.qiu@intel.com>
> Subject: RE: [PATCH v3 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in
> l3fwd
> 
> 
> 
> > -----Original Message-----
> > From: Wang, Zhihong
> > Sent: Wednesday, December 30, 2015 3:15 AM
> > To: Ananyev, Konstantin; dev@dpdk.org
> > Cc: stephen@networkplumber.org; Qiu, Michael
> > Subject: RE: [PATCH v3 3/3] examples/l3fwd: Handle SIGINT and SIGTERM
> > in l3fwd
> >
> > > > +static uint8_t
> > > > +start_ports(void)
> > > > +{
> > > > +	unsigned portid, nb_ports, avail_ports;
> > > > +	int ret;
> > > > +
> > > > +	nb_ports = rte_eth_dev_count();
> > > > +	avail_ports = 0;
> > > > +	for (portid = 0; portid < nb_ports; portid++) {
> > > > +		if ((enabled_port_mask & (1 << portid)) == 0)
> > > > +			continue;
> > > > +		avail_ports++;
> > > > +		port_started = true;
> > >
> > > Why do you need it at each iteration?
> >
> > Only become true when the first enabled port about to started. In case there's
> no port enabled at all.
> > In my opinion no need to optimize since it's not performance sensitive
> > and the logic is correct :)
> >
> >
> > >
> > > > +		printf("Starting port %d...", portid);
> > > > +		ret = rte_eth_dev_start(portid);
> > > > +		if (ret < 0)
> > > > +			rte_exit(EXIT_FAILURE,
> > > > +					"rte_eth_dev_start: err=%d, port=%d\n",
> > > > +					ret, portid);
> > > > +		/*
> > > > +		 * If enabled, put device in promiscuous mode.
> > > > +		 * This allows IO forwarding mode to forward packets
> > > > +		 * to itself through 2 cross-connected  ports of the
> > > > +		 * target machine.
> > > > +		 */
> > > > +		if (promiscuous_on)
> > > > +			rte_eth_promiscuous_enable(portid);
> > > > +		printf(" Done\n");
> > > > +	}
> > > > +
> > > > +	return avail_ports;
> > > > +}
> >
> > [...]
> >
> > > > +static void
> > > > +signal_handler(int signum)
> > > > +{
> > > > +	if (signum == SIGINT || signum == SIGTERM) {
> > > > +		printf("\nSignal %d received, preparing to exit...\n",
> > > > +				signum);
> > > > +		if (port_started) {
> > > > +			printf("Ports started already...\n");
> > > > +			signo_quit = signum;
> > > > +			force_quit = true;
> > > > +		} else {
> > >
> > >
> > > Hmm, and what if signal_handler() would be executed not in the
> > > context of master lcore?
> > > Then there could be a raise condition, and you could end up here,
> > > while master lcore would be in the middle of
> start_ports()->rte_eth_dev_start().
> >
> > Good point! Then we need rte_atomic16_cmpset() to avoid the race condition.
> >
> >
> > > Probably not a big deal, but why do you need this  if (port_started)
> > > {...} else {...} at all?
> > > Why not just:
> >
> > If no port has been started, then just kill itself.
> > This is for cases like when you just started it and then want to shut
> > it down, it'll wait a long time for initialization (memory, etc.) before the
> force_quit signal take effect.
> 
> Do you mean rte_eal_init()?
> Then why not to install non-default signal handlers after rte_eal_init()?
> Konstantin

Yes that does sounds better :)



> 
> >
> >
> > >
> > > signal_handler(int signum)
> > > {
> > > 	signo_quit = signum;
> > > 	force_quit = true;
> > > }
> > > ?
> > >
> > > Konstantin
> > >
> > > > +			printf("Ports not started yet...\n");
> > > > +			printf("Bye...\n");
> > > > +			/* exit with the expected status */
> > > > +			signal(signum, SIG_DFL);
> > > > +			kill(getpid(), signum);
> > > > +		}
> > > > +	}
> > > > +}
> > > > +

Re: [PATCH v4 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

[Wang, Zhihong]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, December 31, 2015 10:09 AM
> To: Wang, Zhihong <zhihong.wang@intel.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org; Qiu,
> Michael <michael.qiu@intel.com>
> Subject: Re: [PATCH v4 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in
> l3fwd
> 
> On Thu, 31 Dec 2015 01:44:20 +0000
> "Wang, Zhihong" <zhihong.wang@intel.com> wrote:
> 
> > > > +#define PORT_IDLE 0
> > > > +#define PORT_INIT 1
> > > > +#define PORT_WORK 2
> > > > +#define PORT_STOP 3
> > > > +#define PORT_QUIT 4
> > >
> > > Seems ok, but over-complicated.
> > > I think all you need is just IDLE, INIT, QUIT.
> >
> > Yes for l2/l3fwd 3 states are enough.
> > I implement a full state machine so it can also serve as an example on how to
> do this in other cases, like where stop might be called before or during init.
> 
> These are examples, it is better to have as little code as necessary to get the job
> done. That makes the example clearer.  Adding extra unnecessary complexity
> just makes it harder to understand.


Thanks for the suggestions!
I'll send the v5 combining your comments and Konstantin's together to make it simpler.

Re: [PATCH v5 2/3] examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd

[Stephen Hemminger]

On Wed, 30 Dec 2015 16:59:50 -0500
Zhihong Wang <zhihong.wang@intel.com> wrote:

> +static void
> +signal_handler(int signum)
> +{
> +	if (signum == SIGINT || signum == SIGTERM) {
> +		printf("\n\nSignal %d received, preparing to exit...\n",
> +				signum);
> +		force_quit = true;

Actually, the if () is redundant since you only registered SIGINT, and SIGTERM those are the
only signals you could possibly receive.

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

Re: [PATCH v5 2/3] examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd

[Wang, Zhihong]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Friday, January 1, 2016 1:02 AM
> To: Wang, Zhihong <zhihong.wang@intel.com>
> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>; Qiu,
> Michael <michael.qiu@intel.com>
> Subject: Re: [PATCH v5 2/3] examples/l2fwd: Handle SIGINT and SIGTERM in
> l2fwd
> 
> On Wed, 30 Dec 2015 16:59:50 -0500
> Zhihong Wang <zhihong.wang@intel.com> wrote:
> 
> > +static void
> > +signal_handler(int signum)
> > +{
> > +	if (signum == SIGINT || signum == SIGTERM) {
> > +		printf("\n\nSignal %d received, preparing to exit...\n",
> > +				signum);
> > +		force_quit = true;
> 
> Actually, the if () is redundant since you only registered SIGINT, and SIGTERM
> those are the only signals you could possibly receive.

Yes it's kind of an obsession I guess, just want to make the code crystal clear :)

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

Re: [PATCH v5 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

[Ananyev, Konstantin]

> -----Original Message-----
> From: Wang, Zhihong
> Sent: Wednesday, December 30, 2015 10:00 PM
> To: dev@dpdk.org
> Cc: Ananyev, Konstantin; stephen@networkplumber.org; Qiu, Michael; Wang, Zhihong
> Subject: [PATCH v5 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd
> 
> Handle SIGINT and SIGTERM in l3fwd.
> 
> Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
> Acked-by: Michael Qiu <michael.qiu@intel.com>
> ---

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Re: [PATCH v5 0/3] Handle SIGINT and SIGTERM in DPDK examples

[Thomas Monjalon]

2015-12-30 16:59, Zhihong Wang:
> Zhihong Wang (3):
>   app/test-pmd: Handle SIGINT and SIGTERM in testpmd
>   examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd
>   examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

Applied, thanks