From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f50.google.com (mail-wm0-f50.google.com [74.125.82.50]) by dpdk.org (Postfix) with ESMTP id 26DF31B2A4 for ; Wed, 8 Nov 2017 15:58:11 +0100 (CET) Received: by mail-wm0-f50.google.com with SMTP id n74so18336474wmi.1 for ; Wed, 08 Nov 2017 06:58:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=R7beENysJUcZid9evfze4ZBWrDM+miWQDAZ/I3iBuwE=; b=d97f4VgmQKqB7wZMCOKXkXDUX0sQDMAHgbVnWIueXpqvAAgQdTUoyqs18DFVMJlIiH 8XqGuVsKqiKld7tBHvr32Lq2F1uH+glscQRxakXzY8wKEDncF0SHWqXg4xHbYWQqL0k0 /71M6Y7CYoDNlEYm0mm+kFwJSLN8QlXuJngsISAi8iQGElmL4ABK3Lurp7O4105C9egC NV7ggLgZ2HFDv89yoe8XaX3eLB2/xw/9TtbbUSz96rTfFchrmITT4DaKvhiYd78orUtv Zw8x/ok2cM0HELrb15cHWIDgZt485Ffzl3X/fv+4SMonW514uylGHkJK0okn3jvRnC5t fPyg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=R7beENysJUcZid9evfze4ZBWrDM+miWQDAZ/I3iBuwE=; b=Nfd6BKlbdNm8/GmWODP6qNina0PD44bL6OkpuGUKVJ/99JFr5daYFa7RyJWShvcMW6 2lS39LhGLpP+ACzC4oSoBWaIwagBBgGy/mPCcbwQDOBKiu/WzqxjVvsTCgBOX7XnetO5 TP7nxGs2SDMPXWy+rVMwYD0Hk2OoT06lIqoOW5t7+ZXu8hPwAwQN+xy05R4wGbdL0DPf Eh24tEYuf/lGds9URPF4Na9aSIQWWDhKx+dC7y5oprrP00/If21fmI/5vtYQgVQuvgTn QkdA/a+ssz/o/SXOOyvAhIB+4mw3ka0WFkfK/L76/Bmpx/n+BQDaagkiAyXeFHTa+tke hGDA== X-Gm-Message-State: AJaThX68q70ELC2eZl7ywdqHsQn/1HTIH65smXwxCs37vcavN3PoCQWB n+RF3K7TLbWIzY+/oybi5sGbaQ== X-Google-Smtp-Source: ABhQp+QdVKugfPYPKjKmUPeIJKiPs7FAG625Hf1MNDR2DLKys0HD5a67DH5plF5QfnKlW28x39mxgw== X-Received: by 10.80.204.72 with SMTP id n8mr1047990edi.122.1510153090837; Wed, 08 Nov 2017 06:58:10 -0800 (PST) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id s56sm4185177edm.86.2017.11.08.06.58.09 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 08 Nov 2017 06:58:09 -0800 (PST) Date: Wed, 8 Nov 2017 15:57:58 +0100 From: Adrien Mazarguil To: Moti Haimovsky Cc: dev@dpdk.org Message-ID: <20171108145758.GV24849@6wind.com> References: <1510142565-142115-1-git-send-email-motih@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1510142565-142115-1-git-send-email-motih@mellanox.com> Subject: Re: [dpdk-dev] [PATCH v1] net/mlx4: improve Rx packet type offloads report X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Nov 2017 14:58:11 -0000 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 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 > +#include What for? > #include > #include > > @@ -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