Re: [dpdk-dev] [PATCH v3 3/3] examples/l3fwd: Handle SIGINT and SIGTERM in l3fwd

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

> > +static uint8_t
> > +start_ports(void)
> > +{
> > +	unsigned portid, nb_ports, avail_ports;
> > +	int ret;
> > +
> > +	nb_ports = rte_eth_dev_count();
> > +	avail_ports = 0;
> > +	for (portid = 0; portid < nb_ports; portid++) {
> > +		if ((enabled_port_mask & (1 << portid)) == 0)
> > +			continue;
> > +		avail_ports++;
> > +		port_started = true;
> 
> Why do you need it at each iteration?

Only become true when the first enabled port about to started. In case there's no port enabled at all.
In my opinion no need to optimize since it's not performance sensitive and the logic is correct :)


> 
> > +		printf("Starting port %d...", portid);
> > +		ret = rte_eth_dev_start(portid);
> > +		if (ret < 0)
> > +			rte_exit(EXIT_FAILURE,
> > +					"rte_eth_dev_start: err=%d, port=%d\n",
> > +					ret, portid);
> > +		/*
> > +		 * If enabled, put device in promiscuous mode.
> > +		 * This allows IO forwarding mode to forward packets
> > +		 * to itself through 2 cross-connected  ports of the
> > +		 * target machine.
> > +		 */
> > +		if (promiscuous_on)
> > +			rte_eth_promiscuous_enable(portid);
> > +		printf(" Done\n");
> > +	}
> > +
> > +	return avail_ports;
> > +}

[...]

> > +static void
> > +signal_handler(int signum)
> > +{
> > +	if (signum == SIGINT || signum == SIGTERM) {
> > +		printf("\nSignal %d received, preparing to exit...\n",
> > +				signum);
> > +		if (port_started) {
> > +			printf("Ports started already...\n");
> > +			signo_quit = signum;
> > +			force_quit = true;
> > +		} else {
> 
> 
> Hmm, and what if signal_handler() would be executed not in the context of
> master lcore?
> Then there could be a raise condition, and you could end up here, while master
> lcore would be in the middle of start_ports()->rte_eth_dev_start().

Good point! Then we need rte_atomic16_cmpset() to avoid the race condition.


> Probably not a big deal, but why do you need this  if (port_started) {...} else {...}
> at all?
> Why not just:

If no port has been started, then just kill itself.
This is for cases like when you just started it and then want to shut it down, it'll wait a long time for initialization (memory, etc.) before the force_quit signal take effect.


> 
> signal_handler(int signum)
> {
> 	signo_quit = signum;
> 	force_quit = true;
> }
> ?
> 
> Konstantin
> 
> > +			printf("Ports not started yet...\n");
> > +			printf("Bye...\n");
> > +			/* exit with the expected status */
> > +			signal(signum, SIG_DFL);
> > +			kill(getpid(), signum);
> > +		}
> > +	}
> > +}
> > +