Re: [dpdk-dev] [PATCH 2/4] net/ena: fix delayed cleanup of Rx descriptors

[Jakub Palider <jpalider@gmail.com>]

On Fri, Apr 7, 2017 at 12:48 PM, Marcin Wojtas <mw@semihalf.com> wrote:

> From: Michal Krawczyk <mk@semihalf.com>
>
> On RX path, after receiving bunch of packets, variable tracking
> available descriptors in HW queue was not updated.
>
> To fix this issue, additional variable was added which is storing number
> of depleted descriptors updated by number of descriptors used in this
> cycle.
>
> Finally whole number is substracted by one to do not refill all
> descriptors what is required by the driver.
>
> Fixes: 1daff5260ff8 ("net/ena: use unmasked head and tail")
>
> Signed-off-by: Michal Krawczyk <mk@semihalf.com>
> ---
>  drivers/net/ena/ena_ethdev.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
> index 2345bab..d875a2b 100644
> --- a/drivers/net/ena/ena_ethdev.c
> +++ b/drivers/net/ena/ena_ethdev.c
> @@ -1144,7 +1144,7 @@ static int ena_populate_rx_queue(struct ena_ring
> *rxq, unsigned int count)
>                 return 0;
>
>         in_use = rxq->next_to_use - rxq->next_to_clean;
> -       ena_assert_msg(((in_use + count) <= ring_size), "bad ring state");
> +       ena_assert_msg(((in_use + count) < ring_size), "bad ring state");
>
>         count = RTE_MIN(count,
>                         (uint16_t)(ring_size - (next_to_use & ring_mask)));
> @@ -1504,7 +1504,7 @@ static uint16_t eth_ena_recv_pkts(void *rx_queue,
> struct rte_mbuf **rx_pkts,
>         unsigned int ring_size = rx_ring->ring_size;
>         unsigned int ring_mask = ring_size - 1;
>         uint16_t next_to_clean = rx_ring->next_to_clean;
> -       uint16_t desc_in_use = 0;
> +       uint16_t desc_in_use, desc_to_refill;
>         unsigned int recv_idx = 0;
>         struct rte_mbuf *mbuf = NULL;
>         struct rte_mbuf *mbuf_head = NULL;
> @@ -1575,12 +1575,13 @@ static uint16_t eth_ena_recv_pkts(void *rx_queue,
> struct rte_mbuf **rx_pkts,
>                 recv_idx++;
>         }
>
> -       /* Burst refill to save doorbells, memory barriers, const interval
> */
> -       if (ring_size - desc_in_use > ENA_RING_DESCS_RATIO(ring_size))
> -               ena_populate_rx_queue(rx_ring, ring_size - desc_in_use);
> -
>         rx_ring->next_to_clean = next_to_clean;
>
> +       desc_to_refill = ring_size - desc_in_use + completed - 1;
> +       /* Burst refill to save doorbells, memory barriers, const interval
> */
> +       if (desc_to_refill > ENA_RING_DESCS_RATIO(ring_size))
> +               ena_populate_rx_queue(rx_ring, desc_to_refill);
> +
>         return recv_idx;
>  }
>
> --
> 1.8.3.1
>
>
​
Good catch! I would suggest, however, to slightly rework this this way:
adjust the desc_in_use to reflect the actual state:
desc_in_use -= completed;
so that it would be the only change in this patch, and should do the trick.
As for the adjustment "-1" I will refer to 1/4 separately.

Jakub


​