[dpdk-dev] [PATCH] net/mlx5: fix Tx doorbell

[dpdk-dev] [PATCH] net/mlx5: fix Tx doorbell

[Nelio Laranjeiro]

Too much data is uselessly written to the Tx doorbell.

Fixes: 1d88ba171942 ("net/mlx5: refactor Tx data path")

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

---

This patch should be applied on top of [1] or [2].

[1] http://dpdk.org/ml/archives/dev/2016-November/050716.html
[2] http://dpdk.org/dev/patchwork/patch/17254/
---
 drivers/net/mlx5/mlx5_rxtx.c | 26 ++++++++++++--------------
 drivers/net/mlx5/mlx5_rxtx.h |  2 --
 drivers/net/mlx5/mlx5_txq.c  |  2 --
 3 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index 22b22ac..029d872 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -82,7 +82,8 @@ txq_mp2mr(struct txq *txq, struct rte_mempool *mp)
 	__attribute__((always_inline));
 
 static inline void
-mlx5_tx_dbrec(struct txq *txq) __attribute__((always_inline));
+mlx5_tx_dbrec(struct txq *txq, volatile struct mlx5_wqe *wqe)
+	__attribute__((always_inline));
 
 static inline uint32_t
 rxq_cq_to_pkt_type(volatile struct mlx5_cqe *cqe)
@@ -326,23 +327,20 @@ txq_mp2mr(struct txq *txq, struct rte_mempool *mp)
  *
  * @param txq
  *   Pointer to TX queue structure.
+ * @param wqe
+ *   Pointer to the last WQE posted in the NIC.
  */
 static inline void
-mlx5_tx_dbrec(struct txq *txq)
+mlx5_tx_dbrec(struct txq *txq, volatile struct mlx5_wqe *wqe)
 {
-	uint8_t *dst = (uint8_t *)((uintptr_t)txq->bf_reg + txq->bf_offset);
-	uint32_t data[4] = {
-		htonl((txq->wqe_ci << 8) | MLX5_OPCODE_SEND),
-		htonl(txq->qp_num_8s),
-		0,
-		0,
-	};
+	uint64_t *dst = (uint64_t *)((uintptr_t)txq->bf_reg);
+	volatile uint64_t *src = ((volatile uint64_t *)wqe);
+
 	rte_wmb();
 	*txq->qp_db = htonl(txq->wqe_ci);
 	/* Ensure ordering between DB record and BF copy. */
 	rte_wmb();
-	memcpy(dst, (uint8_t *)data, 16);
-	txq->bf_offset ^= (1 << txq->bf_buf_size);
+	*dst = *src;
 }
 
 /**
@@ -609,7 +607,7 @@ mlx5_tx_burst(void *dpdk_txq, struct rte_mbuf **pkts, uint16_t pkts_n)
 	txq->stats.opackets += i;
 #endif
 	/* Ring QP doorbell. */
-	mlx5_tx_dbrec(txq);
+	mlx5_tx_dbrec(txq, (volatile struct mlx5_wqe *)wqe);
 	txq->elts_head = elts_head;
 	return i;
 }
@@ -816,7 +814,7 @@ mlx5_tx_burst_mpw(void *dpdk_txq, struct rte_mbuf **pkts, uint16_t pkts_n)
 	/* Ring QP doorbell. */
 	if (mpw.state == MLX5_MPW_STATE_OPENED)
 		mlx5_mpw_close(txq, &mpw);
-	mlx5_tx_dbrec(txq);
+	mlx5_tx_dbrec(txq, mpw.wqe);
 	txq->elts_head = elts_head;
 	return i;
 }
@@ -1084,7 +1082,7 @@ mlx5_tx_burst_mpw_inline(void *dpdk_txq, struct rte_mbuf **pkts,
 		mlx5_mpw_inline_close(txq, &mpw);
 	else if (mpw.state == MLX5_MPW_STATE_OPENED)
 		mlx5_mpw_close(txq, &mpw);
-	mlx5_tx_dbrec(txq);
+	mlx5_tx_dbrec(txq, mpw.wqe);
 	txq->elts_head = elts_head;
 	return i;
 }
