|SUCCESS| net/mana: fix RX mempool leak on port stop

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/162956

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-09

Commit: [git sha not provided in patch file]
Author: Long Li <longli at microsoft.com>
Subject: [PATCH] net/mana: fix RX mempool leak on port stop

This patch fixes a resource leak in the MANA driver where RX mbufs are
not properly freed during port stop due to incorrect ring drain logic.


> diff --git a/drivers/net/mana/mana.h b/drivers/net/mana/mana.h
> index 7d94840dc4..79cc47b6ab 100644
> --- a/drivers/net/mana/mana.h
> +++ b/drivers/net/mana/mana.h
> @@ -443,7 +443,7 @@ struct mana_rxq {
>  	/* desc_ring_head is where we put pending requests to ring,
>  	 * completion pull off desc_ring_tail
>  	 */
> -	uint32_t desc_ring_head, desc_ring_tail;
> +	uint32_t desc_ring_head, desc_ring_tail, desc_ring_len;

Style: Multiple variable declarations on a single line should only be used
for closely related variables. Consider placing desc_ring_len on its own
line with a comment explaining its purpose, similar to the comment for
head and tail.

[ ... ]

> diff --git a/drivers/net/mana/rx.c b/drivers/net/mana/rx.c
> index f196d43aee..1b8ba1f3a9 100644
> --- a/drivers/net/mana/rx.c
> +++ b/drivers/net/mana/rx.c
> @@ -102,6 +102,7 @@ mana_post_rx_wqe(struct mana_rxq *rxq, struct rte_mbuf *mbuf)
>  		rxq->wqe_cnt_to_short_db += wqe_size_in_bu;
>  #endif
>  		rxq->desc_ring_head = (rxq->desc_ring_head + 1) % rxq->num_desc;
> +		rxq->desc_ring_len++;
>  	} else {
>  		DP_LOG(DEBUG, "failed to post recv ret %d", ret);
>  		return ret;

Does desc_ring_len overflow if rxq->num_desc is reached? The increment
happens after the modulo operation on desc_ring_head, but there's no
corresponding check that desc_ring_len doesn't exceed rxq->num_desc.
Should there be a bounds check or assertion here?

> @@ -215,7 +216,7 @@ mana_stop_rx_queues(struct rte_eth_dev *dev)
>  		}
>  
>  		/* Drain and free posted WQEs */
> -		while (rxq->desc_ring_tail != rxq->desc_ring_head) {
> +		while (rxq->desc_ring_len > 0) {
>  			struct mana_rxq_desc *desc =
>  				&rxq->desc_ring[rxq->desc_ring_tail];
>  
> @@ -223,9 +224,11 @@ mana_stop_rx_queues(struct rte_eth_dev *dev)
>  
>  			rxq->desc_ring_tail =
>  				(rxq->desc_ring_tail + 1) % rxq->num_desc;
> +			rxq->desc_ring_len--;
>  		}
>  		rxq->desc_ring_head = 0;
>  		rxq->desc_ring_tail = 0;
> +		rxq->desc_ring_len = 0;

The fix looks correct for the drain loop. However, is desc_ring_len
initialized to zero when the queue is first allocated? If this field is
not properly initialized in the queue setup function, could we be starting
with a garbage value?

> @@ -560,6 +563,7 @@ mana_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n)
>  			rxq->desc_ring_tail = 0;
>  
>  		rxq->gdma_rq.tail += desc->wqe_size_in_bu;
> +		rxq->desc_ring_len--;

Is desc_ring_len decremented in all code paths where descriptors are
consumed from the ring? Are there any error paths or exceptional cases in
mana_rx_burst() where a descriptor might be consumed but desc_ring_len is
not decremented, leading to incorrect accounting?

[ ... ]

> diff --git a/drivers/net/mana/tx.c b/drivers/net/mana/tx.c
> index e5ab566e8a..57dbbc3651 100644
> --- a/drivers/net/mana/tx.c
> +++ b/drivers/net/mana/tx.c
> @@ -39,7 +39,7 @@ mana_stop_tx_queues(struct rte_eth_dev *dev)
>  		}
>  
>  		/* Drain and free posted WQEs */
> -		while (txq->desc_ring_tail != txq->desc_ring_head) {
> +		while (txq->desc_ring_len > 0) {

The commit message states this change is for "consistency" with RX, but
does the TX ring have the same full-ring ambiguity issue as RX? If TX
already had desc_ring_len tracking, was it being maintained correctly
before this patch? Should the commit message clarify whether this fixes an
actual TX bug or is purely for consistency?