[dpdk-dev] [PATCH v1] net/mlx4: improve Rx packet type offloads report

[Adrien Mazarguil]

Hi Moti,

On Wed, Nov 08, 2017 at 02:02:45PM +0200, Moti Haimovsky wrote:
> This patch improves Rx packet type offload report in case the device is
> a virtual function device. L2 tunnel indications are not reported by
> those devices and therefore should not be checked by the PMD.
> 
> Fixes: aee4a03fee4f ("net/mlx4: enhance Rx packet type offloads")
> 
> Signed-off-by: Moti Haimovsky <motih at mellanox.com>

Does "not reporting them" cause any issue? Is this patch adding anything on
top of checking they can't be reported before not reporting them either?

Otherwise this additional unnecessary check may cause a minor performance
regression for no good reason.

I think this patch should really update mlx4_dev_supported_ptypes_get()
(control path) instead of rxq_cq_to_pkt_type() (data path). What's your
opinion?

A few other suggestions, see below.

> ---
>  drivers/net/mlx4/mlx4.c      | 2 ++
>  drivers/net/mlx4/mlx4.h      | 1 +
>  drivers/net/mlx4/mlx4_rxq.c  | 1 +
>  drivers/net/mlx4/mlx4_rxtx.c | 9 ++++++---
>  drivers/net/mlx4/mlx4_rxtx.h | 1 +
>  5 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
> index f9e4f9d..efff65d 100644
> --- a/drivers/net/mlx4/mlx4.c
> +++ b/drivers/net/mlx4/mlx4.c
> @@ -573,6 +573,8 @@ struct mlx4_conf {
>  			 PCI_DEVICE_ID_MELLANOX_CONNECTX3PRO);
>  		DEBUG("L2 tunnel checksum offloads are %ssupported",
>  		      (priv->hw_csum_l2tun ? "" : "not "));
> +		/* VFs are not configured to offload L2 tunnels */
> +		priv->hw_l2tun_offload = !vf;

Seeing how you're adding a new bit to this field, is hw_l2tun_offload really
different from hw_csum_l2tun? 

Can you get inner VXLAN checksums if the packet can't be recognized as VXLAN
in the first place? I don't think so.

Perhaps hw_csum_l2tun should be renamed, however in my opinion you could
simply update the hw_csum_l2tun assignment with a vf check and use that.

>  		/* Configure the first MAC address by default. */
>  		if (mlx4_get_mac(priv, &mac.addr_bytes)) {
>  			ERROR("cannot get MAC address, is mlx4_en loaded?"
> diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h
> index 3aeef87..cbb8628 100644
> --- a/drivers/net/mlx4/mlx4.h
> +++ b/drivers/net/mlx4/mlx4.h
> @@ -128,6 +128,7 @@ struct priv {
>  	uint32_t isolated:1; /**< Toggle isolated mode. */
>  	uint32_t hw_csum:1; /* Checksum offload is supported. */
>  	uint32_t hw_csum_l2tun:1; /* Checksum support for L2 tunnels. */
> +	uint32_t hw_l2tun_offload:1; /**< L2 tunnel offload is configured. */

This change would become unnecessary.

>  	struct rte_intr_handle intr_handle; /**< Port interrupt handle. */
>  	struct mlx4_drop *drop; /**< Shared resources for drop flow rules. */
>  	LIST_HEAD(, mlx4_rss) rss; /**< Shared targets for Rx flow rules. */
> diff --git a/drivers/net/mlx4/mlx4_rxq.c b/drivers/net/mlx4/mlx4_rxq.c
> index 8b97a89..802be84 100644
> --- a/drivers/net/mlx4/mlx4_rxq.c
> +++ b/drivers/net/mlx4/mlx4_rxq.c
> @@ -750,6 +750,7 @@ struct mlx4_rss *
>  			 dev->data->dev_conf.rxmode.hw_ip_checksum),
>  		.csum_l2tun = (priv->hw_csum_l2tun &&
>  			       dev->data->dev_conf.rxmode.hw_ip_checksum),
> +		.l2tun_offload = priv->hw_l2tun_offload,

Assuming a data path change is really needed, this one could likely stay
since it doesn't depend on the user enabling checksums.

>  		.stats = {
>  			.idx = idx,
>  		},
> diff --git a/drivers/net/mlx4/mlx4_rxtx.c b/drivers/net/mlx4/mlx4_rxtx.c
> index 3985e06..1131d56 100644
> --- a/drivers/net/mlx4/mlx4_rxtx.c
> +++ b/drivers/net/mlx4/mlx4_rxtx.c
> @@ -37,6 +37,7 @@
>   */
>  
>  #include <assert.h>
> +#include <stdbool.h>

What for?

>  #include <stdint.h>
>  #include <string.h>
>  
> @@ -751,7 +752,8 @@ struct pv {
>   *   Packet type for struct rte_mbuf.
>   */
>  static inline uint32_t
> -rxq_cq_to_pkt_type(volatile struct mlx4_cqe *cqe)
> +rxq_cq_to_pkt_type(volatile struct mlx4_cqe *cqe,
> +		   uint32_t l2tun_offload)
>  {
>  	uint8_t idx = 0;
>  	uint32_t pinfo = rte_be_to_cpu_32(cqe->vlan_my_qpn);
> @@ -762,7 +764,7 @@ struct pv {
>  	 *  bit[7] - MLX4_CQE_L2_TUNNEL
>  	 *  bit[6] - MLX4_CQE_L2_TUNNEL_IPV4
>  	 */
> -	if (!(pinfo & MLX4_CQE_L2_VLAN_MASK) && (pinfo & MLX4_CQE_L2_TUNNEL))
> +	if (l2tun_offload && (pinfo & MLX4_CQE_L2_TUNNEL))
>  		idx |= ((pinfo & MLX4_CQE_L2_TUNNEL) >> 20) |
>  		       ((pinfo & MLX4_CQE_L2_TUNNEL_IPV4) >> 19);
>  	/*
> @@ -960,7 +962,8 @@ struct pv {
>  			}
>  			pkt = seg;
>  			/* Update packet information. */
> -			pkt->packet_type = rxq_cq_to_pkt_type(cqe);
> +			pkt->packet_type =
> +				rxq_cq_to_pkt_type(cqe, rxq->l2tun_offload);
>  			pkt->ol_flags = 0;
>  			pkt->pkt_len = len;
>  			if (rxq->csum | rxq->csum_l2tun) {
> diff --git a/drivers/net/mlx4/mlx4_rxtx.h b/drivers/net/mlx4/mlx4_rxtx.h
> index 4acad80..463df2b 100644
> --- a/drivers/net/mlx4/mlx4_rxtx.h
> +++ b/drivers/net/mlx4/mlx4_rxtx.h
> @@ -80,6 +80,7 @@ struct rxq {
>  	volatile uint32_t *rq_db; /**< RQ doorbell record. */
>  	uint32_t csum:1; /**< Enable checksum offloading. */
>  	uint32_t csum_l2tun:1; /**< Same for L2 tunnels. */
> +	uint32_t l2tun_offload:1; /**< L2 tunnel offload is enabled. */
>  	struct mlx4_cq mcq;  /**< Info for directly manipulating the CQ. */
>  	struct mlx4_rxq_stats stats; /**< Rx queue counters. */
>  	unsigned int socket; /**< CPU socket ID for allocations. */
> -- 
> 1.8.3.1
> 

-- 
Adrien Mazarguil
6WIND