Inline
> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com]
> Sent: Wednesday, November 8, 2017 4:58 PM
> To: Mordechay Haimovsky <mo...@mellanox.com>
> Cc: dev@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 <mo...@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
