DPDK-dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] net/mana: fix Tx stall from send queue free-space unit mismatch
@ 2026-09-14 10:58 Rita Ruvinsky
  2026-09-14 16:14 ` Stephen Hemminger
  2026-09-17  6:23 ` [PATCH v2] " Rita Ruvinsky
  0 siblings, 2 replies; 6+ messages in thread
From: Rita Ruvinsky @ 2026-09-14 10:58 UTC (permalink / raw)
  To: dev; +Cc: longli, weh, stable, Rita Ruvinsky

gdma_post_work_request() subtracted a unit count from an entry count:

  queue_free_units = queue->count - (queue->head - queue->tail);

queue->count is in entries, while head and tail are in WQE alignment
units. On a 512-entry, 128KB send queue the check saw 512 units of
capacity instead of queue->size / GDMA_WQE_ALIGNMENT_UNIT_SIZE = 4096,
and returned -EBUSY with the queue one eighth full. A workload that
fills that window faster than it drains makes rte_eth_tx_burst() return
0 for long enough to look like a dead port.

Derive the capacity from queue->size, which is also what the ring wrap
in gdma_get_wqe_pointer() uses. Rx is unaffected: its WQEs occupy
exactly one unit, so entries and units coincide.

Fixes: 56dd45c0ce7b ("net/mana: implement hardware layer operations")
Cc: stable@dpdk.org

Signed-off-by: Rita Ruvinsky <rita.ruvinsky@weka.io>
---
Independent of patchwork 167383 and 167384 (net/mana MR length
truncation / Rx WQE double free) from the same author, currently in
awaiting-upstream: this touches gdma.c only and applies cleanly with or
without them.

 drivers/net/mana/gdma.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mana/gdma.c b/drivers/net/mana/gdma.c
index 7f66a7a7cf..c80fc25f21 100644
--- a/drivers/net/mana/gdma.c
+++ b/drivers/net/mana/gdma.c
@@ -138,7 +138,12 @@ gdma_post_work_request(struct mana_gdma_queue *queue,
 				client_oob_size + sgl_data_size,
 			  GDMA_WQE_ALIGNMENT_UNIT_SIZE);
 	uint8_t *wq_buffer_pointer;