diff --git a/drivers/net/mlx5/mlx5_rxtx.h b/drivers/net/mlx5/mlx5_rxtx.h
index 6dcd35d..e244c48 100644
--- a/drivers/net/mlx5/mlx5_rxtx.h
+++ b/drivers/net/mlx5/mlx5_rxtx.h
@@ -251,8 +251,6 @@ struct txq {
 	uint16_t elts_n:4; /* (*elts)[] length (in log2). */
 	uint16_t cqe_n:4; /* Number of CQ elements (in log2). */
 	uint16_t wqe_n:4; /* Number of of WQ elements (in log2). */
-	uint16_t bf_buf_size:4; /* Log2 Blueflame size. */
-	uint16_t bf_offset; /* Blueflame offset. */
 	uint16_t max_inline; /* Multiple of RTE_CACHE_LINE_SIZE to inline. */
 	uint32_t qp_num_8s; /* QP number shifted by 8. */
 	volatile struct mlx5_cqe (*cqes)[]; /* Completion queue. */
diff --git a/drivers/net/mlx5/mlx5_txq.c b/drivers/net/mlx5/mlx5_txq.c
index f4c6682..ad39ddc 100644
--- a/drivers/net/mlx5/mlx5_txq.c
+++ b/drivers/net/mlx5/mlx5_txq.c
@@ -220,8 +220,6 @@ txq_setup(struct txq_ctrl *tmpl, struct txq_ctrl *txq_ctrl)
 	tmpl->txq.wqe_n = log2above(qp->sq.wqe_cnt);
 	tmpl->txq.qp_db = &qp->gen_data.db[MLX5_SND_DBR];
 	tmpl->txq.bf_reg = qp->gen_data.bf->reg;
-	tmpl->txq.bf_offset = qp->gen_data.bf->offset;
-	tmpl->txq.bf_buf_size = log2above(qp->gen_data.bf->buf_size);
 	tmpl->txq.cq_db = cq->dbrec;
 	tmpl->txq.cqes =
 		(volatile struct mlx5_cqe (*)[])
-- 
2.1.4

Re: [dpdk-dev] [PATCH] net/mlx5: fix Tx doorbell

[Ferruh Yigit]

On 12/9/2016 1:27 PM, Nelio Laranjeiro wrote:
> Too much data is uselessly written to the Tx doorbell.
> 
> Fixes: 1d88ba171942 ("net/mlx5: refactor Tx data path")
> 
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> 

Applied to dpdk-next-net/master, thanks.

Is not CC'ing stable intentional, since this patch depends on a patch
introduced in this release? If not intentional, please CC stable.

Re: [dpdk-dev] [PATCH] net/mlx5: fix Tx doorbell

[Nélio Laranjeiro]

On Thu, Jan 05, 2017 at 03:19:35PM +0000, Ferruh Yigit wrote:
> On 12/9/2016 1:27 PM, Nelio Laranjeiro wrote:
> > Too much data is uselessly written to the Tx doorbell.
> > 
> > Fixes: 1d88ba171942 ("net/mlx5: refactor Tx data path")
> > 
> > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> > Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > 
> 
> Applied to dpdk-next-net/master, thanks.
> 
> Is not CC'ing stable intentional, since this patch depends on a patch
> introduced in this release? If not intentional, please CC stable.

Hi Ferruh,

It was intentional, this commit won't apply as is on stable branch.

Thanks for the remind,

-- 
Nélio Laranjeiro
6WIND

Re: [dpdk-dev] [PATCH] net/mlx5: fix Tx doorbell

[Adrien Mazarguil]

Hi Ferruh,

On Thu, Jan 05, 2017 at 03:19:35PM +0000, Ferruh Yigit wrote:
> On 12/9/2016 1:27 PM, Nelio Laranjeiro wrote:
> > Too much data is uselessly written to the Tx doorbell.
> > 
> > Fixes: 1d88ba171942 ("net/mlx5: refactor Tx data path")
> > 
> > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> > Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > 
> 
> Applied to dpdk-next-net/master, thanks.
> 
> Is not CC'ing stable intentional, since this patch depends on a patch
> introduced in this release? If not intentional, please CC stable.

I intended to update the commit message for this patch as in the meantime we
discovered it addresses a significant regression introduced in v16.11.

CC'ing stable now.

If possible, can you amend the commit log with:

---

net/mlx5: fix Tx doorbell

Too much data is uselessly written to the Tx doorbell, which since v16.11
may also cause Tx queues to behave erratically and crash applications.

This regression was seen on VF devices when the BlueFlame buffer size is
zero (txq->cqe_n == 0) due to the following change:

 -       cqe = &(*txq->cqes)[ci & (txq->cqe_n - 1)].cqe64;
 +       cqe = &(*txq->cqes)[ci & ((1 << txq->cqe_n) - 1)];

Fixes: 1d88ba171942 ("net/mlx5: refactor Tx data path")
Fixes: e2f116ee3cac ("net/mlx5: reduce memory overhead for CQE handling")

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Cc: stable@dpdk.org

---

Thanks,

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH] net/mlx5: fix Tx doorbell

