[dpdk-dev,v3,3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd
Commit Message
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(-)
Comments
> -----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
> > +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);
> > + }
> > + }
> > +}
> > +
> -----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);
> > > + }
> > > + }
> > > +}
> > > +
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.
> -----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);
> > > > + }
> > > > + }
> > > > +}
> > > > +
@@ -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;
}