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

  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(-)
  

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
>
  

Hi Adrien and Nelio,

See below comment regarding your output on the offload check.
Rest of the comments were accepted.

Wednesday, January 3, 2018 7:29 PM, Adrien Mazarguil :

[...]

> 
> > +{
> > +	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.

You are correct that the purpose of this function is to check if the offload configuration is correct.
However the current code being done on mlx4 does not validate if the queue offloads configured are supported. 
It only validates if the port offloads configuration matches the queue offload configuration.

The reason it lack the supported offloads check was discussed in internal mail (you both CC I believe). Generally it was due to the fact that CRC and VLAN strip offloads are not supported by the PMD, however set for almost every example/application in DPDK.
For the complete check look on mlx5 patches on this series. 

> requesting an unsupported offload does not necessarily fail:
> 
>  mandatory = 0x00
>  requested = 0x40
>  supported = 0x10
> 
>  => valid but shouldn't be

It should if the offload is per-queue offload.


> 
> 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

It is invalid indeed. If the application declare some port offload not to be set on dev_configure, it cannot enable it from the queue setup.
Port offloads can be set only on device configuration, and when set every queue should have them set as well.

> 
> A naive translation of the above requirements results in the following
> expression:
> 
>  return (requested | supported) == supported &&
>         (requested & mandatory) == mandatory;
> 
> What's your opinion?
>
  

Hi Shahaf,

On Thu, Jan 04, 2018 at 11:55:17AM +0000, Shahaf Shuler wrote:
> Hi Adrien and Nelio,
> 
> See below comment regarding your output on the offload check.
> Rest of the comments were accepted.
> 
> Wednesday, January 3, 2018 7:29 PM, Adrien Mazarguil :
> 
> [...]
> 
> > 
> > > +{
> > > +	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.
> 
> You are correct that the purpose of this function is to check if the offload configuration is correct.
> However the current code being done on mlx4 does not validate if the queue offloads configured are supported. 
> It only validates if the port offloads configuration matches the queue offload configuration.
> 
> The reason it lack the supported offloads check was discussed in internal mail (you both CC I believe). Generally it was due to the fact that CRC and VLAN strip offloads are not supported by the PMD, however set for almost every example/application in DPDK.
> For the complete check look on mlx5 patches on this series. 
> 
> > requesting an unsupported offload does not necessarily fail:
> > 
> >  mandatory = 0x00
> >  requested = 0x40
> >  supported = 0x10
> > 
> >  => valid but shouldn't be
> 
> It should if the offload is per-queue offload.
> 
> 
> > 
> > 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
> 
> It is invalid indeed. If the application declare some port offload not to be set on dev_configure, it cannot enable it from the queue setup.
> Port offloads can be set only on device configuration, and when set every queue should have them set as well.
> 
> > 
> > A naive translation of the above requirements results in the following
> > expression:
> > 
> >  return (requested | supported) == supported &&
> >         (requested & mandatory) == mandatory;
> > 
> > What's your opinion?
> > 

From an application point of view, it seems strange to provide an
already configured offload when configuring the queues, i.e.
rte_eth_dev_configure() is called before rte_eth_{tx,rx}_queue_setup().

I think this "mandatory" information should be removed from the API
documentation letting the application the capability to request a null
offload when configuring the queue.
As this modification does not break the API/ABI it only needs eventually
a modification in the driver, it can be done in the future.

For mlx5 part:
Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>