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.