[dpdk-dev] [PATCH 0/4] Ena PMD fixes

[dpdk-dev] [PATCH 0/4] Ena PMD fixes

[Marcin Wojtas]

Hi,

I sent 4 various fixes for ena PMD. 3 of them fix descriptors
handling and the last one adjusts to the device firmware issue.

We are looking forward to any comments or remarks.

Best regards,
Marcin

Michal Krawczyk (4):
  net/ena: fix incorrect Rx descriptors allocation
  net/ena: fix delayed cleanup of Rx descriptors
  net/ena: cleanup if refilling of Rx descriptors fails
  net/ena: calculate partial checksum if DF bit is disabled

 drivers/net/ena/ena_ethdev.c | 52 ++++++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 19 deletions(-)

-- 
1.8.3.1

[dpdk-dev] [PATCH 1/4] net/ena: fix incorrect Rx descriptors allocation

[Marcin Wojtas]

From: Michal Krawczyk <mk@semihalf.com>

When application tried to allocate 1024 descriptors, device was not
initializing properly.

This patch solves it by avoiding allocation of all descriptors in the ring
in one attempt. At least one descriptor must remain unused in the HW ring.

Fixes: 1173fca25af9 ("ena: add polling-mode driver")

Signed-off-by: Michal Krawczyk <mk@semihalf.com>
---
 drivers/net/ena/ena_ethdev.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
