Re: [PATCH V3 1/2] net/failsafe: convert to new Tx offloads API

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

Hi Moti,

Thanks for the patches.
I have some nitpicks, but overall it is good. Sorry for doing the review
so late, the changes should not take long, it's mostly syntax.

For the commit title, I'd prefer "use" instead of "convert to"

   [PATCH V3 1/2] net/failsafe: use new Tx offloads API

As the old API is still supported and used.

On Wed, Jan 10, 2018 at 04:40:22PM +0200, Moti Haimovsky wrote:

..

> @@ -84,9 +85,18 @@
>  fs_dev_configure(struct rte_eth_dev *dev)
>  {
>  	struct sub_device *sdev;
> +	uint64_t supp_tx_offloads = PRIV(dev)->infos.tx_offload_capa;
> +	uint64_t tx_offloads = dev->data->dev_conf.txmode.offloads;

I'm not fond of compound variable definition and initialization.
Initialization should be done after the definition list.

>  	uint8_t i;
>  	int ret;
>  
> +	if ((tx_offloads & supp_tx_offloads) != tx_offloads) {
> +		rte_errno = ENOTSUP;
> +		ERROR("Some Tx offloads are not supported, "
> +		      "requested 0x%lx supported 0x%lx\n",

Please use PRIx64 instead of %lx for displaying uint64_t.

> +		      tx_offloads, supp_tx_offloads);
> +		return -rte_errno;
> +	}
>  	FOREACH_SUBDEV(sdev, i, dev) {
>  		int rmv_interrupt = 0;
>  		int lsc_interrupt = 0;
> @@ -311,6 +321,22 @@
>  	return ret;
>  }
>  
> +static bool
> +fs_txq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads)

_are seems unnecessary here:

        if (fs_txq_offloads_valid() == false) {
                ...
        }

reads well enough and is shorter.

> +{
> +	uint64_t port_offloads = dev->data->dev_conf.txmode.offloads;
> +	uint64_t queue_supp_offloads = PRIV(dev)->infos.tx_queue_offload_capa;
> +	uint64_t port_supp_offloads = PRIV(dev)->infos.tx_offload_capa;
> +
> +	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
> +	     offloads)
> +		return false;
> +	/* Verify we have no conflict with port offloads */
> +	if ((port_offloads ^ offloads) & port_supp_offloads)
> +		return false;
> +	return true;
> +}
> +
>  static void
>  fs_tx_queue_release(void *queue)
>  {
> @@ -347,6 +373,22 @@
>  		fs_tx_queue_release(txq);
>  		dev->data->tx_queues[tx_queue_id] = NULL;
>  	}
> +	/*
> +	 * Don't verify queue offloads for applications which
> +	 * use the old API.
> +	 */
> +	if (tx_conf != NULL &&
> +	   !!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE) &&

No need for !!

> +	   !fs_txq_are_offloads_valid(dev, tx_conf->offloads)) {

Please check against explicit "false" instead of using '!'.

-- 
Gaëtan Rivet
6WIND