[Adrien Mazarguil]

On Thu, Jan 05, 2017 at 05:32:26PM +0100, Adrien Mazarguil wrote:
> Hi Ferruh,
> 
> On Thu, Jan 05, 2017 at 03:19:35PM +0000, Ferruh Yigit wrote:
> > On 12/9/2016 1:27 PM, Nelio Laranjeiro wrote:
> > > Too much data is uselessly written to the Tx doorbell.
> > > 
> > > Fixes: 1d88ba171942 ("net/mlx5: refactor Tx data path")
> > > 
> > > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> > > Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > 
> > 
> > Applied to dpdk-next-net/master, thanks.
> > 
> > Is not CC'ing stable intentional, since this patch depends on a patch
> > introduced in this release? If not intentional, please CC stable.
> 
> I intended to update the commit message for this patch as in the meantime we
> discovered it addresses a significant regression introduced in v16.11.
> 
> CC'ing stable now.
> 
> If possible, can you amend the commit log with:
> 
> ---
> 
> net/mlx5: fix Tx doorbell
> 
> Too much data is uselessly written to the Tx doorbell, which since v16.11
> may also cause Tx queues to behave erratically and crash applications.
> 
> This regression was seen on VF devices when the BlueFlame buffer size is
> zero (txq->cqe_n == 0) due to the following change:
> 
>  -       cqe = &(*txq->cqes)[ci & (txq->cqe_n - 1)].cqe64;
>  +       cqe = &(*txq->cqes)[ci & ((1 << txq->cqe_n) - 1)];
> 
> Fixes: 1d88ba171942 ("net/mlx5: refactor Tx data path")
> Fixes: e2f116ee3cac ("net/mlx5: reduce memory overhead for CQE handling")
> 
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Cc: stable@dpdk.org
> 
> ---

I mixed the commit that introduced the regression with a similar looking yet
harmless one, here is the proper message to use, sorry for the noise:

---

net/mlx5: fix Tx doorbell

Too much data is uselessly written to the Tx doorbell, which since v16.11
may also cause Tx queues to behave erratically and crash applications.

This regression was seen on VF devices when the BlueFlame buffer size is
zero (txq->bf_buf_size) due to the following change:

 -       txq->bf_offset ^= txq->bf_buf_size;
 +       txq->bf_offset ^= (1 << txq->bf_buf_size);

Fixes: 1d88ba171942 ("net/mlx5: refactor Tx data path")
Fixes: d5793daefec8 ("net/mlx5: reduce memory overhead for BF handling")

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Cc: stable@dpdk.org

---

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH] net/mlx5: fix Tx doorbell

[Ferruh Yigit]

