diff mbox series

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

Message ID 20190618074159.3322-2-tiwei.bie@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series Some fixes for mergeable Rx |

Checks

Context Check Description
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS
ci/Intel-compilation fail Compilation issues
ci/checkpatch success coding style OK

Commit Message

Tiwei Bie June 18, 2019, 7:41 a.m. UTC 
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(-)

Comments

Maxime Coquelin June 19, 2019, 12:34 p.m. UTC | #1 
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;
>   		}
>
Tiwei Bie June 20, 2019, 1:56 a.m. UTC | #2 
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;
> >   		}
> >
diff mbox series

Patch

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;
 		}