[dpdk-stable] [PATCH 1/4] net/virtio: fix memory leak in in-order Rx

[dpdk-stable] [PATCH 1/4] net/virtio: fix memory leak in in-order Rx

[Tiwei Bie]

When there is no enough segments for a packet in in-order
mergeable Rx path, we should free the whole mbuf chain instead
of just the last segment.

Fixes: e5f456a98d3c ("net/virtio: support in-order Rx and Tx")
Cc: stable@dpdk.org

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 drivers/net/virtio/virtio_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 1f1178467..bdb3a2f18 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -1555,7 +1555,7 @@ virtio_recv_pkts_inorder(void *rx_queue,
 		} else {
 			PMD_RX_LOG(ERR,
 					"No enough segments for packet.");
-			virtio_discard_rxbuf_inorder(vq, prev);
+			rte_pktmbuf_free(rx_pkts[nb_rx]);
 			rxvq->stats.errors++;
 			break;
 		}
-- 
2.17.1

Re: [dpdk-stable] [PATCH 1/4] net/virtio: fix memory leak in in-order Rx

[Maxime Coquelin]

On 6/18/19 9:41 AM, Tiwei Bie wrote:
> When there is no enough segments for a packet in in-order
> mergeable Rx path, we should free the whole mbuf chain instead
> of just the last segment.

I would write instead:

"
we should free the whole mbuf chain instead of just recycling the last 
segment.
"

Because what was done before the patch it to refill the VQ with the last
segment. With your patch, the full chain is freed, then the refill is
done afterwards.

Do you agree?

Other than that:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks!
Maxime

> 
> Fixes: e5f456a98d3c ("net/virtio: support in-order Rx and Tx")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   drivers/net/virtio/virtio_rxtx.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index 1f1178467..bdb3a2f18 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -1555,7 +1555,7 @@ virtio_recv_pkts_inorder(void *rx_queue,
>   		} else {
>   			PMD_RX_LOG(ERR,
>   					"No enough segments for packet.");
> -			virtio_discard_rxbuf_inorder(vq, prev);
> +			rte_pktmbuf_free(rx_pkts[nb_rx]);
>   			rxvq->stats.errors++;
>   			break;
>   		}
> 

Re: [dpdk-stable] [PATCH 1/4] net/virtio: fix memory leak in in-order Rx

[Tiwei Bie]

On Wed, Jun 19, 2019 at 02:34:40PM +0200, Maxime Coquelin wrote:
> On 6/18/19 9:41 AM, Tiwei Bie wrote:
> > When there is no enough segments for a packet in in-order
> > mergeable Rx path, we should free the whole mbuf chain instead
> > of just the last segment.
> 
> I would write instead:
> 
> "
> we should free the whole mbuf chain instead of just recycling the last
> segment.
> "
> 
> Because what was done before the patch it to refill the VQ with the last
> segment. With your patch, the full chain is freed, then the refill is
> done afterwards.
> 
> Do you agree?

Yeah, I totally agree :)

Thanks!
Tiwei

> 
> Other than that:
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Thanks!
> Maxime
> 
> > 
> > Fixes: e5f456a98d3c ("net/virtio: support in-order Rx and Tx")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > ---
> >   drivers/net/virtio/virtio_rxtx.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> > index 1f1178467..bdb3a2f18 100644
> > --- a/drivers/net/virtio/virtio_rxtx.c
> > +++ b/drivers/net/virtio/virtio_rxtx.c
> > @@ -1555,7 +1555,7 @@ virtio_recv_pkts_inorder(void *rx_queue,
> >   		} else {
> >   			PMD_RX_LOG(ERR,
> >   					"No enough segments for packet.");
> > -			virtio_discard_rxbuf_inorder(vq, prev);
> > +			rte_pktmbuf_free(rx_pkts[nb_rx]);
> >   			rxvq->stats.errors++;
> >   			break;
> >   		}
> > 

[dpdk-stable] [PATCH 2/4] net/virtio: fix memory leak in mergeable Rx

[Tiwei Bie]

When there is no enough segments for a packet in mergeable
Rx path, we should free the whole mbuf chain instead of just
the last segment.

Fixes: bcac5aa207f8 ("net/virtio: improve batching in mergeable path")
Cc: stable@dpdk.org

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 drivers/net/virtio/virtio_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index bdb3a2f18..9cf422ffe 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -1737,7 +1737,7 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 		} else {
 			PMD_RX_LOG(ERR,
 					"No enough segments for packet.");
-			virtio_discard_rxbuf(vq, prev);
+			rte_pktmbuf_free(rx_pkts[nb_rx]);
 			rxvq->stats.errors++;
 			break;
 		}
-- 
2.17.1

Re: [dpdk-stable] [PATCH 2/4] net/virtio: fix memory leak in mergeable Rx

[Maxime Coquelin]

On 6/18/19 9:41 AM, Tiwei Bie wrote:
> When there is no enough segments for a packet in mergeable
> Rx path, we should free the whole mbuf chain instead of just
> the last segment.
> 
> Fixes: bcac5aa207f8 ("net/virtio: improve batching in mergeable path")
> Cc:stable@dpdk.org
> 
> Signed-off-by: Tiwei Bie<tiwei.bie@intel.com>
> ---
>   drivers/net/virtio/virtio_rxtx.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Same comment as patch 1.

Other than that:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