On 1/5/2017 4:52 PM, Adrien Mazarguil wrote:
> On Thu, Jan 05, 2017 at 05:32:26PM +0100, Adrien Mazarguil wrote:
>> Hi Ferruh,
>>
>> On Thu, Jan 05, 2017 at 03:19:35PM +0000, Ferruh Yigit wrote:
>>> On 12/9/2016 1:27 PM, Nelio Laranjeiro wrote:
>>>> Too much data is uselessly written to the Tx doorbell.
>>>>
>>>> Fixes: 1d88ba171942 ("net/mlx5: refactor Tx data path")
>>>>
>>>> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
>>>> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>>>>
>>>
>>> Applied to dpdk-next-net/master, thanks.
>>>
>>> Is not CC'ing stable intentional, since this patch depends on a patch
>>> introduced in this release? If not intentional, please CC stable.
>>
>> I intended to update the commit message for this patch as in the meantime we
>> discovered it addresses a significant regression introduced in v16.11.
>>
>> CC'ing stable now.
>>
>> If possible, can you amend the commit log with:
>>
>> ---
>>
>> net/mlx5: fix Tx doorbell
>>
>> Too much data is uselessly written to the Tx doorbell, which since v16.11
>> may also cause Tx queues to behave erratically and crash applications.
>>
>> This regression was seen on VF devices when the BlueFlame buffer size is
>> zero (txq->cqe_n == 0) due to the following change:
>>
>>  -       cqe = &(*txq->cqes)[ci & (txq->cqe_n - 1)].cqe64;
>>  +       cqe = &(*txq->cqes)[ci & ((1 << txq->cqe_n) - 1)];
>>
>> Fixes: 1d88ba171942 ("net/mlx5: refactor Tx data path")
>> Fixes: e2f116ee3cac ("net/mlx5: reduce memory overhead for CQE handling")
>>
>> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
>> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>> Cc: stable@dpdk.org
>>
>> ---
> 
> I mixed the commit that introduced the regression with a similar looking yet
> harmless one, here is the proper message to use, sorry for the noise:
> 
> ---
> 
> net/mlx5: fix Tx doorbell
> 
> Too much data is uselessly written to the Tx doorbell, which since v16.11
> may also cause Tx queues to behave erratically and crash applications.
> 
> This regression was seen on VF devices when the BlueFlame buffer size is
> zero (txq->bf_buf_size) due to the following change:
> 
>  -       txq->bf_offset ^= txq->bf_buf_size;
>  +       txq->bf_offset ^= (1 << txq->bf_buf_size);
> 
> Fixes: 1d88ba171942 ("net/mlx5: refactor Tx data path")
> Fixes: d5793daefec8 ("net/mlx5: reduce memory overhead for BF handling")
> 
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Cc: stable@dpdk.org
> 
> ---
> 

Can you please confirm commit in latest next-net?

Re: [dpdk-dev] [PATCH] net/mlx5: fix Tx doorbell

[Adrien Mazarguil]

On Thu, Jan 05, 2017 at 05:01:27PM +0000, Ferruh Yigit wrote:
> On 1/5/2017 4:52 PM, Adrien Mazarguil wrote:
> > On Thu, Jan 05, 2017 at 05:32:26PM +0100, Adrien Mazarguil wrote:
> >> Hi Ferruh,
> >>
> >> On Thu, Jan 05, 2017 at 03:19:35PM +0000, Ferruh Yigit wrote:
> >>> On 12/9/2016 1:27 PM, Nelio Laranjeiro wrote:
> >>>> Too much data is uselessly written to the Tx doorbell.
> >>>>
> >>>> Fixes: 1d88ba171942 ("net/mlx5: refactor Tx data path")
> >>>>
> >>>> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> >>>> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> >>>>
> >>>
> >>> Applied to dpdk-next-net/master, thanks.
> >>>
> >>> Is not CC'ing stable intentional, since this patch depends on a patch
> >>> introduced in this release? If not intentional, please CC stable.
> >>
> >> I intended to update the commit message for this patch as in the meantime we
> >> discovered it addresses a significant regression introduced in v16.11.
> >>
> >> CC'ing stable now.
> >>
> >> If possible, can you amend the commit log with:
> >>
> >> ---
> >>
> >> net/mlx5: fix Tx doorbell
> >>
> >> Too much data is uselessly written to the Tx doorbell, which since v16.11
> >> may also cause Tx queues to behave erratically and crash applications.
> >>
> >> This regression was seen on VF devices when the BlueFlame buffer size is
> >> zero (txq->cqe_n == 0) due to the following change:
> >>
> >>  -       cqe = &(*txq->cqes)[ci & (txq->cqe_n - 1)].cqe64;
> >>  +       cqe = &(*txq->cqes)[ci & ((1 << txq->cqe_n) - 1)];
> >>
> >> Fixes: 1d88ba171942 ("net/mlx5: refactor Tx data path")
> >> Fixes: e2f116ee3cac ("net/mlx5: reduce memory overhead for CQE handling")
> >>
> >> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> >> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> >> Cc: stable@dpdk.org
> >>
> >> ---
> > 
> > I mixed the commit that introduced the regression with a similar looking yet
> > harmless one, here is the proper message to use, sorry for the noise:
> > 
> > ---
> > 
> > net/mlx5: fix Tx doorbell
> > 
> > Too much data is uselessly written to the Tx doorbell, which since v16.11
> > may also cause Tx queues to behave erratically and crash applications.
> > 
> > This regression was seen on VF devices when the BlueFlame buffer size is
> > zero (txq->bf_buf_size) due to the following change:
> > 
> >  -       txq->bf_offset ^= txq->bf_buf_size;
> >  +       txq->bf_offset ^= (1 << txq->bf_buf_size);
> > 
> > Fixes: 1d88ba171942 ("net/mlx5: refactor Tx data path")
> > Fixes: d5793daefec8 ("net/mlx5: reduce memory overhead for BF handling")
> > 
> > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> > Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Cc: stable@dpdk.org
> > 
> > ---
> > 
> 
> Can you please confirm commit in latest next-net?

