From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Mordechay Haimovsky <motih@mellanox.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v1] net/mlx4: improve Rx packet type offloads report
Date: Wed, 8 Nov 2017 16:42:45 +0100 [thread overview]
Message-ID: <20171108154245.GX24849@6wind.com> (raw)
In-Reply-To: <AM4PR05MB1907F0CCECEE0B5E61771220D2560@AM4PR05MB1907.eurprd05.prod.outlook.com>
On Wed, Nov 08, 2017 at 03:33:57PM +0000, Mordechay Haimovsky wrote:
> Inline
>
> > -----Original Message-----
> > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > Sent: Wednesday, November 8, 2017 4:58 PM
> > To: Mordechay Haimovsky <motih@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 <motih@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
prev parent reply other threads:[~2017-11-08 15:42 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-08 12:02 Moti Haimovsky
2017-11-08 14:57 ` Adrien Mazarguil
2017-11-08 15:33 ` Mordechay Haimovsky
2017-11-08 15:42 ` Adrien Mazarguil [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171108154245.GX24849@6wind.com \
--to=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=motih@mellanox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).