On Wed, Nov 08, 2017 at 03:33:57PM +0000, Mordechay Haimovsky wrote:
> 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.
OK, please mention this in the commit log then, otherwise it doesn't look
like this patch addresses anything.
> 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 :)
My remaining comments still stand, particularly the need to update
mlx4_dev_supported_ptypes_get() as well.
>
> > 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
--
Adrien Mazarguil
6WIND