-	uint32_t queue_free_units = queue->count - (queue->head - queue->tail);
+	/* head/tail count WQE alignment units, so the capacity they are compared
+	 * against must too: queue->count is in entries and undercounts the
+	 * queue, stalling Tx well below capacity.
+	 */
+	uint32_t queue_free_units = queue->size / GDMA_WQE_ALIGNMENT_UNIT_SIZE -
+					(queue->head - queue->tail);
 
 	if (wqe_size / GDMA_WQE_ALIGNMENT_UNIT_SIZE > queue_free_units) {
 		DP_LOG(DEBUG, "WQE size %u queue count %u head %u tail %u",
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] net/mana: fix Tx stall from send queue free-space unit mismatch
  2026-09-14 10:58 [PATCH] net/mana: fix Tx stall from send queue free-space unit mismatch Rita Ruvinsky
@ 2026-09-14 16:14 ` Stephen Hemminger
  2026-09-14 16:24   ` [EXTERNAL] " Wei Hu
  2026-09-17  6:23 ` [PATCH v2] " Rita Ruvinsky
  1 sibling, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2026-09-14 16:14 UTC (permalink / raw)
  To: Rita Ruvinsky; +Cc: dev, longli, weh, stable

On Mon, 14 Sep 2026 13:58:08 +0300
Rita Ruvinsky <rita.ruvinsky@weka.io> wrote:

> gdma_post_work_request() subtracted a unit count from an entry count:
> 
>   queue_free_units = queue->count - (queue->head - queue->tail);
> 
> queue->count is in entries, while head and tail are in WQE alignment
> units. On a 512-entry, 128KB send queue the check saw 512 units of
> capacity instead of queue->size / GDMA_WQE_ALIGNMENT_UNIT_SIZE = 4096,
> and returned -EBUSY with the queue one eighth full. A workload that
> fills that window faster than it drains makes rte_eth_tx_burst() return
> 0 for long enough to look like a dead port.
> 
> Derive the capacity from queue->size, which is also what the ring wrap
> in gdma_get_wqe_pointer() uses. Rx is unaffected: its WQEs occupy
> exactly one unit, so entries and units coincide.
> 
> Fixes: 56dd45c0ce7b ("net/mana: implement hardware layer operations")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Rita Ruvinsky <rita.ruvinsky@weka.io>
> ---

Applied to next-net

The long form AI review had some observations worth including:

On Mon, 14 Sep 2026 13:58:08 +0300
Rita Ruvinsky <rita.ruvinsky@weka.io> wrote:

> gdma_post_work_request() subtracted a unit count from an entry count:

The unit analysis is right.  head/tail are advanced in alignment
units (queue->head += wqe_size / GDMA_WQE_ALIGNMENT_UNIT_SIZE, and
gdma_get_wqe_pointer() multiplies head by the same constant), while
sq_count comes from rdma-core as attr->cap.max_send_wr and sq_size as
align_hw_size(max_send_wr * get_wqe_size(max_send_sge)).  Deriving the
capacity from size is the only self-consistent choice, and it is what
mana_gd_wq_avail_space() in the kernel driver does.

Info:

1. The debug line in the -EBUSY path still reports queue->count:

	DP_LOG(DEBUG, "WQE size %u queue count %u head %u tail %u",
	       wqe_size, queue->count, queue->head, queue->tail);

After this patch count no longer takes part in the decision for the
send or receive queue; only gdma_poll_completion_queue() still uses it,
for the CQ.  The one line printed when a post is rejected no longer
shows what it was rejected against.  Suggest:

	DP_LOG(DEBUG, "WQE size %u queue size %u free %u head %u tail %u",
	       wqe_size, queue->size, queue_free_units,
	       queue->head, queue->tail);

2. The comment describes the old bug rather than the invariant:

	/* head/tail count WQE alignment units, so the capacity they are
	 * compared against must too: queue->count is in entries and
	 * undercounts the queue, stalling Tx well below capacity.
	 */

The stall belongs in the commit message, where it already is.  In the
source the invariant is enough:

	/* head and tail are in WQE alignment units, so the capacity must
	 * come from the queue size in bytes, not the entry count.
	 */

3. Worth a sentence in the commit message that the kernel mana driver
computes the same limit in mana_gd_wq_avail_space(), in bytes:

	u32 used_space = (wq->head - wq->tail) * GDMA_WQE_BU_SIZE;
	return wq->queue_size - used_space;

It is independent confirmation of the unit convention and tells anyone
backporting this that the two drivers now agree.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [EXTERNAL] Re: [PATCH] net/mana: fix Tx stall from send queue free-space unit mismatch
  2026-09-14 16:14 ` Stephen Hemminger
@ 2026-09-14 16:24   ` Wei Hu
  2026-09-16 11:12     ` Rita Ruvinsky
  0 siblings, 1 reply; 6+ messages in thread
From: Wei Hu @ 2026-09-14 16:24 UTC (permalink / raw)
  To: Stephen Hemminger, Rita Ruvinsky
  Cc: dev@dpdk.org, longli@microsoft.com, stable@dpdk.org



> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, September 15, 2026 12:14 AM
> To: Rita Ruvinsky <rita.ruvinsky@weka.io>
> Cc: dev@dpdk.org; longli@microsoft.com; Wei Hu <weh@microsoft.com>;
> stable@dpdk.org
> Subject: [EXTERNAL] Re: [PATCH] net/mana: fix Tx stall from send queue free-
> space unit mismatch
> 
> On Mon, 14 Sep 2026 13:58:08 +0300
> Rita Ruvinsky <rita.ruvinsky@weka.io> wrote:
> 
> > gdma_post_work_request() subtracted a unit count from an entry count:
> >
> >   queue_free_units = queue->count - (queue->head - queue->tail);
> >
> > queue->count is in entries, while head and tail are in WQE alignment
> > units. On a 512-entry, 128KB send queue the check saw 512 units of
> > capacity instead of queue->size / GDMA_WQE_ALIGNMENT_UNIT_SIZE =
> 4096,
> > and returned -EBUSY with the queue one eighth full. A workload that
> > fills that window faster than it drains makes rte_eth_tx_burst()
> > return
> > 0 for long enough to look like a dead port.
> >
> > Derive the capacity from queue->size, which is also what the ring wrap
> > in gdma_get_wqe_pointer() uses. Rx is unaffected: its WQEs occupy
> > exactly one unit, so entries and units coincide.
> >
> > Fixes: 56dd45c0ce7b ("net/mana: implement hardware layer operations")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Rita Ruvinsky <rita.ruvinsky@weka.io>
> > ---
> 
> Applied to next-net
> 
> The long form AI review had some observations worth including:
> 
> On Mon, 14 Sep 2026 13:58:08 +0300
> Rita Ruvinsky <rita.ruvinsky@weka.io> wrote:
> 
> > gdma_post_work_request() subtracted a unit count from an entry count:
> 
> The unit analysis is right.  head/tail are advanced in alignment units (queue-
> >head += wqe_size / GDMA_WQE_ALIGNMENT_UNIT_SIZE, and
> gdma_get_wqe_pointer() multiplies head by the same constant), while
> sq_count comes from rdma-core as attr->cap.max_send_wr and sq_size as
> align_hw_size(max_send_wr * get_wqe_size(max_send_sge)).  Deriving the
> capacity from size is the only self-consistent choice, and it is what
> mana_gd_wq_avail_space() in the kernel driver does.
> 
> Info:
> 
> 1. The debug line in the -EBUSY path still reports queue->count:
> 
> 	DP_LOG(DEBUG, "WQE size %u queue count %u head %u tail %u",
> 	       wqe_size, queue->count, queue->head, queue->tail);
> 
> After this patch count no longer takes part in the decision for the send or
> receive queue; only gdma_poll_completion_queue() still uses it, for the CQ.
> The one line printed when a post is rejected no longer shows what it was
> rejected against.  Suggest:
> 
> 	DP_LOG(DEBUG, "WQE size %u queue size %u free %u head %u
> tail %u",
> 	       wqe_size, queue->size, queue_free_units,
> 	       queue->head, queue->tail);
> 
> 2. The comment describes the old bug rather than the invariant:
> 
> 	/* head/tail count WQE alignment units, so the capacity they are
> 	 * compared against must too: queue->count is in entries and
> 	 * undercounts the queue, stalling Tx well below capacity.
> 	 */
> 
> The stall belongs in the commit message, where it already is.  In the source the
> invariant is enough:
> 
> 	/* head and tail are in WQE alignment units, so the capacity must
> 	 * come from the queue size in bytes, not the entry count.
> 	 */
> 
> 3. Worth a sentence in the commit message that the kernel mana driver
> computes the same limit in mana_gd_wq_avail_space(), in bytes:
> 
> 	u32 used_space = (wq->head - wq->tail) * GDMA_WQE_BU_SIZE;
> 	return wq->queue_size - used_space;
> 
> It is independent confirmation of the unit convention and tells anyone
> backporting this that the two drivers now agree.

I was about to say the same. The change in DP_LOG and code comments
all make a lot of sense. Thanks for fixing this. 

Wei

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [EXTERNAL] Re: [PATCH] net/mana: fix Tx stall from send queue free-space unit mismatch
  2026-09-14 16:24   ` [EXTERNAL] " Wei Hu
@ 2026-09-16 11:12     ` Rita Ruvinsky
  2026-09-16 15:17       ` Stephen Hemminger
  0 siblings, 1 reply; 6+ messages in thread
From: Rita Ruvinsky @ 2026-09-16 11:12 UTC (permalink / raw)
  To: Wei Hu
  Cc: Stephen Hemminger, dev@dpdk.org, longli@microsoft.com,
	stable@dpdk.org

[-- Attachment #1: Type: text/plain, Size: 4346 bytes --]

Hi Stephen,
I am happy to send these as a follow-up if you've already committed v1 or
create a v2 patch.

Thanks,
Rita

On Mon, Sep 14, 2026 at 7:25 PM Wei Hu <weh@microsoft.com> wrote:

>
>
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Tuesday, September 15, 2026 12:14 AM
> > To: Rita Ruvinsky <rita.ruvinsky@weka.io>
> > Cc: dev@dpdk.org; longli@microsoft.com; Wei Hu <weh@microsoft.com>;
> > stable@dpdk.org
> > Subject: [EXTERNAL] Re: [PATCH] net/mana: fix Tx stall from send queue
> free-
> > space unit mismatch
> >
> > On Mon, 14 Sep 2026 13:58:08 +0300
> > Rita Ruvinsky <rita.ruvinsky@weka.io> wrote:
> >
> > > gdma_post_work_request() subtracted a unit count from an entry count:
> > >
> > >   queue_free_units = queue->count - (queue->head - queue->tail);
> > >
> > > queue->count is in entries, while head and tail are in WQE alignment
> > > units. On a 512-entry, 128KB send queue the check saw 512 units of
> > > capacity instead of queue->size / GDMA_WQE_ALIGNMENT_UNIT_SIZE =
> > 4096,
> > > and returned -EBUSY with the queue one eighth full. A workload that
> > > fills that window faster than it drains makes rte_eth_tx_burst()
> > > return
> > > 0 for long enough to look like a dead port.
> > >
> > > Derive the capacity from queue->size, which is also what the ring wrap
> > > in gdma_get_wqe_pointer() uses. Rx is unaffected: its WQEs occupy
> > > exactly one unit, so entries and units coincide.
> > >
> > > Fixes: 56dd45c0ce7b ("net/mana: implement hardware layer operations")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Rita Ruvinsky <rita.ruvinsky@weka.io>
> > > ---
> >
> > Applied to next-net
> >
> > The long form AI review had some observations worth including:
> >
> > On Mon, 14 Sep 2026 13:58:08 +0300
> > Rita Ruvinsky <rita.ruvinsky@weka.io> wrote:
> >
> > > gdma_post_work_request() subtracted a unit count from an entry count:
> >
> > The unit analysis is right.  head/tail are advanced in alignment units
> (queue-
> > >head += wqe_size / GDMA_WQE_ALIGNMENT_UNIT_SIZE, and
> > gdma_get_wqe_pointer() multiplies head by the same constant), while
> > sq_count comes from rdma-core as attr->cap.max_send_wr and sq_size as
> > align_hw_size(max_send_wr * get_wqe_size(max_send_sge)).  Deriving the
> > capacity from size is the only self-consistent choice, and it is what
> > mana_gd_wq_avail_space() in the kernel driver does.
> >
> > Info:
> >
> > 1. The debug line in the -EBUSY path still reports queue->count:
> >
> >       DP_LOG(DEBUG, "WQE size %u queue count %u head %u tail %u",
> >              wqe_size, queue->count, queue->head, queue->tail);
> >
> > After this patch count no longer takes part in the decision for the send
> or
> > receive queue; only gdma_poll_completion_queue() still uses it, for the
> CQ.
> > The one line printed when a post is rejected no longer shows what it was
> > rejected against.  Suggest:
> >
> >       DP_LOG(DEBUG, "WQE size %u queue size %u free %u head %u
> > tail %u",
> >              wqe_size, queue->size, queue_free_units,
> >              queue->head, queue->tail);
> >
> > 2. The comment describes the old bug rather than the invariant:
> >
> >       /* head/tail count WQE alignment units, so the capacity they are
> >        * compared against must too: queue->count is in entries and
> >        * undercounts the queue, stalling Tx well below capacity.
> >        */
> >
> > The stall belongs in the commit message, where it already is.  In the
> source the
> > invariant is enough:
> >
> >       /* head and tail are in WQE alignment units, so the capacity must
> >        * come from the queue size in bytes, not the entry count.
> >        */
> >
> > 3. Worth a sentence in the commit message that the kernel mana driver
> > computes the same limit in mana_gd_wq_avail_space(), in bytes:
> >
> >       u32 used_space = (wq->head - wq->tail) * GDMA_WQE_BU_SIZE;
> >       return wq->queue_size - used_space;
> >
> > It is independent confirmation of the unit convention and tells anyone
> > backporting this that the two drivers now agree.
>
> I was about to say the same. The change in DP_LOG and code comments
> all make a lot of sense. Thanks for fixing this.
>
> Wei
>

[-- Attachment #2: Type: text/html, Size: 5989 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [EXTERNAL] Re: [PATCH] net/mana: fix Tx stall from send queue free-space unit mismatch
  2026-09-16 11:12     ` Rita Ruvinsky
@ 2026-09-16 15:17       ` Stephen Hemminger
  0 siblings, 0 replies; 6+ messages in thread
From: Stephen Hemminger @ 2026-09-16 15:17 UTC (permalink / raw)
  To: Rita Ruvinsky; +Cc: Wei Hu, dev@dpdk.org, longli@microsoft.com, stable@dpdk.org

On Wed, 16 Sep 2026 14:12:12 +0300
Rita Ruvinsky <rita.ruvinsky@weka.io> wrote:

> Hi Stephen,
> I am happy to send these as a follow-up if you've already committed v1 or
> create a v2 patch.
> 
> Thanks,
> Rita

It is in next-net branch now. But if you send V2 I can swap to new version.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2] net/mana: fix Tx stall from send queue free-space unit mismatch
  2026-09-14 10:58 [PATCH] net/mana: fix Tx stall from send queue free-space unit mismatch Rita Ruvinsky
  2026-09-14 16:14 ` Stephen Hemminger
@ 2026-09-17  6:23 ` Rita Ruvinsky
  1 sibling, 0 replies; 6+ messages in thread
From: Rita Ruvinsky @ 2026-09-17  6:23 UTC (permalink / raw)
  To: dev; +Cc: stephen, weh, stable, Rita Ruvinsky

gdma_post_work_request() subtracted a unit count from an entry count:

  queue_free_units = queue->count - (queue->head - queue->tail);

queue->count is in entries, while head and tail are in WQE alignment
units. On a 512-entry, 128KB send queue the check saw 512 units of
capacity instead of queue->size / GDMA_WQE_ALIGNMENT_UNIT_SIZE = 4096,
and returned -EBUSY with the queue one eighth full. A workload that
fills that window faster than it drains makes rte_eth_tx_burst() return
0 for long enough to look like a dead port.

The size is the authoritative capacity: rdma-core derives sq_size from
sq_count as align_hw_size(max_send_wr * get_wqe_size(max_send_sge)), and
the kernel mana driver computes the same limit in bytes in
mana_gd_wq_avail_space(). Derive it from queue->size, which is also what
the ring wrap in gdma_get_wqe_pointer() uses. Rx is unaffected: its WQEs
occupy exactly one unit, so entries and units coincide.

Also report the size and computed free space in the -EBUSY debug line,
since queue->count no longer takes part in the decision.

Fixes: 56dd45c0ce7b ("net/mana: implement hardware layer operations")
Cc: stable@dpdk.org

Signed-off-by: Rita Ruvinsky <rita.ruvinsky@weka.io>
---
v2:
  - comment states the invariant rather than the old bug (Stephen Hemminger)
  - report queue->size and the computed free space in the -EBUSY debug line
    (Stephen Hemminger, Wei Hu)
  - reference mana_gd_wq_avail_space() in the commit message (Stephen Hemminger)

Independent of patchwork 167383 and 167384 from the same author.

 drivers/net/mana/gdma.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/mana/gdma.c b/drivers/net/mana/gdma.c
index 7f66a7a7cf..a92aef59d2 100644
--- a/drivers/net/mana/gdma.c
+++ b/drivers/net/mana/gdma.c
@@ -138,11 +138,16 @@ gdma_post_work_request(struct mana_gdma_queue *queue,
 				client_oob_size + sgl_data_size,
 			  GDMA_WQE_ALIGNMENT_UNIT_SIZE);
 	uint8_t *wq_buffer_pointer;
-	uint32_t queue_free_units = queue->count - (queue->head - queue->tail);
+	/* head and tail are in WQE alignment units, so the capacity must
+	 * come from the queue size in bytes, not the entry count.
+	 */
+	uint32_t queue_free_units = queue->size / GDMA_WQE_ALIGNMENT_UNIT_SIZE -
+					(queue->head - queue->tail);
 
 	if (wqe_size / GDMA_WQE_ALIGNMENT_UNIT_SIZE > queue_free_units) {
-		DP_LOG(DEBUG, "WQE size %u queue count %u head %u tail %u",
-		       wqe_size, queue->count, queue->head, queue->tail);
+		DP_LOG(DEBUG, "WQE size %u queue size %u free %u head %u tail %u",
+		       wqe_size, queue->size, queue_free_units,
+		       queue->head, queue->tail);
 		return -EBUSY;
 	}
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-09-17  6:24 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-14 10:58 [PATCH] net/mana: fix Tx stall from send queue free-space unit mismatch Rita Ruvinsky
2026-09-14 16:14 ` Stephen Hemminger
2026-09-14 16:24   ` [EXTERNAL] " Wei Hu
2026-09-16 11:12     ` Rita Ruvinsky
2026-09-16 15:17       ` Stephen Hemminger
2026-09-17  6:23 ` [PATCH v2] " Rita Ruvinsky

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox