Re: [dpdk-dev] [PATCH v2 6/7] net/mlx4: convert to new Tx offloads API

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

Hi Shahaf,

Some relatively minor nits mostly unrelated to functionality, please see
below.

On Wed, Jan 03, 2018 at 09:16:16AM +0200, Shahaf Shuler wrote:
> Ethdev Tx offloads API has changed since:
> 
> commit cba7f53b717d ("ethdev: introduce Tx queue offloads API")
> 
> This commit support the new Tx offloads API.
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> ---
>  drivers/net/mlx4/mlx4_ethdev.c |  7 +---
>  drivers/net/mlx4/mlx4_rxtx.h   |  1 +
>  drivers/net/mlx4/mlx4_txq.c    | 71 +++++++++++++++++++++++++++++++++++--
>  3 files changed, 70 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/mlx4/mlx4_ethdev.c b/drivers/net/mlx4/mlx4_ethdev.c
> index 2f69e7d4f..63e00b1da 100644
> --- a/drivers/net/mlx4/mlx4_ethdev.c
> +++ b/drivers/net/mlx4/mlx4_ethdev.c
> @@ -767,17 +767,12 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info)
>  	info->max_tx_queues = max;
>  	info->max_mac_addrs = RTE_DIM(priv->mac);
>  	info->rx_offload_capa = 0;
> -	info->tx_offload_capa = 0;
> +	info->tx_offload_capa = mlx4_priv_get_tx_port_offloads(priv);
>  	if (priv->hw_csum) {
> -		info->tx_offload_capa |= (DEV_TX_OFFLOAD_IPV4_CKSUM |
> -					  DEV_TX_OFFLOAD_UDP_CKSUM |
> -					  DEV_TX_OFFLOAD_TCP_CKSUM);
>  		info->rx_offload_capa |= (DEV_RX_OFFLOAD_IPV4_CKSUM |
>  					  DEV_RX_OFFLOAD_UDP_CKSUM |
>  					  DEV_RX_OFFLOAD_TCP_CKSUM);
>  	}
> -	if (priv->hw_csum_l2tun)
> -		info->tx_offload_capa |= DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM;
>  	if (mlx4_get_ifname(priv, &ifname) == 0)
>  		info->if_index = if_nametoindex(ifname);
>  	info->hash_key_size = MLX4_RSS_HASH_KEY_SIZE;
> diff --git a/drivers/net/mlx4/mlx4_rxtx.h b/drivers/net/mlx4/mlx4_rxtx.h
> index b93e2bcda..91971c4fb 100644
> --- a/drivers/net/mlx4/mlx4_rxtx.h
> +++ b/drivers/net/mlx4/mlx4_rxtx.h
> @@ -184,6 +184,7 @@ int mlx4_tx_queue_setup(struct rte_eth_dev *dev, uint16_t idx,
>  			uint16_t desc, unsigned int socket,
>  			const struct rte_eth_txconf *conf);
>  void mlx4_tx_queue_release(void *dpdk_txq);
> +uint64_t mlx4_priv_get_tx_port_offloads(struct priv *priv);

No need for "priv_" prefixes (or anywhere in function names) in the mlx4 PMD
anymore since DPDK 17.11. Visible symbols only need to be prefixed with
"mlx4_" to tell them apart from mlx5's.

Also the declaration in mlx4_rxtx.h should appear in the same order as in
mlx4_txq.c, that is, before mlx4_tx_queue_setup().

>  /**
>   * Get memory region (MR) <-> memory pool (MP) association from txq->mp2mr[].
> diff --git a/drivers/net/mlx4/mlx4_txq.c b/drivers/net/mlx4/mlx4_txq.c
> index d651e4980..f74e4a735 100644
> --- a/drivers/net/mlx4/mlx4_txq.c
> +++ b/drivers/net/mlx4/mlx4_txq.c
> @@ -182,6 +182,53 @@ mlx4_txq_fill_dv_obj_info(struct txq *txq, struct mlx4dv_obj *mlxdv)
>  }
>  
>  /**
> + * Returns the per-port supported offloads.
> + *
> + * @param priv
> + *   Pointer to private structure.
> + *
> + * @return
> + *   Supported Tx offloads.
> + */
> +uint64_t
> +mlx4_priv_get_tx_port_offloads(struct priv *priv)
> +{

Please remove "priv_" as previously described.

> +	uint64_t offloads = DEV_TX_OFFLOAD_MULTI_SEGS;
> +
> +	if (priv->hw_csum) {
> +		offloads |= (DEV_TX_OFFLOAD_IPV4_CKSUM |
> +			     DEV_TX_OFFLOAD_UDP_CKSUM |
> +			     DEV_TX_OFFLOAD_TCP_CKSUM);
> +	}
> +	if (priv->hw_csum_l2tun)
> +		offloads |= DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM;
> +

Unnecessary empty line.

> +	return offloads;
> +}
> +
> +/**
> + * Checks if the per-queue offload configuration is valid.
> + *
> + * @param priv
> + *   Pointer to private structure.
> + * @param offloads
> + *   Per-queue offloads configuration.
> + *
> + * @return
> + *   1 if the configuration is valid, 0 otherwise.

Better described as "Nonzero when configuration is valid."

> + */
> +static int
> +priv_is_tx_queue_offloads_allowed(struct priv *priv, uint64_t offloads)

s/priv_/mlx4_/ (no prefix also allowed since it's static)

Not be super picky, "is" followed by "offloads allowed" sounds odd, how
about:

 [mlx4_]check_tx_queue_offloads()

> +{
> +	uint64_t port_offloads = priv->dev->data->dev_conf.txmode.offloads;
> +	uint64_t port_supp_offloads = mlx4_priv_get_tx_port_offloads(priv);

Instead of a redundant "port_", how about clarifying it all as follows:

 offloads -> requested
 port_offloads -> mandatory
 port_supp_offloads -> supported

> +
> +	if ((port_offloads ^ offloads) & port_supp_offloads)
> +		return 0;
> +	return 1;

And simplify this as:

 return !((mandatory ^ requested) & supported);

Maybe I missed something, but there seems to be an inconsistency,
e.g. requesting an unsupported offload does not necessarily fail:
 
 mandatory = 0x00
 requested = 0x40
 supported = 0x10

 => valid but shouldn't be

And requesting a supported offload when there are no mandatory ones should
not be a problem:

 mandatory = 0x00
 requested = 0x10
 supported = 0x10

 => invalid but it should be

A naive translation of the above requirements results in the following
expression:

 return (requested | supported) == supported &&
        (requested & mandatory) == mandatory;

What's your opinion?

> +}
> +
> +/**
>   * DPDK callback to configure a Tx queue.
>   *
>   * @param dev
> @@ -229,9 +276,22 @@ mlx4_tx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc,
>  	};
>  	int ret;
>  
> -	(void)conf; /* Thresholds configuration (ignored). */
>  	DEBUG("%p: configuring queue %u for %u descriptors",
>  	      (void *)dev, idx, desc);
> +	/*
> +	 * Don't verify port offloads for application which
> +	 * use the old API.
> +	 */
> +	if (!!(conf->txq_flags & ETH_TXQ_FLAGS_IGNORE) &&

Enclosing "!!(...)" seems unnecessary, only the fact the result is nonzero
matters.

> +	    !priv_is_tx_queue_offloads_allowed(priv, conf->offloads)) {
> +		rte_errno = ENOTSUP;
> +		ERROR("%p: Tx queue offloads 0x%lx don't match port "
> +		      "offloads 0x%lx or supported offloads 0x%lx",

"%lx" may cause a compilation error depending on the platform, you need to
use "%" PRIx64 after including inttypes.h.

> +		      (void *)dev, conf->offloads,
> +		      dev->data->dev_conf.txmode.offloads,
> +		      mlx4_priv_get_tx_port_offloads(priv));
> +		return -rte_errno;
> +	}
>  	if (idx >= dev->data->nb_tx_queues) {
>  		rte_errno = EOVERFLOW;
>  		ERROR("%p: queue index out of range (%u >= %u)",
> @@ -281,8 +341,13 @@ mlx4_tx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc,
>  			RTE_MIN(MLX4_PMD_TX_PER_COMP_REQ, desc / 4),
>  		.elts_comp_cd_init =
>  			RTE_MIN(MLX4_PMD_TX_PER_COMP_REQ, desc / 4),
> -		.csum = priv->hw_csum,
> -		.csum_l2tun = priv->hw_csum_l2tun,
> +		.csum = priv->hw_csum &&
> +			(conf->offloads & (DEV_TX_OFFLOAD_IPV4_CKSUM |
> +					   DEV_TX_OFFLOAD_UDP_CKSUM |
> +					   DEV_TX_OFFLOAD_TCP_CKSUM)),
> +		.csum_l2tun = priv->hw_csum_l2tun &&
> +			      (conf->offloads &
> +			       DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM),
>  		/* Enable Tx loopback for VF devices. */
>  		.lb = !!priv->vf,
>  		.bounce_buf = bounce_buf,
> -- 
> 2.12.0
> 

-- 
Adrien Mazarguil
6WIND