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