Thread (36 messages) 36 messages, 5 authors, 2016-01-27

Re: [PATCH v2 2/3] examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd

From: Wang, Zhihong <hidden>
Date: 2015-12-28 01:35:39

Hi Stephen,

Really appreciate the detailed review!
Please see comments below.

quoted
+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.
quoted
 #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) {
quoted
 		cur_tsc = rte_rdtsc();
@@ -534,6 +541,45 @@ check_all_ports_link_status(uint8_t port_num,
uint32_t port_mask)
quoted
 	}
 }

+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.
quoted
+		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.
quoted
 	/* 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.
quoted
 	rte_eal_mp_remote_launch(l2fwd_launch_one_lcore, NULL,
CALL_MASTER);
quoted
 	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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help