[dpdk-dev] [PATCH v2 2/3] examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd
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
> 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.
[dpdk-dev] [PATCH v2 2/3] examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd
On Thu, 24 Dec 2015 21:37:11 -0500
Zhihong Wang wrote:
> Handle SIGINT and SIGTERM in l2fwd.
>
> Signed-off-by: Zhihong Wang
> ---
> 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
> #include
> #include
> +#include
> +#include
>
> #include
> #include
> @@ -69,6 +71,9 @@
> #include
> #include
>
> +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 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.
[dpdk-dev] [PATCH v2 2/3] examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd
Handle SIGINT and SIGTERM in l2fwd.
Signed-off-by: Zhihong Wang
---
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
#include
#include
+#include
+#include
#include
#include
@@ -69,6 +71,9 @@
#include
#include
+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
