Re: [PATCH v6] distributor_app: new sample app

["De Lara Guarch, Pablo" <pablo.de.lara.guarch-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

Hi Reshma,

> -----Original Message-----
> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Reshma Pattan
[...]

> diff --git a/examples/distributor_app/main.c
> b/examples/distributor_app/main.c
> new file mode 100644
> index 0000000..c9994a6
> --- /dev/null
> +++ b/examples/distributor_app/main.c
> @@ -0,0 +1,642 @@

[...]

> +static const struct rte_eth_rxconf rx_conf_default = {
> +	.rx_thresh = {
> +		.pthresh = RX_PTHRESH,
> +		.hthresh = RX_HTHRESH,
> +		.wthresh = RX_WTHRESH,
> +	},
> +	.rx_free_thresh = RX_FREE_THRESH,
> +	.rx_drop_en = 0,
> +};
> +
> +static const struct rte_eth_txconf tx_conf_default = {
> +	.tx_thresh = {
> +		.pthresh = TX_PTHRESH,
> +		.hthresh = TX_HTHRESH,
> +		.wthresh = TX_WTHRESH,
> +	},
> +	.tx_free_thresh = TX_FREE_THRESH,
> +	.tx_rs_thresh = TX_RSBIT_THRESH,
> +	.txq_flags = TX_Q_FLAGS
> +
> +};

You can remove these two structures (and all the macros involved),
if you pass NULL to rxconf/txconf parameters when calling RX/TX queue setup.

[...]

> +static inline int
> +port_init(uint8_t port, struct rte_mempool *mbuf_pool)
> +{
> +	struct rte_eth_conf port_conf = port_conf_default;
> +	const uint16_t rxRings = 1, txRings = rte_lcore_count() - 1;
> +	int retval;
> +	uint16_t q;
> +
> +	if (port >= rte_eth_dev_count())
> +		return -1;
> +
> +	retval = rte_eth_dev_configure(port, rxRings, txRings, &port_conf);
> +	if (retval != 0)
> +		return retval;
> +
> +	for (q = 0; q < rxRings; q++) {
> +		retval = rte_eth_rx_queue_setup(port, q, RX_RING_SIZE,
> +
> 	rte_eth_dev_socket_id(port),
> +						&rx_conf_default,
> mbuf_pool);

Recently, a new functionality has been added for RX/TX queue setup. 
You can now use NULL as rxconf/txconf parameter, so it will use 
the optimal configuration, so it will save you some lines of code and simplify it.

> +		if (retval < 0)
> +			return retval;
> +	}
> +
> +	for (q = 0; q < txRings; q++) {
> +		retval = rte_eth_tx_queue_setup(port, q, TX_RING_SIZE,
> +
> 	rte_eth_dev_socket_id(port),
> +						&tx_conf_default);
> +		if (retval < 0)
> +			return retval;
> +	}
> +
> +	retval  = rte_eth_dev_start(port);

Remove second space before "=".

> +	if (retval < 0)
> +		return retval;
> +

[...]

> +	rte_distributor_process(d, NULL, 0);
> +	/* flush distributor to bring to known state */
> +	rte_distributor_flush(d);
> +	/* set worker & tx threads quit flag */
> +	quit_signal = 1;
> +	/* worker threads may hang in get packet as
> +	   distributor process is not runnig, just make sure workers
> +	   gets packets till quit_signal is actually been
> +	   received and they gracefully shutdown*/

Typos in "running" and "workers gets". Plus, use standard multi-line comments.

> +	quit_workers(d, mem_pool);
> +	/* rx thread should quit at last*/
> +	return 0;
> +}
> +

 [...]

> +static int
> +lcore_worker(struct lcore_params *p)
> +{
> +	struct rte_distributor *d = p->d;
> +	const unsigned id = p->worker_id;
> +	/* for single port, xor_val will be zero so we won't modify the output
> +	 * port, otherwise we send traffic from 0 to 1, 2 to 3, and vice versa
> +	 */

Move "/*" to the first line, to be consistent with other multi-line comments.
 
> +	const unsigned xor_val = (rte_eth_dev_count() > 1);
> +	struct rte_mbuf *buf = NULL;
> +
> +	printf("\nCore %u acting as worker core.\n", rte_lcore_id());
> +	while (!quit_signal) {
> +		buf = rte_distributor_get_pkt(d, id, buf);
> +		buf->port ^= xor_val;
> +	}
> +	return 0;

[...]

> +			default:
> +				print_usage(prgname);
> +				return -1;
> +		}
> +	}
> +
> +if (optind <= 1) {
> +	print_usage(prgname);
> +	return -1;
> +}

Missing indentation.

> +
> +	argv[optind-1] = prgname;
> +
> +	optind = 0; /* reset getopt lib */
> +	return 0;
> +}

Thanks,
Pablo