index b5e6db6..2345bab 100644
--- a/drivers/net/ena/ena_ethdev.c
+++ b/drivers/net/ena/ena_ethdev.c
@@ -919,7 +919,7 @@ static int ena_start(struct rte_eth_dev *dev)
 
 static int ena_queue_restart(struct ena_ring *ring)
 {
-	int rc;
+	int rc, bufs_num;
 
 	ena_assert_msg(ring->configured == 1,
 		       "Trying to restart unconfigured queue\n");
@@ -930,8 +930,9 @@ static int ena_queue_restart(struct ena_ring *ring)
 	if (ring->type == ENA_RING_TYPE_TX)
 		return 0;
 
-	rc = ena_populate_rx_queue(ring, ring->ring_size);
-	if ((unsigned int)rc != ring->ring_size) {
+	bufs_num = ring->ring_size - 1;
+	rc = ena_populate_rx_queue(ring, bufs_num);
+	if (rc != bufs_num) {
 		PMD_INIT_LOG(ERR, "Failed to populate rx ring !");
 		return (-1);
 	}
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 1/4] net/ena: fix incorrect Rx descriptors allocation

[Jakub Palider]

Looks good to me but please, check commnt on patch 2/4. Moving the "-1"
indexing fix from patch 2/4 into 1/4 will reflect all related changes in
one place. But this is not a functional change, so may go in current shape
as well.

Jakub

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

> From: Michal Krawczyk <mk@semihalf.com>
>
> When application tried to allocate 1024 descriptors, device was not
> initializing properly.
>
> This patch solves it by avoiding allocation of all descriptors in the ring
> in one attempt. At least one descriptor must remain unused in the HW ring.
>
> Fixes: 1173fca25af9 ("ena: add polling-mode driver")
>
> Signed-off-by: Michal Krawczyk <mk@semihalf.com>
> ---
>  drivers/net/ena/ena_ethdev.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
> index b5e6db6..2345bab 100644
> --- a/drivers/net/ena/ena_ethdev.c
> +++ b/drivers/net/ena/ena_ethdev.c
> @@ -919,7 +919,7 @@ static int ena_start(struct rte_eth_dev *dev)
>
>  static int ena_queue_restart(struct ena_ring *ring)
>  {
> -       int rc;
> +       int rc, bufs_num;
>
>         ena_assert_msg(ring->configured == 1,
>                        "Trying to restart unconfigured queue\n");
> @@ -930,8 +930,9 @@ static int ena_queue_restart(struct ena_ring *ring)
>         if (ring->type == ENA_RING_TYPE_TX)
>                 return 0;
>
> -       rc = ena_populate_rx_queue(ring, ring->ring_size);
> -       if ((unsigned int)rc != ring->ring_size) {
> +       bufs_num = ring->ring_size - 1;
> +       rc = ena_populate_rx_queue(ring, bufs_num);
> +       if (rc != bufs_num) {
>                 PMD_INIT_LOG(ERR, "Failed to populate rx ring !");
>                 return (-1);
>         }
> --
> 1.8.3.1
>
>

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

[Marcin Wojtas]

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

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

[Jakub Palider]

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


​

[dpdk-dev] [PATCH 3/4] net/ena: cleanup if refilling of Rx descriptors fails

[Marcin Wojtas]

From: Michal Krawczyk <mk@semihalf.com>

If wrong number of descriptors for refilling was passed to the Rx
repopulate function, there was memory leak which caused memory pool to
run out of resources in longer go.

In case of fail when refilling Rx descriptors, all additional mbufs
have to be released.

Fixes: 1173fca25af9 ("ena: add polling-mode driver")

Signed-off-by: Michal Krawczyk <mk@semihalf.com>
---
 drivers/net/ena/ena_ethdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
index d875a2b..54ba5c1 100644
--- a/drivers/net/ena/ena_ethdev.c
+++ b/drivers/net/ena/ena_ethdev.c
@@ -1172,6 +1172,8 @@ static int ena_populate_rx_queue(struct ena_ring *rxq, unsigned int count)
 		rc = ena_com_add_single_rx_desc(rxq->ena_com_io_sq,
 						&ebuf, next_to_use_masked);
 		if (unlikely(rc)) {
+			rte_mempool_put_bulk(rxq->mb_pool, (void **)(&mbuf),
+					     count - i);
 			RTE_LOG(WARNING, PMD, "failed adding rx desc\n");
 			break;
 		}
-- 
1.8.3.1

[dpdk-dev] [PATCH 4/4] net/ena: calculate partial checksum if DF bit is disabled

[Marcin Wojtas]

From: Michal Krawczyk <mk@semihalf.com>

When TSO is disabled we still have to calculate partial checksum if DF bit
if turned off. This is caused by firmware bug.

If application will not set m2_len field, we assume we that it was Ethernet
frame because we have to look inside the packet to check for the DF flag.
To make it work properly, PMD is assuming that before sending
packet application called function rte_eth_tx_prepare().

Signed-off-by: Michal Krawczyk <mk@semihalf.com>
---
 drivers/net/ena/ena_ethdev.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
index 54ba5c1..bb2a8ba 100644
--- a/drivers/net/ena/ena_ethdev.c
+++ b/drivers/net/ena/ena_ethdev.c
@@ -1599,14 +1599,30 @@ static uint16_t eth_ena_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 	uint64_t ol_flags;
 	uint16_t frag_field;
 
-	/* ENA needs partial checksum for TSO packets only, skip early */
-	if (!tx_ring->adapter->tso4_supported)
-		return nb_pkts;
-
 	for (i = 0; i != nb_pkts; i++) {
 		m = tx_pkts[i];
 		ol_flags = m->ol_flags;
 
+		/* If there was not L2 header length specified, assume it is
+		 * length of the ethernet header.
+		 */
+		if (unlikely(m->l2_len == 0))
+			m->l2_len = sizeof(struct ether_hdr);
+
+		ip_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *,
+						 m->l2_len);
+		frag_field = rte_be_to_cpu_16(ip_hdr->fragment_offset);
+
+		if ((frag_field & IPV4_HDR_DF_FLAG) != 0) {
+			m->packet_type |= RTE_PTYPE_L4_NONFRAG;
+
+			/* If IPv4 header has DF flag enabled and TSO support is
+			 * disabled, partial chcecksum should not be calculated.
+			 */
+			if (!tx_ring->adapter->tso4_supported)
+				continue;
+		}
+
 		if ((ol_flags & ENA_TX_OFFLOAD_NOTSUP_MASK) != 0 ||
 				(ol_flags & PKT_TX_L4_MASK) ==
 				PKT_TX_SCTP_CKSUM) {
@@ -1625,12 +1641,6 @@ static uint16_t eth_ena_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 		if (!(m->ol_flags & PKT_TX_IPV4))
 			continue;
 
-		ip_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *,
-						 m->l2_len);
-		frag_field = rte_be_to_cpu_16(ip_hdr->fragment_offset);
-		if (frag_field & IPV4_HDR_DF_FLAG)
-			continue;
-
 		/* In case we are supposed to TSO and have DF not set (DF=0)
 		 * hardware must be provided with partial checksum, otherwise
 		 * it will take care of necessary calculations.
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 4/4] net/ena: calculate partial checksum if DF bit is disabled

[Jan Mędala]

I would recommend to check if packet type is IPv4 before processing IPv4 header for DF flag.
This patch can break logic and go to unknown state when mbuf will contain IPv6 packet. I believe that in case of IPv6 pkt the loop should be skipped to next mbuf, if exists.

Best regards,
Jan