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

["Wang, Zhihong" <zhihong.wang@intel.com>]

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.