From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f67.google.com (mail-wm0-f67.google.com [74.125.82.67]) by dpdk.org (Postfix) with ESMTP id 5EEC144CF for ; Fri, 23 Mar 2018 15:53:23 +0100 (CET) Received: by mail-wm0-f67.google.com with SMTP id t7so4075086wmh.5 for ; Fri, 23 Mar 2018 07:53:23 -0700 (PDT) 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=kjgb4/8Jh7nlftsRbpUafLj07sWnPxHIJB/uyV9zK7Y=; b=x4ZGYBBOKByWHozG1V4z678OFF5k8bucoc5kKPEVfkjmIxCGZo4GMuCbBy3hrfqNUn saGehTDyLSvye/Ib0eWilCfFsQiZjxyUTHq3jl6QVkvIn/Ar9OLKLvC31kY2V4yq2CV8 Y4qQnXxkLRhD9OAEClNrtIJWnG8f2cdxSbFls7Jp0GYzbYZ/HPwBBPcdgRp51YOsbxFj XiIdlGBhVmQzGYlr1htKAW54qlTr6WX+MLWRs3xvpq33XhyB7Q4bCKN05lXcOiiH2V8w mlbax8nmx8YPQddN3R9hFLK/rSEpV+c/WanfCN5HPew338WrDp2LNQd4Xpd1IhmCvJne NFfA== 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=kjgb4/8Jh7nlftsRbpUafLj07sWnPxHIJB/uyV9zK7Y=; b=Nh98HNpGb3NND+kI+KDeQL0XYZCYuqW9RidVHOR5P7mgfCeIgcyHSpPNo+II28P3vr D7cUReJR8zRFsrL2npVteYYGvtPGV2PcYbyh4JlfRm/b3DIVzFDrmfLunytjse7Xb8lO JM4tT2naUVNtrnVIyQ3xtr6Slvkm8pB9IO3118/Vsid3qm34kacClxuZmHzQSMf8OOa/ 11YQuC1RaXxqvDis4N9UIFqFJO+MXviQyDoa287mHNtoMoCKcsVUzzRXMi+lrQM1mBJ+ 3wuHvGpN+EsWvPBxbsQvlWryl2GTOjuOt9sVlh8LXIPcbz9RhAlgGcte+8T0hroH8/zA jvig== X-Gm-Message-State: AElRT7GNusgHX/hCKy7HFW9hk7jEB9/pnXQKeGOQ2qnON59woi8wxh1t Xl4WdClEWoQ84jfl+R8nX0V1mg== X-Google-Smtp-Source: AIpwx49TP/PUn5l+3DzGVCCnsxlVMq89HSeOqzKPnwsWkcOA4Wraib8drIKVF/+CoZY0ccOoJ+AZ/Q== X-Received: by 10.28.170.204 with SMTP id t195mr2279161wme.82.1521816802926; Fri, 23 Mar 2018 07:53:22 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id k1sm11553066wrf.66.2018.03.23.07.53.21 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 23 Mar 2018 07:53:21 -0700 (PDT) Date: Fri, 23 Mar 2018 15:53:07 +0100 From: Adrien Mazarguil To: Ophir Munk Cc: dev@dpdk.org, Thomas Monjalon , Olga Shern , Shahaf shuler Message-ID: <20180323145307.GA3994@6wind.com> References: <1521450055-11523-1-git-send-email-ophirmu@mellanox.com> <1521477410-8936-1-git-send-email-ophirmu@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1521477410-8936-1-git-send-email-ophirmu@mellanox.com> Subject: Re: [dpdk-dev] [PATCH v2] net/mlx4: support CRC strip toggling 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: Fri, 23 Mar 2018 14:53:23 -0000 Hi Ophir, On Mon, Mar 19, 2018 at 04:36:50PM +0000, Ophir Munk wrote: > Previous to this commit mlx4 CRC stripping was executed by default and > there was no verbs API to disable it. > > Current support in MLNX_OFED_4.3-1.5.0.0 and above I suggest to drop this line as it's not exclusive to MLNX_OFED; the documented minimum requirements are already correct and rdma-core v15 also supports it. > Signed-off-by: Ophir Munk A few more comments below. > --- > v1: initial version > v2: following internal reviews > > drivers/net/mlx4/mlx4.c | 4 ++++ > drivers/net/mlx4/mlx4.h | 1 + > drivers/net/mlx4/mlx4_rxq.c | 33 +++++++++++++++++++++++++++++++-- > drivers/net/mlx4/mlx4_rxtx.c | 5 ++++- > drivers/net/mlx4/mlx4_rxtx.h | 1 + > 5 files changed, 41 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c > index ee93daf..d4f4323 100644 > --- a/drivers/net/mlx4/mlx4.c > +++ b/drivers/net/mlx4/mlx4.c > @@ -578,6 +578,10 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev) > } > DEBUG("supported RSS hash fields mask: %016" PRIx64, > priv->hw_rss_sup); > + priv->hw_fcs_strip = !!(device_attr_ex.raw_packet_caps & > + IBV_RAW_PACKET_CAP_SCATTER_FCS); Minor nit, indentation contains one extra space. > + DEBUG("FCS stripping toggling is %ssupported", > + (priv->hw_fcs_strip ? "" : "not ")); Another minor nit, mlx5 prints "configuration" instead of "toggling", wouldn't it make sense for both PMDs to print the same information? Also the extra set of parentheses around the conditional expression could be removed. > /* 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 19c8a22..3ae3ce6 100644 > --- a/drivers/net/mlx4/mlx4.h > +++ b/drivers/net/mlx4/mlx4.h > @@ -105,6 +105,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_fcs_strip:1; /**< FCS stripping toggling is supported. */ > uint64_t hw_rss_sup; /**< Supported RSS hash fields (Verbs format). */ > struct rte_intr_handle intr_handle; /**< Port interrupt handle. */ > struct mlx4_drop *drop; /**< Shared resources for drop flow rules. */ > diff --git a/drivers/net/mlx4/mlx4_rxq.c b/drivers/net/mlx4/mlx4_rxq.c > index 7a036ed..6748355 100644 > --- a/drivers/net/mlx4/mlx4_rxq.c > +++ b/drivers/net/mlx4/mlx4_rxq.c > @@ -491,6 +491,8 @@ mlx4_rxq_attach(struct rxq *rxq) > const char *msg; > struct ibv_cq *cq = NULL; > struct ibv_wq *wq = NULL; > + unsigned int create_flags = 0; > + unsigned int comp_mask = 0; I suggest using uint32_t to align these with Verb's definitions for struct ibv_wq_init_attr. > volatile struct mlx4_wqe_data_seg (*wqes)[]; > unsigned int i; > int ret; > @@ -503,6 +505,11 @@ mlx4_rxq_attach(struct rxq *rxq) > msg = "CQ creation failure"; > goto error; > } > + /* By default, FCS (CRC) is stripped by hardware. */ > + if (rxq->crc_present) { > + create_flags |= IBV_WQ_FLAGS_SCATTER_FCS; > + comp_mask |= IBV_WQ_INIT_ATTR_FLAGS; > + } > wq = mlx4_glue->create_wq > (priv->ctx, > &(struct ibv_wq_init_attr){ > @@ -511,6 +518,8 @@ mlx4_rxq_attach(struct rxq *rxq) > .max_sge = sges_n, > .pd = priv->pd, > .cq = cq, > + .comp_mask = comp_mask, > + .create_flags = create_flags, > }); > if (!wq) { > ret = errno ? errno : EINVAL; > @@ -649,9 +658,10 @@ mlx4_rxq_detach(struct rxq *rxq) > uint64_t > mlx4_get_rx_queue_offloads(struct priv *priv) > { > - uint64_t offloads = DEV_RX_OFFLOAD_SCATTER | > - DEV_RX_OFFLOAD_CRC_STRIP; > + uint64_t offloads = DEV_RX_OFFLOAD_SCATTER; > > + if (priv->hw_fcs_strip) > + offloads |= DEV_RX_OFFLOAD_CRC_STRIP; > if (priv->hw_csum) > offloads |= DEV_RX_OFFLOAD_CHECKSUM; > return offloads; > @@ -781,6 +791,24 @@ mlx4_rx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc, > (void *)dev, idx); > return -rte_errno; > } > + /* By default, FCS (CRC) is stripped by hardware. */ > + unsigned char crc_present; This variable must be grouped with others at the beginning of the block and use the same type as its counterpart in struct rxq for consistency, uint32_t. > + if (conf->offloads & DEV_RX_OFFLOAD_CRC_STRIP) { > + crc_present = 0; > + } else if (priv->hw_fcs_strip) { > + crc_present = 1; > + } else { > + WARN("%p: CRC stripping has been disabled but will still" > + " be performed by hardware, make sure MLNX_OFED and" > + " firmware are up to date", > + (void *)dev); > + crc_present = 0; > + } > + DEBUG("%p: CRC stripping is %s, %u bytes will be subtracted from" > + " incoming frames to hide it", > + (void *)dev, > + crc_present ? "disabled" : "enabled", > + crc_present << 2); The above block must appear prior to the mlx4_zmallocv_socket() call where other configuration checks are performed. > *rxq = (struct rxq){ > .priv = priv, > .mp = mp, > @@ -794,6 +822,7 @@ mlx4_rx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc, > .csum_l2tun = priv->hw_csum_l2tun && > (conf->offloads & DEV_RX_OFFLOAD_CHECKSUM), > .l2tun_offload = priv->hw_csum_l2tun, > + .crc_present = crc_present, One more nit: this line should appear before the l2tun_offload assignment to match the order of definitions in struct rxq. > .stats = { > .idx = idx, > }, > diff --git a/drivers/net/mlx4/mlx4_rxtx.c b/drivers/net/mlx4/mlx4_rxtx.c > index 8ca8b77..84a7bf1 100644 > --- a/drivers/net/mlx4/mlx4_rxtx.c > +++ b/drivers/net/mlx4/mlx4_rxtx.c > @@ -934,12 +934,12 @@ mlx4_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n) > goto skip; > } > pkt = seg; > + assert(len >= (rxq->crc_present << 2)); > /* Update packet information. */ > pkt->packet_type = > rxq_cq_to_pkt_type(cqe, rxq->l2tun_offload); > pkt->ol_flags = PKT_RX_RSS_HASH; > pkt->hash.rss = cqe->immed_rss_invalid; > - pkt->pkt_len = len; > if (rxq->csum | rxq->csum_l2tun) { > uint32_t flags = > mlx4_cqe_flags(cqe, > @@ -951,6 +951,9 @@ mlx4_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n) > rxq->csum, > rxq->csum_l2tun); > } > + if (rxq->crc_present) > + len -= ETHER_CRC_LEN; > + pkt->pkt_len = len; I suggest to move this hunk above, where the pkt->pkt_len assignment was previously made for a shorter diff. > } > rep->nb_segs = 1; > rep->port = rxq->port_id; > diff --git a/drivers/net/mlx4/mlx4_rxtx.h b/drivers/net/mlx4/mlx4_rxtx.h > index c12bd39..a0633bf 100644 > --- a/drivers/net/mlx4/mlx4_rxtx.h > +++ b/drivers/net/mlx4/mlx4_rxtx.h > @@ -52,6 +52,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 crc_present:1; /**< CRC must be subtracted. */ > 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. */ > -- > 2.7.4 > -- Adrien Mazarguil 6WIND