Not sure if the "Cc: stable@dpdk.org" line should have been kept, otherwise
it's perfect, thanks Ferruh!

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH] net/mlx5: fix Tx doorbell

[Ferruh Yigit]

On 1/5/2017 6:12 PM, Adrien Mazarguil wrote:
> On Thu, Jan 05, 2017 at 05:01:27PM +0000, Ferruh Yigit wrote:
>> On 1/5/2017 4:52 PM, Adrien Mazarguil wrote:
>>> On Thu, Jan 05, 2017 at 05:32:26PM +0100, Adrien Mazarguil wrote:
>>>> Hi Ferruh,
>>>>
>>>> On Thu, Jan 05, 2017 at 03:19:35PM +0000, Ferruh Yigit wrote:
>>>>> On 12/9/2016 1:27 PM, Nelio Laranjeiro wrote:
>>>>>> Too much data is uselessly written to the Tx doorbell.
>>>>>>
>>>>>> Fixes: 1d88ba171942 ("net/mlx5: refactor Tx data path")
>>>>>>
>>>>>> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
>>>>>> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>>>>>>
>>>>>
>>>>> Applied to dpdk-next-net/master, thanks.
>>>>>
>>>>> Is not CC'ing stable intentional, since this patch depends on a patch
>>>>> introduced in this release? If not intentional, please CC stable.
>>>>
>>>> I intended to update the commit message for this patch as in the meantime we
>>>> discovered it addresses a significant regression introduced in v16.11.
>>>>
>>>> CC'ing stable now.
>>>>
>>>> If possible, can you amend the commit log with:
>>>>
>>>> ---
>>>>
>>>> net/mlx5: fix Tx doorbell
>>>>
>>>> Too much data is uselessly written to the Tx doorbell, which since v16.11
>>>> may also cause Tx queues to behave erratically and crash applications.
>>>>
>>>> This regression was seen on VF devices when the BlueFlame buffer size is
>>>> zero (txq->cqe_n == 0) due to the following change:
>>>>
>>>>  -       cqe = &(*txq->cqes)[ci & (txq->cqe_n - 1)].cqe64;
>>>>  +       cqe = &(*txq->cqes)[ci & ((1 << txq->cqe_n) - 1)];
>>>>
>>>> Fixes: 1d88ba171942 ("net/mlx5: refactor Tx data path")
>>>> Fixes: e2f116ee3cac ("net/mlx5: reduce memory overhead for CQE handling")
>>>>
>>>> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
>>>> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>>>> Cc: stable@dpdk.org
>>>>
>>>> ---
>>>
>>> I mixed the commit that introduced the regression with a similar looking yet
>>> harmless one, here is the proper message to use, sorry for the noise:
>>>
>>> ---
>>>
>>> net/mlx5: fix Tx doorbell
>>>
>>> Too much data is uselessly written to the Tx doorbell, which since v16.11
>>> may also cause Tx queues to behave erratically and crash applications.
>>>
>>> This regression was seen on VF devices when the BlueFlame buffer size is
>>> zero (txq->bf_buf_size) due to the following change:
>>>
>>>  -       txq->bf_offset ^= txq->bf_buf_size;
>>>  +       txq->bf_offset ^= (1 << txq->bf_buf_size);
>>>
>>> Fixes: 1d88ba171942 ("net/mlx5: refactor Tx data path")
>>> Fixes: d5793daefec8 ("net/mlx5: reduce memory overhead for BF handling")
>>>
>>> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
>>> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>>> Cc: stable@dpdk.org
>>>
>>> ---
>>>
>>
>> Can you please confirm commit in latest next-net?
> 
> Not sure if the "Cc: stable@dpdk.org" line should have been kept, 

No, we are removing it from the commit.
It is useful in the patch since "git send-email" ensures the stable mail
list is added into CC.
But it is not that useful in the git history.

> otherwise it's perfect, thanks Ferruh!

OK, thanks.

>