[dpdk-dev,v3] net/mlx4: support CRC strip toggling
Checks
Commit Message
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(-)
Comments
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.
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
<...>
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
>
> <...>
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.
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.
>
>
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/
>
>>
>>
>
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/
@@ -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?"
@@ -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. */
@@ -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,
@@ -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 =
@@ -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. */