* [dpdk-dev] [PATCH v1] net/mlx4: disable CRC stripping @ 2018-03-19 9:00 Ophir Munk 2018-03-19 16:36 ` [dpdk-dev] [PATCH v2] net/mlx4: support CRC strip toggling Ophir Munk 0 siblings, 1 reply; 12+ messages in thread From: Ophir Munk @ 2018-03-19 9:00 UTC (permalink / raw) To: dev, Adrien Mazarguil; +Cc: Thomas Monjalon, Olga Shern, Ophir Munk Previous to this commit mlx4 CRC stripping was executed by default and there was no verbs API to disable it. Since OFED version 4.3-1.5.0.0 the API query_device_ex() indicates if CRC stripping capability is supported in HW and if so CRC stripping can be disabled during WQ initialization. This commit uses these new APIs to allow disabling CRC stripping through rte configuration. A user can specify --disable-crc-stipping in testpmd command line or 'port config all crc-strip off' in testpmd interactive shell. Signed-off-by: Ophir Munk <ophirmu@mellanox.com> --- drivers/net/mlx4/mlx4.c | 4 ++++ drivers/net/mlx4/mlx4.h | 1 + drivers/net/mlx4/mlx4_rxq.c | 33 +++++++++++++++++++++++++++++++-- drivers/net/mlx4/mlx4_rxtx.h | 1 + 4 files changed, 37 insertions(+), 2 deletions(-) diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c index ee93daf..95e01a6 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); + DEBUG("FCS stripping configuration is %ssupported", + (priv->hw_fcs_strip ? "" : "not ")); /* 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..7de896a 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 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; 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; + 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); *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, .stats = { .idx = idx, }, 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [dpdk-dev] [PATCH v2] net/mlx4: support CRC strip toggling 2018-03-19 9:00 [dpdk-dev] [PATCH v1] net/mlx4: disable CRC stripping Ophir Munk @ 2018-03-19 16:36 ` Ophir Munk 2018-03-23 14:53 ` Adrien Mazarguil 2018-03-25 20:19 ` [dpdk-dev] [PATCH v3] " Ophir Munk 0 siblings, 2 replies; 12+ messages in thread From: Ophir Munk @ 2018-03-19 16:36 UTC (permalink / raw) To: dev, Adrien Mazarguil Cc: Thomas Monjalon, Olga Shern, Ophir Munk, Shahaf shuler 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 Signed-off-by: Ophir Munk <ophirmu@mellanox.com> --- 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); + DEBUG("FCS stripping toggling is %ssupported", + (priv->hw_fcs_strip ? "" : "not ")); /* 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; 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; + 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); *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, .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; } 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/mlx4: support CRC strip toggling 2018-03-19 16:36 ` [dpdk-dev] [PATCH v2] net/mlx4: support CRC strip toggling Ophir Munk @ 2018-03-23 14:53 ` Adrien Mazarguil 2018-03-25 16:17 ` Ophir Munk 2018-03-25 20:19 ` [dpdk-dev] [PATCH v3] " Ophir Munk 1 sibling, 1 reply; 12+ messages in thread From: Adrien Mazarguil @ 2018-03-23 14:53 UTC (permalink / raw) To: Ophir Munk; +Cc: dev, Thomas Monjalon, Olga Shern, Shahaf shuler 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 <ophirmu@mellanox.com> 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/mlx4: support CRC strip toggling 2018-03-23 14:53 ` Adrien Mazarguil @ 2018-03-25 16:17 ` Ophir Munk 0 siblings, 0 replies; 12+ messages in thread From: Ophir Munk @ 2018-03-25 16:17 UTC (permalink / raw) To: Adrien Mazarguil; +Cc: dev, Thomas Monjalon, Olga Shern, Shahaf Shuler Hi Adrien, v3 is ready and will be sent soon. Please see inline more comments. > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > Sent: Friday, March 23, 2018 5:53 PM > To: Ophir Munk <ophirmu@mellanox.com> > Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Olga Shern > <olgas@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com> > Subject: Re: [PATCH v2] net/mlx4: support CRC strip toggling > > 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. > Actually rdma-core v15 does not expose CRC capability in mlx4. It is probably expected in rdma-core v18. The OFED support for CRC stripping only starts with in MLNX_OFED_4.3-1.5.0.0. Regardless - for v3 I have dropped the versioning information (as requested) since I think versioning information can be present in documentation rather than in commit logs. > > Signed-off-by: Ophir Munk <ophirmu@mellanox.com> > > 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. The full block is already with an extra space. So removing the extra space in just this line will appear miss-aligned with the remaining block. I suggest leaving this line as is for v3. > > > + 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? > I have followed your review and changed "toggling" to "configuration" in v3. However please note that the commit title is "support CRC strip toggling". So using the same wording "toggling" is clearer in my view. (it was also an input I got from internal review prior to ML submission). > Also the extra set of parentheses around the conditional expression could be > removed. > Done for v3. Please note that another line had an extra set of parentheses (not included in this patch) which was updated as well in v3. > > /* 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. > Done in v3 > > 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. > Done in v3 > > + 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. Done in v3 > > > *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. > Done in v3 > > .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. Done in v3 > > > } > > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [dpdk-dev] [PATCH v3] net/mlx4: support CRC strip toggling 2018-03-19 16:36 ` [dpdk-dev] [PATCH v2] net/mlx4: support CRC strip toggling Ophir Munk 2018-03-23 14:53 ` Adrien Mazarguil @ 2018-03-25 20:19 ` Ophir Munk 2018-03-26 9:34 ` Adrien Mazarguil 2018-03-26 11:38 ` Ferruh Yigit 1 sibling, 2 replies; 12+ messages in thread From: Ophir Munk @ 2018-03-25 20:19 UTC (permalink / raw) To: dev, Adrien Mazarguil Cc: Thomas Monjalon, Olga Shern, Ophir Munk, Shahaf shuler Previous to this commit mlx4 CRC stripping was executed by default and there was no verbs API to disable it. Signed-off-by: Ophir Munk <ophirmu@mellanox.com> --- v1: initial version v2: following internal reviews v3: following dpdk.org mailing list reviews drivers/net/mlx4/mlx4.c | 6 +++++- drivers/net/mlx4/mlx4.h | 1 + drivers/net/mlx4/mlx4_rxq.c | 33 +++++++++++++++++++++++++++++++-- drivers/net/mlx4/mlx4_rxtx.c | 3 +++ 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..eea6e93 100644 --- a/drivers/net/mlx4/mlx4.c +++ b/drivers/net/mlx4/mlx4.c @@ -562,7 +562,7 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev) (device_attr.vendor_part_id == PCI_DEVICE_ID_MELLANOX_CONNECTX3PRO); DEBUG("L2 tunnel checksum offloads are %ssupported", - (priv->hw_csum_l2tun ? "" : "not ")); + priv->hw_csum_l2tun ? "" : "not "); priv->hw_rss_sup = device_attr_ex.rss_caps.rx_hash_fields_mask; if (!priv->hw_rss_sup) { WARN("no RSS capabilities reported; disabling support" @@ -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); + DEBUG("FCS stripping toggling is %ssupported", + priv->hw_fcs_strip ? "" : "not "); /* 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..b437bb8 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; + uint32_t create_flags = 0; + uint32_t comp_mask = 0; 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; @@ -736,6 +746,7 @@ mlx4_rx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc, }, }; int ret; + uint32_t crc_present; (void)conf; /* Thresholds configuration (ignored). */ DEBUG("%p: configuring queue %u for %u descriptors", @@ -774,6 +785,23 @@ mlx4_rx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc, " to the next power of two (%u)", (void *)dev, idx, desc); } + /* By default, FCS (CRC) is stripped by hardware. */ + 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); /* Allocate and initialize Rx queue. */ mlx4_zmallocv_socket("RXQ", vec, RTE_DIM(vec), socket); if (!rxq) { @@ -793,6 +821,7 @@ mlx4_rx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc, (conf->offloads & DEV_RX_OFFLOAD_CHECKSUM), .csum_l2tun = priv->hw_csum_l2tun && (conf->offloads & DEV_RX_OFFLOAD_CHECKSUM), + .crc_present = crc_present, .l2tun_offload = priv->hw_csum_l2tun, .stats = { .idx = idx, diff --git a/drivers/net/mlx4/mlx4_rxtx.c b/drivers/net/mlx4/mlx4_rxtx.c index 8ca8b77..2a10058 100644 --- a/drivers/net/mlx4/mlx4_rxtx.c +++ b/drivers/net/mlx4/mlx4_rxtx.c @@ -934,11 +934,14 @@ 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; + if (rxq->crc_present) + len -= ETHER_CRC_LEN; pkt->pkt_len = len; if (rxq->csum | rxq->csum_l2tun) { uint32_t flags = 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v3] net/mlx4: support CRC strip toggling 2018-03-25 20:19 ` [dpdk-dev] [PATCH v3] " Ophir Munk @ 2018-03-26 9:34 ` Adrien Mazarguil 2018-04-10 6:26 ` Shahaf Shuler 2018-03-26 11:38 ` Ferruh Yigit 1 sibling, 1 reply; 12+ messages in thread From: Adrien Mazarguil @ 2018-03-26 9:34 UTC (permalink / raw) To: Ophir Munk; +Cc: dev, Thomas Monjalon, Olga Shern, Shahaf shuler On Sun, Mar 25, 2018 at 08:19:29PM +0000, Ophir Munk wrote: > Previous to this commit mlx4 CRC stripping was executed by default and > there was no verbs API to disable it. > > Signed-off-by: Ophir Munk <ophirmu@mellanox.com> > --- > v1: initial version > v2: following internal reviews > v3: following dpdk.org mailing list reviews Except for the remaining extra space mentioned below :) Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com> <snip> > diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c > index ee93daf..eea6e93 100644 > --- a/drivers/net/mlx4/mlx4.c > +++ b/drivers/net/mlx4/mlx4.c > @@ -562,7 +562,7 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev) > (device_attr.vendor_part_id == > PCI_DEVICE_ID_MELLANOX_CONNECTX3PRO); > DEBUG("L2 tunnel checksum offloads are %ssupported", > - (priv->hw_csum_l2tun ? "" : "not ")); > + priv->hw_csum_l2tun ? "" : "not "); > priv->hw_rss_sup = device_attr_ex.rss_caps.rx_hash_fields_mask; > if (!priv->hw_rss_sup) { > WARN("no RSS capabilities reported; disabling support" > @@ -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); I know the extra space before IBV_RAW_PACKET_CAP_SCATTER_FCS is present in the original mlx5 code, but it's misaligned there also. This line should be aligned with "device_attr_ex.raw_packet_caps" for consistency. -- Adrien Mazarguil 6WIND ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v3] net/mlx4: support CRC strip toggling 2018-03-26 9:34 ` Adrien Mazarguil @ 2018-04-10 6:26 ` Shahaf Shuler 0 siblings, 0 replies; 12+ messages in thread From: Shahaf Shuler @ 2018-04-10 6:26 UTC (permalink / raw) To: Adrien Mazarguil, Ophir Munk; +Cc: dev, Thomas Monjalon, Olga Shern Monday, March 26, 2018 12:35 PM, Adrien Mazarguil: > Subject: Re: [PATCH v3] net/mlx4: support CRC strip toggling > > On Sun, Mar 25, 2018 at 08:19:29PM +0000, Ophir Munk wrote: > > Previous to this commit mlx4 CRC stripping was executed by default and > > there was no verbs API to disable it. > > > > Signed-off-by: Ophir Munk <ophirmu@mellanox.com> > > --- > > v1: initial version > > v2: following internal reviews > > v3: following dpdk.org mailing list reviews > > Except for the remaining extra space mentioned below :) > > Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com> It seems the direction of the KEEP_CRC flag discussion[1] is for the flag to be included only on 18.08. Hence applying this patch (with the extra space removal). In case KEEP_CRC will be included on 18.05 we will have another patch to adjust. [1] https://dpdk.org/dev/patchwork/patch/36416/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v3] net/mlx4: support CRC strip toggling 2018-03-25 20:19 ` [dpdk-dev] [PATCH v3] " Ophir Munk 2018-03-26 9:34 ` Adrien Mazarguil @ 2018-03-26 11:38 ` Ferruh Yigit 2018-03-26 11:54 ` Adrien Mazarguil 1 sibling, 1 reply; 12+ messages in thread From: Ferruh Yigit @ 2018-03-26 11:38 UTC (permalink / raw) To: Ophir Munk, dev, Adrien Mazarguil Cc: Thomas Monjalon, Olga Shern, Shahaf shuler On 3/25/2018 9:19 PM, Ophir Munk wrote: > Previous to this commit mlx4 CRC stripping was executed by default and > there was no verbs API to disable it. Are you aware of the discussion about CRC [1]? Is this patch compatible with plans? [1] https://dpdk.org/dev/patchwork/patch/36415/ > Signed-off-by: Ophir Munk <ophirmu@mellanox.com> > --- > v1: initial version > v2: following internal reviews > v3: following dpdk.org mailing list reviews <...> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v3] net/mlx4: support CRC strip toggling 2018-03-26 11:38 ` Ferruh Yigit @ 2018-03-26 11:54 ` Adrien Mazarguil 2018-03-26 12:30 ` Thomas Monjalon 0 siblings, 1 reply; 12+ messages in thread From: Adrien Mazarguil @ 2018-03-26 11:54 UTC (permalink / raw) To: Ferruh Yigit; +Cc: Ophir Munk, dev, Thomas Monjalon, Olga Shern, Shahaf shuler On Mon, Mar 26, 2018 at 12:38:22PM +0100, Ferruh Yigit wrote: > On 3/25/2018 9:19 PM, Ophir Munk wrote: > > Previous to this commit mlx4 CRC stripping was executed by default and > > there was no verbs API to disable it. > > Are you aware of the discussion about CRC [1]? Is this patch compatible with plans? > > [1] > https://dpdk.org/dev/patchwork/patch/36415/ I wasn't aware of this notice. Looks like it makes this patch unnecessary since mlx4 always strip by default; this patch makes it configurable at will and only exposes the capability when HW supports its configuration (i.e. the ability to leave CRC inside mbuf). We'd just need mlx4 to not expose DEV_RX_OFFLOAD_CRC_STRIP at all in mlx5_get_rx_queue_offloads() in order to fully comply. I leave it up to you, I don't mind if we include this patch only to revert it later when we finally get rid of DEV_RX_OFFLOAD_CRC_STRIP. > > Signed-off-by: Ophir Munk <ophirmu@mellanox.com> > > --- > > v1: initial version > > v2: following internal reviews > > v3: following dpdk.org mailing list reviews > > <...> -- Adrien Mazarguil 6WIND ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v3] net/mlx4: support CRC strip toggling 2018-03-26 11:54 ` Adrien Mazarguil @ 2018-03-26 12:30 ` Thomas Monjalon 2018-03-28 6:03 ` Shahaf Shuler 0 siblings, 1 reply; 12+ messages in thread From: Thomas Monjalon @ 2018-03-26 12:30 UTC (permalink / raw) To: Adrien Mazarguil; +Cc: Ferruh Yigit, Ophir Munk, dev, Olga Shern, Shahaf shuler 26/03/2018 13:54, Adrien Mazarguil: > On Mon, Mar 26, 2018 at 12:38:22PM +0100, Ferruh Yigit wrote: > > On 3/25/2018 9:19 PM, Ophir Munk wrote: > > > Previous to this commit mlx4 CRC stripping was executed by default and > > > there was no verbs API to disable it. > > > > Are you aware of the discussion about CRC [1]? Is this patch compatible with plans? > > > > [1] > > https://dpdk.org/dev/patchwork/patch/36415/ > > I wasn't aware of this notice. Looks like it makes this patch unnecessary > since mlx4 always strip by default; this patch makes it configurable at will > and only exposes the capability when HW supports its configuration (i.e. the > ability to leave CRC inside mbuf). > > We'd just need mlx4 to not expose DEV_RX_OFFLOAD_CRC_STRIP at all in > mlx5_get_rx_queue_offloads() in order to fully comply. > > I leave it up to you, I don't mind if we include this patch only to revert > it later when we finally get rid of DEV_RX_OFFLOAD_CRC_STRIP. A new flag to keep CRC will be introduced. We will need toggling anyway. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v3] net/mlx4: support CRC strip toggling 2018-03-26 12:30 ` Thomas Monjalon @ 2018-03-28 6:03 ` Shahaf Shuler 2018-03-28 14:19 ` Ferruh Yigit 0 siblings, 1 reply; 12+ messages in thread From: Shahaf Shuler @ 2018-03-28 6:03 UTC (permalink / raw) To: Thomas Monjalon, Adrien Mazarguil Cc: Ferruh Yigit, Ophir Munk, dev, Olga Shern Monday, March 26, 2018 3:31 PM, Thomas Monjalon: > 26/03/2018 13:54, Adrien Mazarguil: > > On Mon, Mar 26, 2018 at 12:38:22PM +0100, Ferruh Yigit wrote: > > > On 3/25/2018 9:19 PM, Ophir Munk wrote: > > > > Previous to this commit mlx4 CRC stripping was executed by default > > > > and there was no verbs API to disable it. > > > > > > Are you aware of the discussion about CRC [1]? Is this patch compatible > with plans? > > > > > > [1] > > > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fd > > > > pdk.org%2Fdev%2Fpatchwork%2Fpatch%2F36415%2F&data=02%7C01%7Csh > ahafs% > > > > 40mellanox.com%7C90c78c02ec954cc1dfda08d59315619f%7Ca652971c7d2e4 > d9b > > > > a6a4d149256f461b%7C0%7C0%7C636576642435977344&sdata=broGz4U2Kw1 > u9xJk > > > t%2FryP7j2CU9XN0GnM5jsUxHwiNg%3D&reserved=0 > > > > I wasn't aware of this notice. Looks like it makes this patch > > unnecessary since mlx4 always strip by default; this patch makes it > > configurable at will and only exposes the capability when HW supports > > its configuration (i.e. the ability to leave CRC inside mbuf). > > > > We'd just need mlx4 to not expose DEV_RX_OFFLOAD_CRC_STRIP at all in > > mlx5_get_rx_queue_offloads() in order to fully comply. > > > > I leave it up to you, I don't mind if we include this patch only to > > revert it later when we finally get rid of DEV_RX_OFFLOAD_CRC_STRIP. > > A new flag to keep CRC will be introduced. > We will need toggling anyway. Do we have such CRC flag already? If not I will include this patch and we revert/adjust it when the new flag will be introduced. > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v3] net/mlx4: support CRC strip toggling 2018-03-28 6:03 ` Shahaf Shuler @ 2018-03-28 14:19 ` Ferruh Yigit 0 siblings, 0 replies; 12+ messages in thread From: Ferruh Yigit @ 2018-03-28 14:19 UTC (permalink / raw) To: Shahaf Shuler, Thomas Monjalon, Adrien Mazarguil Cc: Ophir Munk, dev, Olga Shern On 3/28/2018 7:03 AM, Shahaf Shuler wrote: > Monday, March 26, 2018 3:31 PM, Thomas Monjalon: >> 26/03/2018 13:54, Adrien Mazarguil: >>> On Mon, Mar 26, 2018 at 12:38:22PM +0100, Ferruh Yigit wrote: >>>> On 3/25/2018 9:19 PM, Ophir Munk wrote: >>>>> Previous to this commit mlx4 CRC stripping was executed by default >>>>> and there was no verbs API to disable it. >>>> >>>> Are you aware of the discussion about CRC [1]? Is this patch compatible >> with plans? >>>> >>>> [1] >>>> >> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fd >>>> >> pdk.org%2Fdev%2Fpatchwork%2Fpatch%2F36415%2F&data=02%7C01%7Csh >> ahafs% >>>> >> 40mellanox.com%7C90c78c02ec954cc1dfda08d59315619f%7Ca652971c7d2e4 >> d9b >>>> >> a6a4d149256f461b%7C0%7C0%7C636576642435977344&sdata=broGz4U2Kw1 >> u9xJk >>>> t%2FryP7j2CU9XN0GnM5jsUxHwiNg%3D&reserved=0 >>> >>> I wasn't aware of this notice. Looks like it makes this patch >>> unnecessary since mlx4 always strip by default; this patch makes it >>> configurable at will and only exposes the capability when HW supports >>> its configuration (i.e. the ability to leave CRC inside mbuf). >>> >>> We'd just need mlx4 to not expose DEV_RX_OFFLOAD_CRC_STRIP at all in >>> mlx5_get_rx_queue_offloads() in order to fully comply. >>> >>> I leave it up to you, I don't mind if we include this patch only to >>> revert it later when we finally get rid of DEV_RX_OFFLOAD_CRC_STRIP. >> >> A new flag to keep CRC will be introduced. >> We will need toggling anyway. > > Do we have such CRC flag already? If not I will include this patch and we revert/adjust it when the new flag will be introduced. Patch sent recently [1] but not merged yet, your review may help there :) [1] https://dpdk.org/dev/patchwork/patch/36416/ > >> >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-04-10 6:26 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-19 9:00 [dpdk-dev] [PATCH v1] net/mlx4: disable CRC stripping Ophir Munk 2018-03-19 16:36 ` [dpdk-dev] [PATCH v2] net/mlx4: support CRC strip toggling Ophir Munk 2018-03-23 14:53 ` Adrien Mazarguil 2018-03-25 16:17 ` Ophir Munk 2018-03-25 20:19 ` [dpdk-dev] [PATCH v3] " Ophir Munk 2018-03-26 9:34 ` Adrien Mazarguil 2018-04-10 6:26 ` Shahaf Shuler 2018-03-26 11:38 ` Ferruh Yigit 2018-03-26 11:54 ` Adrien Mazarguil 2018-03-26 12:30 ` Thomas Monjalon 2018-03-28 6:03 ` Shahaf Shuler 2018-03-28 14:19 ` Ferruh Yigit
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).