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

[Mordechay Haimovsky]

Inline

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com]
> Sent: Wednesday, November 8, 2017 4:58 PM
> To: Mordechay Haimovsky <motih at mellanox.com>
> Cc: dev at dpdk.org
> Subject: Re: [PATCH v1] net/mlx4: improve Rx packet type offloads report
> 
> 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.
> 
In VFs (sriov VF devices) we see that the L2 tunnel flag is set which causes a complete misinterpretation of the packet type being received.
This happens since the tunnel_mode is not set to 0x7 by the driver and therefore has meaningless value in such case and should be ignored.
And yes, performance-wise there is probably an impact.
Another approach which will not affect performance can be to init the mlx4_ptype_table according to the device at hand (i.e. per-device table).
This however will require some effort :)

> 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