[dpdk-dev,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(-)
  

> -----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
  

> > +#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
  

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.
  

> -----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.