[dpdk-dev] [PATCH v3 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd
Handle SIGINT and SIGTERM in l3fwd. Signed-off-by: Zhihong Wang Acked-by: Michael Qiu --- 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 #include #include +#include +#include +#include #include #include @@ -75,6 +78,10 @@ #include #include +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", - re
[dpdk-dev] [PATCH v3 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd
> -Original Message- > From: Wang, Zhihong > Sent: Tuesday, December 29, 2015 1:21 AM > To: dev at dpdk.org > Cc: Ananyev, Konstantin; stephen at 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 > Acked-by: Michael Qiu > --- > 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 > #include > #include > +#include > +#include > +#include > > #include > #include > @@ -75,6 +78,10 @@ > #include > #include > > +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) > { > @@
[dpdk-dev] [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. > > 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); > > + } > > + } > > +} > > +
[dpdk-dev] [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 at dpdk.org > Cc: stephen at 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); > > > + } > > > + } > > > +} > > > +
[dpdk-dev] [PATCH v3 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd
On Mon, 28 Dec 2015 20:20:32 -0500 Zhihong Wang wrote: > Handle SIGINT and SIGTERM in l3fwd. > > Signed-off-by: Zhihong Wang > Acked-by: Michael Qiu > --- > 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 > #include > #include > +#include > +#include > +#include > > #include > #include > @@ -75,6 +78,10 @@ > #include > #include > > +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.
[dpdk-dev] [PATCH v3 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd
> -Original Message- > From: Ananyev, Konstantin > Sent: Wednesday, December 30, 2015 7:30 PM > To: Wang, Zhihong ; dev at dpdk.org > Cc: stephen at networkplumber.org; Qiu, Michael > 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 at dpdk.org > > Cc: stephen at 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); > > > > + } > > > > + } > > > > +} > > > > +