[dpdk-stable] [PATCH 3/4] net/virtio: fix memory leak in mergeable packed Rx

[Tiwei Bie]

When there is no enough segments for a packet in mergeable
packed Rx path, we should free the whole mbuf chain instead
of just the last segment.

Fixes: a76290c8f1cf ("net/virtio: implement Rx path for packed queues")
Cc: stable@dpdk.org

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 drivers/net/virtio/virtio_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 9cf422ffe..8b75291f5 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -1910,7 +1910,7 @@ virtio_recv_mergeable_pkts_packed(void *rx_queue,
 		} else {
 			PMD_RX_LOG(ERR,
 					"No enough segments for packet.");
-			virtio_discard_rxbuf(vq, prev);
+			rte_pktmbuf_free(rx_pkts[nb_rx]);
 			rxvq->stats.errors++;
 			break;
 		}
-- 
2.17.1

Re: [dpdk-stable] [PATCH 3/4] net/virtio: fix memory leak in mergeable packed Rx

[Maxime Coquelin]

On 6/18/19 9:41 AM, Tiwei Bie wrote:
> When there is no enough segments for a packet in mergeable
> packed Rx path, we should free the whole mbuf chain instead
> of just the last segment.
> 
> Fixes: a76290c8f1cf ("net/virtio: implement Rx path for packed queues")
> Cc:stable@dpdk.org
> 
> Signed-off-by: Tiwei Bie<tiwei.bie@intel.com>
> ---
>   drivers/net/virtio/virtio_rxtx.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Same comment as patch 1.

Other than that:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

[dpdk-stable] [PATCH 4/4] net/virtio: fix packets check in mergeable packed Rx

[Tiwei Bie]

We should check the descriptor state instead of vq's internal
free count (i.e. the number of descriptors that we haven't made
available) for the remaining mergeable packets.

Fixes: a76290c8f1cf ("net/virtio: implement Rx path for packed queues")
Cc: stable@dpdk.org

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 drivers/net/virtio/virtio_rxtx.c | 50 +++++++++++++++-----------------
 1 file changed, 23 insertions(+), 27 deletions(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 8b75291f5..b5062ff84 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -1882,38 +1882,34 @@ virtio_recv_mergeable_pkts_packed(void *rx_queue,
 	while (seg_res != 0) {
 		uint16_t rcv_cnt = RTE_MIN((uint16_t)seg_res,
 					VIRTIO_MBUF_BURST_SZ);
-		if (likely(vq->vq_free_cnt >= rcv_cnt)) {
-			num = virtqueue_dequeue_burst_rx_packed(vq, rcv_pkts,
-					len, rcv_cnt);
-			uint16_t extra_idx = 0;
+		uint16_t extra_idx = 0;
 
-			rcv_cnt = num;
-
-			while (extra_idx < rcv_cnt) {
-				rxm = rcv_pkts[extra_idx];
-
-				rxm->data_off =
-					RTE_PKTMBUF_HEADROOM - hdr_size;
-				rxm->pkt_len = (uint32_t)(len[extra_idx]);
-				rxm->data_len = (uint16_t)(len[extra_idx]);
-
-				prev->next = rxm;
-				prev = rxm;
-				rx_pkts[nb_rx]->pkt_len += len[extra_idx];
-				extra_idx += 1;
-			}
-			seg_res -= rcv_cnt;
-			if (!seg_res) {
-				virtio_rx_stats_updated(rxvq, rx_pkts[nb_rx]);
-				nb_rx++;
-			}
-		} else {
-			PMD_RX_LOG(ERR,
-					"No enough segments for packet.");
+		rcv_cnt = virtqueue_dequeue_burst_rx_packed(vq, rcv_pkts,
+				len, rcv_cnt);
+		if (unlikely(rcv_cnt == 0)) {
+			PMD_RX_LOG(ERR, "No enough segments for packet.");
 			rte_pktmbuf_free(rx_pkts[nb_rx]);
 			rxvq->stats.errors++;
 			break;
 		}
+
+		while (extra_idx < rcv_cnt) {
+			rxm = rcv_pkts[extra_idx];
+
+			rxm->data_off = RTE_PKTMBUF_HEADROOM - hdr_size;
+			rxm->pkt_len = (uint32_t)(len[extra_idx]);
+			rxm->data_len = (uint16_t)(len[extra_idx]);
+
+			prev->next = rxm;
+			prev = rxm;
+			rx_pkts[nb_rx]->pkt_len += len[extra_idx];
+			extra_idx += 1;
+		}
+		seg_res -= rcv_cnt;
+		if (!seg_res) {
+			virtio_rx_stats_updated(rxvq, rx_pkts[nb_rx]);
+			nb_rx++;
+		}
 	}
 
 	rxvq->stats.packets += nb_rx;
-- 
2.17.1

Re: [dpdk-stable] [PATCH 4/4] net/virtio: fix packets check in mergeable packed Rx

[Maxime Coquelin]

On 6/18/19 9:41 AM, Tiwei Bie wrote:
> We should check the descriptor state instead of vq's internal
> free count (i.e. the number of descriptors that we haven't made
> available) for the remaining mergeable packets.
> 
> Fixes: a76290c8f1cf ("net/virtio: implement Rx path for packed queues")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   drivers/net/virtio/virtio_rxtx.c | 50 +++++++++++++++-----------------
>   1 file changed, 23 insertions(+), 27 deletions(-)
> 


Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime