[PATCH 0/3] net/virtio: fix memory leak when reinitializing device

[PATCH 0/3] net/virtio: fix memory leak when reinitializing device

[Olivier Matz]

When devops->configure() is called, the queues may be reallocated
if the features flag changed, but the previous are not freed. This
patchset fixes this issue.

To really point out the issue, I instrumented rte_malloc,
rte_memzone_reserve, rte_mbuf_alloc to track the allocations and frees.
For reference, the patch is available here:
https://www.droids-corp.org/~zer0/hidden/0001-debug-malloc.patch
Iit conflicts a bit with the patchset, but it is quite easy to solve.

To reproduce the issue:

  cd dpdk
  make config T=x86_64-native-linuxapp-gcc
  sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=y,' build/.config
  make -j4
  mkdir -p /mnt/huge
  mount -t hugetlbfs nodev /mnt/huge
  echo 256 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
  modprobe uio_pci_generic
  python usertools/dpdk-devbind.py -b uio_pci_generic 0000:00:02.0
  ./build/app/testpmd -l 0,1 --log-level 8 -- --total-num-mbufs=16384 \
     -i --port-topology=chained --disable-hw-vlan-filter \
     --disable-hw-vlan-strip --enable-rx-cksum --enable-lro \
     --txqflags=0 --no-lsc-interrupt

  # testpmd commands
  port stop 0
  port config all rx-cksum off
  port start 0
  quit

Without the patchset, the debug patch displays a leak of mbufs allocated
from virtio_dev_rx_queue_setup_finish() and a leak of rte_mallocs allocated
from virtio_init_queue().

After the patchset, only one malloc is remaining after the device close,
but this is normal, because this area is freed in dev_uninit().

Olivier Matz (3):
  net/virtio: fix typo in function name
  net/virtio: rationalize queue flushing
  net/virtio: fix memory leak when reinitializing device

 drivers/net/virtio/virtio_ethdev.c | 60 ++++++++++++++++++++------------------
 drivers/net/virtio/virtqueue.c     |  2 +-
 drivers/net/virtio/virtqueue.h     |  2 +-
 3 files changed, 33 insertions(+), 31 deletions(-)

-- 
2.11.0

[PATCH 1/3] net/virtio: fix typo in function name

[Olivier Matz]

Fixes: c1f86306a026 ("virtio: add new driver")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 4 ++--
 drivers/net/virtio/virtqueue.c     | 2 +-
 drivers/net/virtio/virtqueue.h     | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index ec012c2ac..c7426951c 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1879,7 +1879,7 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
 		VIRTQUEUE_DUMP(rxvq->vq);
 
 		PMD_INIT_LOG(DEBUG, "rx_queues[%d]=%p", i, rxvq);
-		while ((buf = virtqueue_detatch_unused(rxvq->vq)) != NULL) {
+		while ((buf = virtqueue_detach_unused(rxvq->vq)) != NULL) {
 			rte_pktmbuf_free(buf);
 			mbuf_num++;
 		}
@@ -1899,7 +1899,7 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
 		VIRTQUEUE_DUMP(txvq->vq);
 
 		mbuf_num = 0;
-		while ((buf = virtqueue_detatch_unused(txvq->vq)) != NULL) {
+		while ((buf = virtqueue_detach_unused(txvq->vq)) != NULL) {
 			rte_pktmbuf_free(buf);
 			mbuf_num++;
 		}
diff --git a/drivers/net/virtio/virtqueue.c b/drivers/net/virtio/virtqueue.c
index 1ada4fe08..6988bfea4 100644
--- a/drivers/net/virtio/virtqueue.c
+++ b/drivers/net/virtio/virtqueue.c
@@ -16,7 +16,7 @@
  * 2) mbuf that hasn't been consued by backend.
  */
 struct rte_mbuf *
-virtqueue_detatch_unused(struct virtqueue *vq)
+virtqueue_detach_unused(struct virtqueue *vq)
 {
 	struct rte_mbuf *cookie;
 	int idx;
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index fedbaa39c..1288e5287 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -270,7 +270,7 @@ void virtqueue_dump(struct virtqueue *vq);
 /**
  *  Get all mbufs to be freed.
  */
-struct rte_mbuf *virtqueue_detatch_unused(struct virtqueue *vq);
+struct rte_mbuf *virtqueue_detach_unused(struct virtqueue *vq);
 
 /* Flush the elements in the used ring. */
 void virtqueue_rxvq_flush(struct virtqueue *vq);
-- 
2.11.0

[PATCH 2/3] net/virtio: rationalize queue flushing

[Olivier Matz]

Rationalize the function virtio_dev_free_mbufs():

- skip NULL vqs instead of crashing: this is required for the
  next commit
- use the same kind of loop than in virtio_free_queues()
- also flush mbufs from the control queue (this is useless yet)
- factorize common code between rxq, txq, cq

Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 55 ++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 29 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index c7426951c..d8b3b8ed7 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1868,47 +1868,44 @@ virtio_dev_start(struct rte_eth_dev *dev)
 
 static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
 {
+	struct virtio_hw *hw = dev->data->dev_private;
+	uint16_t nr_vq = virtio_get_nr_vq(hw);
+	const char *type __rte_unused;
+	unsigned int i, mbuf_num = 0;
+	struct virtqueue *vq;
 	struct rte_mbuf *buf;
-	int i, mbuf_num = 0;
-
-	for (i = 0; i < dev->data->nb_rx_queues; i++) {
-		struct virtnet_rx *rxvq = dev->data->rx_queues[i];
-
-		PMD_INIT_LOG(DEBUG,
-			     "Before freeing rxq[%d] used and unused buf", i);
-		VIRTQUEUE_DUMP(rxvq->vq);
-
-		PMD_INIT_LOG(DEBUG, "rx_queues[%d]=%p", i, rxvq);
-		while ((buf = virtqueue_detach_unused(rxvq->vq)) != NULL) {
-			rte_pktmbuf_free(buf);
-			mbuf_num++;
-		}
+	int queue_type;
 
-		PMD_INIT_LOG(DEBUG, "free %d mbufs", mbuf_num);
-		PMD_INIT_LOG(DEBUG,
-			     "After freeing rxq[%d] used and unused buf", i);
-		VIRTQUEUE_DUMP(rxvq->vq);
-	}
+	for (i = 0; i < nr_vq; i++) {
+		vq = hw->vqs[i];
+		if (!vq)
+			continue;
 
-	for (i = 0; i < dev->data->nb_tx_queues; i++) {
-		struct virtnet_tx *txvq = dev->data->tx_queues[i];
+		queue_type = virtio_get_queue_type(hw, i);
+		if (queue_type == VTNET_RQ)
+			type = "rxq";
+		else if (queue_type == VTNET_TQ)
+			type = "txq";
+		else
+			type = "cq";
 
 		PMD_INIT_LOG(DEBUG,
-			     "Before freeing txq[%d] used and unused bufs",
-			     i);
-		VIRTQUEUE_DUMP(txvq->vq);
+			"Before freeing %s[%d] used and unused buf",
+			type, i);
+		VIRTQUEUE_DUMP(vq);
 
-		mbuf_num = 0;
-		while ((buf = virtqueue_detach_unused(txvq->vq)) != NULL) {
+		while ((buf = virtqueue_detach_unused(vq)) != NULL) {
 			rte_pktmbuf_free(buf);
 			mbuf_num++;
 		}
 
-		PMD_INIT_LOG(DEBUG, "free %d mbufs", mbuf_num);
 		PMD_INIT_LOG(DEBUG,
-			     "After freeing txq[%d] used and unused buf", i);
-		VIRTQUEUE_DUMP(txvq->vq);
+			"After freeing %s[%d] used and unused buf",
+			type, i);
+		VIRTQUEUE_DUMP(vq);
 	}
+
+	PMD_INIT_LOG(DEBUG, "%d mbufs freed", mbuf_num);
 }
 
 /*
-- 
2.11.0

[PATCH 3/3] net/virtio: fix memory leak when reinitializing device

[Olivier Matz]

Free the previous queues and the attached mbufs before initializing new
ones.

Cc: stable@dpdk.org
Fixes: 60e6f4707ef2 ("net/virtio: reinitialize device when configuring")

Signed-off-by: Zijie Pan <zijie.pan@6wind.com>
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index d8b3b8ed7..977fd61f5 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1379,6 +1379,11 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 	/* Reset the device although not necessary at startup */
 	vtpci_reset(hw);
 
+	if (hw->vqs) {
+		virtio_dev_free_mbufs(eth_dev);
+		virtio_free_queues(hw);
+	}
+
 	/* Tell the host we've noticed this device. */
 	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
 
-- 
2.11.0

Re: [PATCH 2/3] net/virtio: rationalize queue flushing

[Yuanhan Liu]

Hi Oliver,

On Thu, Jan 18, 2018 at 10:07:32AM +0100, Olivier Matz wrote:
> Rationalize the function virtio_dev_free_mbufs():
> 
> - skip NULL vqs instead of crashing: this is required for the
>   next commit
> - use the same kind of loop than in virtio_free_queues()
> - also flush mbufs from the control queue (this is useless yet)

Could we just do "nr_vq = virtio_get_nr_vq(hw) - 1" with a comment that
CQ is excluded, for skipping the CQ?

> - factorize common code between rxq, txq, cq
> 
> Cc: stable@dpdk.org

Could you split the patch two 2:

- one for fixing the crash (skip the NULL vqs). We only need this one
  for stable release.
- another one for the refactoring

Thanks.

	--yliu

Re: [PATCH 1/3] net/virtio: fix typo in function name

[Yuanhan Liu]

On Thu, Jan 18, 2018 at 10:07:31AM +0100, Olivier Matz wrote:
> Fixes: c1f86306a026 ("virtio: add new driver")
> Cc: stable@dpdk.org

I would not suggest to include such patch for a stable release. It doesn't
fix a real issue.

Thanks.

	--yliu
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 4 ++--
>  drivers/net/virtio/virtqueue.c     | 2 +-
>  drivers/net/virtio/virtqueue.h     | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index ec012c2ac..c7426951c 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1879,7 +1879,7 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
>  		VIRTQUEUE_DUMP(rxvq->vq);
>  
>  		PMD_INIT_LOG(DEBUG, "rx_queues[%d]=%p", i, rxvq);
> -		while ((buf = virtqueue_detatch_unused(rxvq->vq)) != NULL) {
> +		while ((buf = virtqueue_detach_unused(rxvq->vq)) != NULL) {
>  			rte_pktmbuf_free(buf);
>  			mbuf_num++;
>  		}
> @@ -1899,7 +1899,7 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
>  		VIRTQUEUE_DUMP(txvq->vq);
>  
>  		mbuf_num = 0;
> -		while ((buf = virtqueue_detatch_unused(txvq->vq)) != NULL) {
> +		while ((buf = virtqueue_detach_unused(txvq->vq)) != NULL) {
>  			rte_pktmbuf_free(buf);
>  			mbuf_num++;
>  		}
> diff --git a/drivers/net/virtio/virtqueue.c b/drivers/net/virtio/virtqueue.c
> index 1ada4fe08..6988bfea4 100644
> --- a/drivers/net/virtio/virtqueue.c
> +++ b/drivers/net/virtio/virtqueue.c
> @@ -16,7 +16,7 @@
>   * 2) mbuf that hasn't been consued by backend.
>   */
>  struct rte_mbuf *
> -virtqueue_detatch_unused(struct virtqueue *vq)
> +virtqueue_detach_unused(struct virtqueue *vq)
>  {
>  	struct rte_mbuf *cookie;
>  	int idx;
> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> index fedbaa39c..1288e5287 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -270,7 +270,7 @@ void virtqueue_dump(struct virtqueue *vq);
>  /**
>   *  Get all mbufs to be freed.
>   */
> -struct rte_mbuf *virtqueue_detatch_unused(struct virtqueue *vq);
> +struct rte_mbuf *virtqueue_detach_unused(struct virtqueue *vq);
>  
>  /* Flush the elements in the used ring. */
>  void virtqueue_rxvq_flush(struct virtqueue *vq);
> -- 
> 2.11.0

Re: [PATCH 1/3] net/virtio: fix typo in function name

[Olivier Matz]

On Thu, Jan 18, 2018 at 09:27:10PM +0800, Yuanhan Liu wrote:
> On Thu, Jan 18, 2018 at 10:07:31AM +0100, Olivier Matz wrote:
> > Fixes: c1f86306a026 ("virtio: add new driver")
> > Cc: stable@dpdk.org
> 
> I would not suggest to include such patch for a stable release. It doesn't
> fix a real issue.

Yes.

I've included it in the patchset to avoid conflicts for backports because it
changes the same code area.

If you prefer, I can send a new patchset with the current 2/3 and 3/3, plus
the typo fix as a individual patch, on top of them.

Does it look good to you?

Thanks


> 
> Thanks.
> 
> 	--yliu
> > 
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > ---
> >  drivers/net/virtio/virtio_ethdev.c | 4 ++--
> >  drivers/net/virtio/virtqueue.c     | 2 +-
> >  drivers/net/virtio/virtqueue.h     | 2 +-
> >  3 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> > index ec012c2ac..c7426951c 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -1879,7 +1879,7 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
> >  		VIRTQUEUE_DUMP(rxvq->vq);
> >  
> >  		PMD_INIT_LOG(DEBUG, "rx_queues[%d]=%p", i, rxvq);
> > -		while ((buf = virtqueue_detatch_unused(rxvq->vq)) != NULL) {
> > +		while ((buf = virtqueue_detach_unused(rxvq->vq)) != NULL) {
> >  			rte_pktmbuf_free(buf);
> >  			mbuf_num++;
> >  		}
> > @@ -1899,7 +1899,7 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
> >  		VIRTQUEUE_DUMP(txvq->vq);
> >  
> >  		mbuf_num = 0;
> > -		while ((buf = virtqueue_detatch_unused(txvq->vq)) != NULL) {
> > +		while ((buf = virtqueue_detach_unused(txvq->vq)) != NULL) {
> >  			rte_pktmbuf_free(buf);
> >  			mbuf_num++;
> >  		}
> > diff --git a/drivers/net/virtio/virtqueue.c b/drivers/net/virtio/virtqueue.c
> > index 1ada4fe08..6988bfea4 100644
> > --- a/drivers/net/virtio/virtqueue.c
> > +++ b/drivers/net/virtio/virtqueue.c
> > @@ -16,7 +16,7 @@
> >   * 2) mbuf that hasn't been consued by backend.
> >   */
> >  struct rte_mbuf *
> > -virtqueue_detatch_unused(struct virtqueue *vq)
> > +virtqueue_detach_unused(struct virtqueue *vq)
> >  {
> >  	struct rte_mbuf *cookie;
> >  	int idx;
> > diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> > index fedbaa39c..1288e5287 100644
> > --- a/drivers/net/virtio/virtqueue.h
> > +++ b/drivers/net/virtio/virtqueue.h
> > @@ -270,7 +270,7 @@ void virtqueue_dump(struct virtqueue *vq);
> >  /**
> >   *  Get all mbufs to be freed.
> >   */
> > -struct rte_mbuf *virtqueue_detatch_unused(struct virtqueue *vq);
> > +struct rte_mbuf *virtqueue_detach_unused(struct virtqueue *vq);
> >  
> >  /* Flush the elements in the used ring. */
> >  void virtqueue_rxvq_flush(struct virtqueue *vq);
> > -- 
> > 2.11.0

Re: [PATCH 2/3] net/virtio: rationalize queue flushing

[Olivier Matz]

On Thu, Jan 18, 2018 at 09:26:09PM +0800, Yuanhan Liu wrote:
> Hi Oliver,
> 
> On Thu, Jan 18, 2018 at 10:07:32AM +0100, Olivier Matz wrote:
> > Rationalize the function virtio_dev_free_mbufs():
> > 
> > - skip NULL vqs instead of crashing: this is required for the
> >   next commit
> > - use the same kind of loop than in virtio_free_queues()
> > - also flush mbufs from the control queue (this is useless yet)
> 
> Could we just do "nr_vq = virtio_get_nr_vq(hw) - 1" with a comment that
> CQ is excluded, for skipping the CQ?

Is "nr_vq = virtio_get_nr_vq(hw) - 1" always valid?
Shouldn't we do this check?
  if (vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_VQ))

Instead, I suggest this:

		queue_type = virtio_get_queue_type(hw, i);
		if (queue_type == VTNET_RQ)
			type = "rxq";
		else if (queue_type == VTNET_TQ)
			type = "txq";
		else
-			type = "cq";
+			continue;


> > - factorize common code between rxq, txq, cq
> > 
> > Cc: stable@dpdk.org
> 
> Could you split the patch two 2:
> 
> - one for fixing the crash (skip the NULL vqs). We only need this one
>   for stable release.
> - another one for the refactoring

Yes, do you want them all in the same patchset?


Thank you for the comments
Olivier

Re: [PATCH 2/3] net/virtio: rationalize queue flushing

[Yuanhan Liu]

On Thu, Jan 18, 2018 at 02:55:44PM +0100, Olivier Matz wrote:
> On Thu, Jan 18, 2018 at 09:26:09PM +0800, Yuanhan Liu wrote:
> > Hi Oliver,
> > 
> > On Thu, Jan 18, 2018 at 10:07:32AM +0100, Olivier Matz wrote:
> > > Rationalize the function virtio_dev_free_mbufs():
> > > 
> > > - skip NULL vqs instead of crashing: this is required for the
> > >   next commit
> > > - use the same kind of loop than in virtio_free_queues()
> > > - also flush mbufs from the control queue (this is useless yet)
> > 
> > Could we just do "nr_vq = virtio_get_nr_vq(hw) - 1" with a comment that
> > CQ is excluded, for skipping the CQ?
> 
> Is "nr_vq = virtio_get_nr_vq(hw) - 1" always valid?
> Shouldn't we do this check?
>   if (vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_VQ))
> 
> Instead, I suggest this:
> 
> 		queue_type = virtio_get_queue_type(hw, i);
> 		if (queue_type == VTNET_RQ)
> 			type = "rxq";
> 		else if (queue_type == VTNET_TQ)
> 			type = "txq";
> 		else
> -			type = "cq";
> +			continue;

Yes, this is better.

> 
> > > - factorize common code between rxq, txq, cq
> > > 
> > > Cc: stable@dpdk.org
> > 
> > Could you split the patch two 2:
> > 
> > - one for fixing the crash (skip the NULL vqs). We only need this one
> >   for stable release.
> > - another one for the refactoring
> 
> Yes, do you want them all in the same patchset?

I think it's okay.

Thanks.

	--yliu

Re: [PATCH 2/3] net/virtio: rationalize queue flushing

[Tiwei Bie]

On Thu, Jan 18, 2018 at 10:07:32AM +0100, Olivier Matz wrote:
> Rationalize the function virtio_dev_free_mbufs():
> 
> - skip NULL vqs instead of crashing: this is required for the
>   next commit
> - use the same kind of loop than in virtio_free_queues()
> - also flush mbufs from the control queue (this is useless yet)
> - factorize common code between rxq, txq, cq
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 55 ++++++++++++++++++--------------------
>  1 file changed, 26 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index c7426951c..d8b3b8ed7 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1868,47 +1868,44 @@ virtio_dev_start(struct rte_eth_dev *dev)
>  
>  static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
>  {
[...]
>  
> -		mbuf_num = 0;
> -		while ((buf = virtqueue_detach_unused(txvq->vq)) != NULL) {
> +		while ((buf = virtqueue_detach_unused(vq)) != NULL) {

Thanks for working on this! The virtqueue_detach_unused() can't
handle the vector Rx case correctly. Because vq->vq_descx[] is
initialized for vector Rx, but isn't updated by the vector Rx.
So together with the next commit, it may cause problems during
dev_stop/dev_configure/dev_start if vector Rx is used.

Thanks,
Tiwei

>  			rte_pktmbuf_free(buf);
>  			mbuf_num++;
>  		}
>  
> -		PMD_INIT_LOG(DEBUG, "free %d mbufs", mbuf_num);
>  		PMD_INIT_LOG(DEBUG,
> -			     "After freeing txq[%d] used and unused buf", i);
> -		VIRTQUEUE_DUMP(txvq->vq);
> +			"After freeing %s[%d] used and unused buf",
> +			type, i);
> +		VIRTQUEUE_DUMP(vq);
>  	}
> +
> +	PMD_INIT_LOG(DEBUG, "%d mbufs freed", mbuf_num);
>  }
>  
>  /*
> -- 
> 2.11.0
> 

Re: [PATCH 1/3] net/virtio: fix typo in function name

[Yuanhan Liu]

On Thu, Jan 18, 2018 at 02:45:48PM +0100, Olivier Matz wrote:
> On Thu, Jan 18, 2018 at 09:27:10PM +0800, Yuanhan Liu wrote:
> > On Thu, Jan 18, 2018 at 10:07:31AM +0100, Olivier Matz wrote:
> > > Fixes: c1f86306a026 ("virtio: add new driver")
> > > Cc: stable@dpdk.org
> > 
> > I would not suggest to include such patch for a stable release. It doesn't
> > fix a real issue.
> 
> Yes.
> 
> I've included it in the patchset to avoid conflicts for backports because it
> changes the same code area.
> 
> If you prefer, I can send a new patchset with the current 2/3 and 3/3, plus
> the typo fix as a individual patch, on top of them.
> 
> Does it look good to you?

Yes, that's the reason I personaly always put the bug fix patches in
head of a patchset, for avoiding conflicts like this.

Thanks.

	--yliu

Re: [PATCH 2/3] net/virtio: rationalize queue flushing

[Olivier Matz]

On Thu, Jan 18, 2018 at 10:05:49PM +0800, Tiwei Bie wrote:
> On Thu, Jan 18, 2018 at 10:07:32AM +0100, Olivier Matz wrote:
> > Rationalize the function virtio_dev_free_mbufs():
> > 
> > - skip NULL vqs instead of crashing: this is required for the
> >   next commit
> > - use the same kind of loop than in virtio_free_queues()
> > - also flush mbufs from the control queue (this is useless yet)
> > - factorize common code between rxq, txq, cq
> > 
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > ---
> >  drivers/net/virtio/virtio_ethdev.c | 55 ++++++++++++++++++--------------------
> >  1 file changed, 26 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> > index c7426951c..d8b3b8ed7 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -1868,47 +1868,44 @@ virtio_dev_start(struct rte_eth_dev *dev)
> >  
> >  static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
> >  {
> [...]
> >  
> > -		mbuf_num = 0;
> > -		while ((buf = virtqueue_detach_unused(txvq->vq)) != NULL) {
> > +		while ((buf = virtqueue_detach_unused(vq)) != NULL) {
> 
> Thanks for working on this! The virtqueue_detach_unused() can't
> handle the vector Rx case correctly. Because vq->vq_descx[] is
> initialized for vector Rx, but isn't updated by the vector Rx.
> So together with the next commit, it may cause problems during
> dev_stop/dev_configure/dev_start if vector Rx is used.

What would be the appropriate way to fix it?

Either we fix the vector functions to set vq->vq_descx, or we find
another method to flush the mbufs in case of vector rx. Can we use
vq->sw_ring[] to get the mbufs instead?

Thanks

Re: [PATCH 2/3] net/virtio: rationalize queue flushing

[Tiwei Bie]

On Thu, Jan 18, 2018 at 03:55:53PM +0100, Olivier Matz wrote:
> On Thu, Jan 18, 2018 at 10:05:49PM +0800, Tiwei Bie wrote:
> > On Thu, Jan 18, 2018 at 10:07:32AM +0100, Olivier Matz wrote:
> > > Rationalize the function virtio_dev_free_mbufs():
> > > 
> > > - skip NULL vqs instead of crashing: this is required for the
> > >   next commit
> > > - use the same kind of loop than in virtio_free_queues()
> > > - also flush mbufs from the control queue (this is useless yet)
> > > - factorize common code between rxq, txq, cq
> > > 
> > > Cc: stable@dpdk.org
> > > 
> > > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > > ---
> > >  drivers/net/virtio/virtio_ethdev.c | 55 ++++++++++++++++++--------------------
> > >  1 file changed, 26 insertions(+), 29 deletions(-)
> > > 
> > > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> > > index c7426951c..d8b3b8ed7 100644
> > > --- a/drivers/net/virtio/virtio_ethdev.c
> > > +++ b/drivers/net/virtio/virtio_ethdev.c
> > > @@ -1868,47 +1868,44 @@ virtio_dev_start(struct rte_eth_dev *dev)
> > >  
> > >  static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
> > >  {
> > [...]
> > >  
> > > -		mbuf_num = 0;
> > > -		while ((buf = virtqueue_detach_unused(txvq->vq)) != NULL) {
> > > +		while ((buf = virtqueue_detach_unused(vq)) != NULL) {
> > 
> > Thanks for working on this! The virtqueue_detach_unused() can't
> > handle the vector Rx case correctly. Because vq->vq_descx[] is
> > initialized for vector Rx, but isn't updated by the vector Rx.
> > So together with the next commit, it may cause problems during
> > dev_stop/dev_configure/dev_start if vector Rx is used.
> 
> What would be the appropriate way to fix it?
> 
> Either we fix the vector functions to set vq->vq_descx, or we find
> another method to flush the mbufs in case of vector rx. Can we use
> vq->sw_ring[] to get the mbufs instead?

The first one may hurt the performance. If not, it would be the
best choice IMO. For the second one and your question, there is
some related code in virtqueue_rxvq_flush() you could refer to:

https://github.com/DPDK/dpdk/blob/e00093f381a1/drivers/net/virtio/virtqueue.c#L51

Thanks,
Tiwei

Re: [PATCH 2/3] net/virtio: rationalize queue flushing

[Olivier Matz]

On Thu, Jan 18, 2018 at 11:48:09PM +0800, Tiwei Bie wrote:
> On Thu, Jan 18, 2018 at 03:55:53PM +0100, Olivier Matz wrote:
> > On Thu, Jan 18, 2018 at 10:05:49PM +0800, Tiwei Bie wrote:
> > > On Thu, Jan 18, 2018 at 10:07:32AM +0100, Olivier Matz wrote:
> > > > Rationalize the function virtio_dev_free_mbufs():
> > > > 
> > > > - skip NULL vqs instead of crashing: this is required for the
> > > >   next commit
> > > > - use the same kind of loop than in virtio_free_queues()
> > > > - also flush mbufs from the control queue (this is useless yet)
> > > > - factorize common code between rxq, txq, cq
> > > > 
> > > > Cc: stable@dpdk.org
> > > > 
> > > > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > > > ---
> > > >  drivers/net/virtio/virtio_ethdev.c | 55 ++++++++++++++++++--------------------
> > > >  1 file changed, 26 insertions(+), 29 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> > > > index c7426951c..d8b3b8ed7 100644
> > > > --- a/drivers/net/virtio/virtio_ethdev.c
> > > > +++ b/drivers/net/virtio/virtio_ethdev.c
> > > > @@ -1868,47 +1868,44 @@ virtio_dev_start(struct rte_eth_dev *dev)
> > > >  
> > > >  static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
> > > >  {
> > > [...]
> > > >  
> > > > -		mbuf_num = 0;
> > > > -		while ((buf = virtqueue_detach_unused(txvq->vq)) != NULL) {
> > > > +		while ((buf = virtqueue_detach_unused(vq)) != NULL) {
> > > 
> > > Thanks for working on this! The virtqueue_detach_unused() can't
> > > handle the vector Rx case correctly. Because vq->vq_descx[] is
> > > initialized for vector Rx, but isn't updated by the vector Rx.
> > > So together with the next commit, it may cause problems during
> > > dev_stop/dev_configure/dev_start if vector Rx is used.
> > 
> > What would be the appropriate way to fix it?
> > 
> > Either we fix the vector functions to set vq->vq_descx, or we find
> > another method to flush the mbufs in case of vector rx. Can we use
> > vq->sw_ring[] to get the mbufs instead?
> 
> The first one may hurt the performance. If not, it would be the
> best choice IMO. For the second one and your question, there is
> some related code in virtqueue_rxvq_flush() you could refer to:
> 
> https://github.com/DPDK/dpdk/blob/e00093f381a1/drivers/net/virtio/virtqueue.c#L51

Yes, I've just seen it too :)
I'll give it a try.

[PATCH v2 0/4] net/virtio: fix memory leak when reinitializing device

[Olivier Matz]

When devops->configure() is called, the queues may be reallocated
if the features flag changed, but the previous are not freed. This
patchset fixes this issue.

To really point out the issue, I instrumented rte_malloc,
rte_memzone_reserve, rte_mbuf_alloc to track the allocations and frees.
For reference, the patch is available here:
https://www.droids-corp.org/~zer0/hidden/0001-debug-malloc.patch
Iit conflicts a bit with the patchset, but it is quite easy to solve.

To reproduce the issue:

  cd dpdk
  make config T=x86_64-native-linuxapp-gcc
  sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=y,' build/.config
  make -j4
  mkdir -p /mnt/huge
  mount -t hugetlbfs nodev /mnt/huge
  echo 256 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
  modprobe uio_pci_generic
  python usertools/dpdk-devbind.py -b uio_pci_generic 0000:00:02.0
  ./build/app/testpmd -l 0,1 --log-level 8 -- --total-num-mbufs=16384 \
     -i --port-topology=chained --disable-hw-vlan-filter \
     --disable-hw-vlan-strip --enable-rx-cksum --enable-lro \
     --txqflags=0 --no-lsc-interrupt

  # testpmd commands
  port stop 0
  port config all rx-cksum off
  port start 0
  quit

Without the patchset, the debug patch displays a leak of mbufs allocated
from virtio_dev_rx_queue_setup_finish() and a leak of rte_mallocs allocated
from virtio_init_queue().

After the patchset, only one malloc is remaining after the device close,
but this is normal, because this area is freed in dev_uninit().

v1 -> v2:
- move the vq != NULL check from the refactor patch to the memory
  leak fix patch (Yuanhan)
- new patch to fix queue flushing with vector Rx enabled (Tiwei)
- move the backportable fixes at the beginning of the patchset (Yuanhan)
- remove Cc stable on the typo fix (Yuanhan)
  note: I finally kept it in the patchset


Olivier Matz (4):
  net/virtio: fix queue flushing with vector Rx enabled
  net/virtio: fix memory leak when reinitializing device
  net/virtio: rationalize queue flushing
  net/virtio: fix typo in function name

 drivers/net/virtio/virtio_ethdev.c | 71 +++++++++++++++++---------------------
 drivers/net/virtio/virtqueue.c     | 19 ++++++++--
 drivers/net/virtio/virtqueue.h     | 13 ++++++-
 3 files changed, 59 insertions(+), 44 deletions(-)

-- 
2.11.0

[PATCH v2 1/4] net/virtio: fix queue flushing with vector Rx enabled

[Olivier Matz]

When using vector Rx mode (use_simple_rx = 1), vq->vq_descx[] is not
kept up to date. To properly detach the mbufs in this case, browse
sw_ring[] instead, as it's done in virtqueue_rxvq_flush().

Since we need virtio_get_queue_type(), also move this function in
virtqueue.h as a static inline.

Fixes: fc3d66212fed ("virtio: add vector Rx")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---

Tiwei,

While it passes my test plan, please carefully check that what I did in
virtqueue_detatch_unused() is correct. I tried to reproduce what is done
in virtqueue_rxvq_flush(), but I may be mistaking due to the different
ring layout assumption and mbuf management between standard and vector.

Thanks
Olivier


 drivers/net/virtio/virtio_ethdev.c | 11 -----------
 drivers/net/virtio/virtqueue.c     | 17 +++++++++++++++--
 drivers/net/virtio/virtqueue.h     | 11 +++++++++++
 3 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index ec012c2ac..2c7f902e7 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -268,17 +268,6 @@ virtio_dev_queue_release(void *queue __rte_unused)
 	/* do nothing */
 }
 
-static int
-virtio_get_queue_type(struct virtio_hw *hw, uint16_t vtpci_queue_idx)
-{
-	if (vtpci_queue_idx == hw->max_queue_pairs * 2)
-		return VTNET_CQ;
-	else if (vtpci_queue_idx % 2 == 0)
-		return VTNET_RQ;
-	else
-		return VTNET_TQ;
-}
-
 static uint16_t
 virtio_get_nr_vq(struct virtio_hw *hw)
 {
diff --git a/drivers/net/virtio/virtqueue.c b/drivers/net/virtio/virtqueue.c
index 1ada4fe08..43cb51d1b 100644
--- a/drivers/net/virtio/virtqueue.c
+++ b/drivers/net/virtio/virtqueue.c
@@ -18,17 +18,30 @@
 struct rte_mbuf *
 virtqueue_detatch_unused(struct virtqueue *vq)
 {
+	struct virtio_hw *hw = vq->hw;
 	struct rte_mbuf *cookie;
 	int idx;
+	int type = virtio_get_queue_type(hw, vq->vq_queue_index);
+
+	if (vq == NULL)
+		return NULL;
 
-	if (vq != NULL)
-		for (idx = 0; idx < vq->vq_nentries; idx++) {
+	for (idx = 0; idx < vq->vq_nentries; idx++) {
+		if (hw->use_simple_rx && type == VTNET_RQ) {
+			cookie = vq->sw_ring[idx];
+			if (cookie != NULL) {
+				vq->sw_ring[idx] = NULL;
+				return cookie;
+			}
+		} else {
 			cookie = vq->vq_descx[idx].cookie;
 			if (cookie != NULL) {
 				vq->vq_descx[idx].cookie = NULL;
 				return cookie;
 			}
 		}
+	}
+
 	return NULL;
 }
 
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index fedbaa39c..cae1b318a 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -281,6 +281,17 @@ virtqueue_full(const struct virtqueue *vq)
 	return vq->vq_free_cnt == 0;
 }
 
+static inline int
+virtio_get_queue_type(struct virtio_hw *hw, uint16_t vtpci_queue_idx)
+{
+	if (vtpci_queue_idx == hw->max_queue_pairs * 2)
+		return VTNET_CQ;
+	else if (vtpci_queue_idx % 2 == 0)
+		return VTNET_RQ;
+	else
+		return VTNET_TQ;
+}
+
 #define VIRTQUEUE_NUSED(vq) ((uint16_t)((vq)->vq_ring.used->idx - (vq)->vq_used_cons_idx))
 
 void vq_ring_free_chain(struct virtqueue *vq, uint16_t desc_idx);
-- 
2.11.0

[PATCH v2 2/4] net/virtio: fix memory leak when reinitializing device

[Olivier Matz]

Free the previous queues and the attached mbufs before initializing new
ones.

The function virtio_dev_free_mbufs() is now called when reconfiguring the
device, so we also need to add a check to ensure that it won't crash for
uninitialized queues.

Cc: stable@dpdk.org
Fixes: 60e6f4707ef2 ("net/virtio: reinitialize device when configuring")

Signed-off-by: Zijie Pan <zijie.pan@6wind.com>
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 2c7f902e7..a8adf0037 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1368,6 +1368,11 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 	/* Reset the device although not necessary at startup */
 	vtpci_reset(hw);
 
+	if (hw->vqs) {
+		virtio_dev_free_mbufs(eth_dev);
+		virtio_free_queues(hw);
+	}
+
 	/* Tell the host we've noticed this device. */
 	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
 
@@ -1863,6 +1868,9 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		struct virtnet_rx *rxvq = dev->data->rx_queues[i];
 
+		if (rxvq == NULL || rxvq->vq == NULL)
+			continue;
+
 		PMD_INIT_LOG(DEBUG,
 			     "Before freeing rxq[%d] used and unused buf", i);
 		VIRTQUEUE_DUMP(rxvq->vq);
@@ -1882,6 +1890,9 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
 	for (i = 0; i < dev->data->nb_tx_queues; i++) {
 		struct virtnet_tx *txvq = dev->data->tx_queues[i];
 
+		if (txvq == NULL || txvq->vq == NULL)
+			continue;
+
 		PMD_INIT_LOG(DEBUG,
 			     "Before freeing txq[%d] used and unused bufs",
 			     i);
-- 
2.11.0

[PATCH v2 3/4] net/virtio: rationalize queue flushing

[Olivier Matz]

Use the same kind of loop than in virtio_free_queues() and factorize
common code.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 57 ++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 33 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index a8adf0037..b8a5f90c7 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1862,53 +1862,44 @@ virtio_dev_start(struct rte_eth_dev *dev)
 
 static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
 {
+	struct virtio_hw *hw = dev->data->dev_private;
+	uint16_t nr_vq = virtio_get_nr_vq(hw);
+	const char *type __rte_unused;
+	unsigned int i, mbuf_num = 0;
+	struct virtqueue *vq;
 	struct rte_mbuf *buf;
-	int i, mbuf_num = 0;
-
-	for (i = 0; i < dev->data->nb_rx_queues; i++) {
-		struct virtnet_rx *rxvq = dev->data->rx_queues[i];
+	int queue_type;
 
-		if (rxvq == NULL || rxvq->vq == NULL)
+	for (i = 0; i < nr_vq; i++) {
+		vq = hw->vqs[i];
+		if (!vq)
 			continue;
 
-		PMD_INIT_LOG(DEBUG,
-			     "Before freeing rxq[%d] used and unused buf", i);
-		VIRTQUEUE_DUMP(rxvq->vq);
-
-		PMD_INIT_LOG(DEBUG, "rx_queues[%d]=%p", i, rxvq);
-		while ((buf = virtqueue_detatch_unused(rxvq->vq)) != NULL) {
-			rte_pktmbuf_free(buf);
-			mbuf_num++;
-		}
-
-		PMD_INIT_LOG(DEBUG, "free %d mbufs", mbuf_num);
-		PMD_INIT_LOG(DEBUG,
-			     "After freeing rxq[%d] used and unused buf", i);
-		VIRTQUEUE_DUMP(rxvq->vq);
-	}
-
-	for (i = 0; i < dev->data->nb_tx_queues; i++) {
-		struct virtnet_tx *txvq = dev->data->tx_queues[i];
-
-		if (txvq == NULL || txvq->vq == NULL)
+		queue_type = virtio_get_queue_type(hw, i);
+		if (queue_type == VTNET_RQ)
+			type = "rxq";
+		else if (queue_type == VTNET_TQ)
+			type = "txq";
+		else
 			continue;
 
 		PMD_INIT_LOG(DEBUG,
-			     "Before freeing txq[%d] used and unused bufs",
-			     i);
-		VIRTQUEUE_DUMP(txvq->vq);
+			"Before freeing %s[%d] used and unused buf",
+			type, i);
+		VIRTQUEUE_DUMP(vq);
 
-		mbuf_num = 0;
-		while ((buf = virtqueue_detatch_unused(txvq->vq)) != NULL) {
+		while ((buf = virtqueue_detatch_unused(vq)) != NULL) {
 			rte_pktmbuf_free(buf);
 			mbuf_num++;
 		}
 
-		PMD_INIT_LOG(DEBUG, "free %d mbufs", mbuf_num);
 		PMD_INIT_LOG(DEBUG,
-			     "After freeing txq[%d] used and unused buf", i);
-		VIRTQUEUE_DUMP(txvq->vq);
+			"After freeing %s[%d] used and unused buf",
+			type, i);
+		VIRTQUEUE_DUMP(vq);
 	}
+
+	PMD_INIT_LOG(DEBUG, "%d mbufs freed", mbuf_num);
 }
 
 /*
-- 
2.11.0

[PATCH v2 4/4] net/virtio: fix typo in function name

[Olivier Matz]

Fixes: c1f86306a026 ("virtio: add new driver")

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 2 +-
 drivers/net/virtio/virtqueue.c     | 2 +-
 drivers/net/virtio/virtqueue.h     | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index b8a5f90c7..d196afad3 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1888,7 +1888,7 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
 			type, i);
 		VIRTQUEUE_DUMP(vq);
 
-		while ((buf = virtqueue_detatch_unused(vq)) != NULL) {
+		while ((buf = virtqueue_detach_unused(vq)) != NULL) {
 			rte_pktmbuf_free(buf);
 			mbuf_num++;
 		}
diff --git a/drivers/net/virtio/virtqueue.c b/drivers/net/virtio/virtqueue.c
index 43cb51d1b..8682455b1 100644
--- a/drivers/net/virtio/virtqueue.c
+++ b/drivers/net/virtio/virtqueue.c
@@ -16,7 +16,7 @@
  * 2) mbuf that hasn't been consued by backend.
  */
 struct rte_mbuf *
-virtqueue_detatch_unused(struct virtqueue *vq)
+virtqueue_detach_unused(struct virtqueue *vq)
 {
 	struct virtio_hw *hw = vq->hw;
 	struct rte_mbuf *cookie;
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index cae1b318a..35145e7b2 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -270,7 +270,7 @@ void virtqueue_dump(struct virtqueue *vq);
 /**
  *  Get all mbufs to be freed.
  */
-struct rte_mbuf *virtqueue_detatch_unused(struct virtqueue *vq);
+struct rte_mbuf *virtqueue_detach_unused(struct virtqueue *vq);
 
 /* Flush the elements in the used ring. */
 void virtqueue_rxvq_flush(struct virtqueue *vq);
-- 
2.11.0

Re: [PATCH v2 1/4] net/virtio: fix queue flushing with vector Rx enabled

[Tiwei Bie]

On Fri, Jan 19, 2018 at 04:55:53PM +0100, Olivier Matz wrote:
> When using vector Rx mode (use_simple_rx = 1), vq->vq_descx[] is not
> kept up to date. To properly detach the mbufs in this case, browse
> sw_ring[] instead, as it's done in virtqueue_rxvq_flush().
> 
> Since we need virtio_get_queue_type(), also move this function in
> virtqueue.h as a static inline.
> 
> Fixes: fc3d66212fed ("virtio: add vector Rx")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
> 
> Tiwei,
> 
> While it passes my test plan, please carefully check that what I did in
> virtqueue_detatch_unused() is correct. I tried to reproduce what is done
> in virtqueue_rxvq_flush(), but I may be mistaking due to the different
> ring layout assumption and mbuf management between standard and vector.
> 
> Thanks
> Olivier
> 
> 
>  drivers/net/virtio/virtio_ethdev.c | 11 -----------
>  drivers/net/virtio/virtqueue.c     | 17 +++++++++++++++--
>  drivers/net/virtio/virtqueue.h     | 11 +++++++++++
>  3 files changed, 26 insertions(+), 13 deletions(-)
> 
[...]
>  struct rte_mbuf *
>  virtqueue_detatch_unused(struct virtqueue *vq)
>  {
> +	struct virtio_hw *hw = vq->hw;
>  	struct rte_mbuf *cookie;
>  	int idx;
> +	int type = virtio_get_queue_type(hw, vq->vq_queue_index);
> +
> +	if (vq == NULL)
> +		return NULL;
>  
> -	if (vq != NULL)
> -		for (idx = 0; idx < vq->vq_nentries; idx++) {
> +	for (idx = 0; idx < vq->vq_nentries; idx++) {
> +		if (hw->use_simple_rx && type == VTNET_RQ) {
> +			cookie = vq->sw_ring[idx];
> +			if (cookie != NULL) {
> +				vq->sw_ring[idx] = NULL;

Thanks for working on this!

The vq->sw_ring[idx] isn't zeroed during Rx. So besides the
check of (cookie != NULL), some other check is also needed
to avoid freeing the mbufs already delivered to application.

The mbufs in below interval belong to application:

start: sw_ring[vq->vq_avail_idx & (vq->vq_nentries - 1)] (included)
end: sw_ring[(vq->vq_avail_idx + vq->vq_free_cnt) & (vq->vq_nentries - 1)] (excluded)

PS. (vq->vq_avail_idx & (vq->vq_nentries - 1)) can be greater than
    (vq->vq_avail_idx + vq->vq_free_cnt) & (vq->vq_nentries - 1).

Thanks,
Tiwei

Re: [PATCH v2 1/4] net/virtio: fix queue flushing with vector Rx enabled

[Olivier Matz]

On Mon, Jan 22, 2018 at 10:56:41AM +0800, Tiwei Bie wrote:
> On Fri, Jan 19, 2018 at 04:55:53PM +0100, Olivier Matz wrote:
> > When using vector Rx mode (use_simple_rx = 1), vq->vq_descx[] is not
> > kept up to date. To properly detach the mbufs in this case, browse
> > sw_ring[] instead, as it's done in virtqueue_rxvq_flush().
> > 
> > Since we need virtio_get_queue_type(), also move this function in
> > virtqueue.h as a static inline.
> > 
> > Fixes: fc3d66212fed ("virtio: add vector Rx")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > ---
> > 
> > Tiwei,
> > 
> > While it passes my test plan, please carefully check that what I did in
> > virtqueue_detatch_unused() is correct. I tried to reproduce what is done
> > in virtqueue_rxvq_flush(), but I may be mistaking due to the different
> > ring layout assumption and mbuf management between standard and vector.
> > 
> > Thanks
> > Olivier
> > 
> > 
> >  drivers/net/virtio/virtio_ethdev.c | 11 -----------
> >  drivers/net/virtio/virtqueue.c     | 17 +++++++++++++++--
> >  drivers/net/virtio/virtqueue.h     | 11 +++++++++++
> >  3 files changed, 26 insertions(+), 13 deletions(-)
> > 
> [...]
> >  struct rte_mbuf *
> >  virtqueue_detatch_unused(struct virtqueue *vq)
> >  {
> > +	struct virtio_hw *hw = vq->hw;
> >  	struct rte_mbuf *cookie;
> >  	int idx;
> > +	int type = virtio_get_queue_type(hw, vq->vq_queue_index);
> > +
> > +	if (vq == NULL)
> > +		return NULL;
> >  
> > -	if (vq != NULL)
> > -		for (idx = 0; idx < vq->vq_nentries; idx++) {
> > +	for (idx = 0; idx < vq->vq_nentries; idx++) {
> > +		if (hw->use_simple_rx && type == VTNET_RQ) {
> > +			cookie = vq->sw_ring[idx];
> > +			if (cookie != NULL) {
> > +				vq->sw_ring[idx] = NULL;
> 
> Thanks for working on this!
> 
> The vq->sw_ring[idx] isn't zeroed during Rx. So besides the
> check of (cookie != NULL), some other check is also needed
> to avoid freeing the mbufs already delivered to application.
> 
> The mbufs in below interval belong to application:
> 
> start: sw_ring[vq->vq_avail_idx & (vq->vq_nentries - 1)] (included)
> end: sw_ring[(vq->vq_avail_idx + vq->vq_free_cnt) & (vq->vq_nentries - 1)] (excluded)
> 
> PS. (vq->vq_avail_idx & (vq->vq_nentries - 1)) can be greater than
>     (vq->vq_avail_idx + vq->vq_free_cnt) & (vq->vq_nentries - 1).

Thank you for the review. What about something like this?

struct rte_mbuf *
virtqueue_detach_unused(struct virtqueue *vq)
{
	struct rte_mbuf *cookie;
	struct virtio_hw *hw;
	uint16_t start, end;
	int type, idx;

	if (vq == NULL)
		return NULL;

	hw = vq->hw;
	type = virtio_get_queue_type(hw, vq->vq_queue_index);
	start = vq->vq_avail_idx & (vq->vq_nentries - 1);
	end = (vq->vq_avail_idx + vq->vq_free_cnt) & (vq->vq_nentries - 1);
	end = (vq->vq_avail_idx + vq->vq_free_cnt) & (vq->vq_nentries - 1);

	for (idx = 0; idx < vq->vq_nentries; idx++) {
		if (hw->use_simple_rx && type == VTNET_RQ) {
			if (start <= end && idx >= start && idx < end)
				continue;
			if (start > end && (idx >= start || idx < end))
				continue;
			cookie = vq->sw_ring[idx];
			if (cookie == NULL)
				continue;
			vq->sw_ring[idx] = NULL;
			return cookie;
		} else {
			cookie = vq->vq_descx[idx].cookie;
			if (cookie != NULL) {
				vq->vq_descx[idx].cookie = NULL;
				return cookie;
			}
		}
	}

	return NULL;
}


Regards,
Olivier

Re: [PATCH v2 1/4] net/virtio: fix queue flushing with vector Rx enabled

[Tiwei Bie]

On Mon, Jan 22, 2018 at 11:38:58AM +0100, Olivier Matz wrote:
> On Mon, Jan 22, 2018 at 10:56:41AM +0800, Tiwei Bie wrote:
> > On Fri, Jan 19, 2018 at 04:55:53PM +0100, Olivier Matz wrote:
> > > When using vector Rx mode (use_simple_rx = 1), vq->vq_descx[] is not
> > > kept up to date. To properly detach the mbufs in this case, browse
> > > sw_ring[] instead, as it's done in virtqueue_rxvq_flush().
> > > 
> > > Since we need virtio_get_queue_type(), also move this function in
> > > virtqueue.h as a static inline.
> > > 
> > > Fixes: fc3d66212fed ("virtio: add vector Rx")
> > > Cc: stable@dpdk.org
> > > 
> > > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > > ---
> > > 
> > > Tiwei,
> > > 
> > > While it passes my test plan, please carefully check that what I did in
> > > virtqueue_detatch_unused() is correct. I tried to reproduce what is done
> > > in virtqueue_rxvq_flush(), but I may be mistaking due to the different
> > > ring layout assumption and mbuf management between standard and vector.
> > > 
> > > Thanks
> > > Olivier
> > > 
> > > 
> > >  drivers/net/virtio/virtio_ethdev.c | 11 -----------
> > >  drivers/net/virtio/virtqueue.c     | 17 +++++++++++++++--
> > >  drivers/net/virtio/virtqueue.h     | 11 +++++++++++
> > >  3 files changed, 26 insertions(+), 13 deletions(-)
> > > 
> > [...]
> > >  struct rte_mbuf *
> > >  virtqueue_detatch_unused(struct virtqueue *vq)
> > >  {
> > > +	struct virtio_hw *hw = vq->hw;
> > >  	struct rte_mbuf *cookie;
> > >  	int idx;
> > > +	int type = virtio_get_queue_type(hw, vq->vq_queue_index);
> > > +
> > > +	if (vq == NULL)
> > > +		return NULL;
> > >  
> > > -	if (vq != NULL)
> > > -		for (idx = 0; idx < vq->vq_nentries; idx++) {
> > > +	for (idx = 0; idx < vq->vq_nentries; idx++) {
> > > +		if (hw->use_simple_rx && type == VTNET_RQ) {
> > > +			cookie = vq->sw_ring[idx];
> > > +			if (cookie != NULL) {
> > > +				vq->sw_ring[idx] = NULL;
> > 
> > Thanks for working on this!
> > 
> > The vq->sw_ring[idx] isn't zeroed during Rx. So besides the
> > check of (cookie != NULL), some other check is also needed
> > to avoid freeing the mbufs already delivered to application.
> > 
> > The mbufs in below interval belong to application:
> > 
> > start: sw_ring[vq->vq_avail_idx & (vq->vq_nentries - 1)] (included)
> > end: sw_ring[(vq->vq_avail_idx + vq->vq_free_cnt) & (vq->vq_nentries - 1)] (excluded)
> > 
> > PS. (vq->vq_avail_idx & (vq->vq_nentries - 1)) can be greater than
> >     (vq->vq_avail_idx + vq->vq_free_cnt) & (vq->vq_nentries - 1).
> 
> Thank you for the review. What about something like this?
> 
> struct rte_mbuf *
> virtqueue_detach_unused(struct virtqueue *vq)
> {
> 	struct rte_mbuf *cookie;
> 	struct virtio_hw *hw;
> 	uint16_t start, end;
> 	int type, idx;
> 
> 	if (vq == NULL)
> 		return NULL;
> 
> 	hw = vq->hw;
> 	type = virtio_get_queue_type(hw, vq->vq_queue_index);
> 	start = vq->vq_avail_idx & (vq->vq_nentries - 1);
> 	end = (vq->vq_avail_idx + vq->vq_free_cnt) & (vq->vq_nentries - 1);
> 	end = (vq->vq_avail_idx + vq->vq_free_cnt) & (vq->vq_nentries - 1);
> 
> 	for (idx = 0; idx < vq->vq_nentries; idx++) {
> 		if (hw->use_simple_rx && type == VTNET_RQ) {
> 			if (start <= end && idx >= start && idx < end)
> 				continue;
> 			if (start > end && (idx >= start || idx < end))
> 				continue;
> 			cookie = vq->sw_ring[idx];
> 			if (cookie == NULL)
> 				continue;
> 			vq->sw_ring[idx] = NULL;
> 			return cookie;
> 		} else {
> 			cookie = vq->vq_descx[idx].cookie;
> 			if (cookie != NULL) {
> 				vq->vq_descx[idx].cookie = NULL;
> 				return cookie;
> 			}
> 		}
> 	}
> 
> 	return NULL;
> }

Apart from the repeated line, it looks good to me. Thanks!

Thanks,
Tiwei

[PATCH v3 0/4] net/virtio: fix memory leak when reinitializing device

[Olivier Matz]

When devops->configure() is called, the queues may be reallocated
if the features flag changed, but the previous are not freed. This
patchset fixes this issue.

To really point out the issue, I instrumented rte_malloc,
rte_memzone_reserve, rte_mbuf_alloc to track the allocations and frees.
For reference, the patch is available here:
https://www.droids-corp.org/~zer0/hidden/0001-debug-malloc.patch
Iit conflicts a bit with the patchset, but it is quite easy to solve.

To reproduce the issue:

  cd dpdk
  make config T=x86_64-native-linuxapp-gcc
  sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=y,' build/.config
  make -j4
  mkdir -p /mnt/huge
  mount -t hugetlbfs nodev /mnt/huge
  echo 256 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
  modprobe uio_pci_generic
  python usertools/dpdk-devbind.py -b uio_pci_generic 0000:00:02.0
  ./build/app/testpmd -l 0,1 --log-level 8 -- --total-num-mbufs=16384 \
     -i --port-topology=chained --disable-hw-vlan-filter \
     --disable-hw-vlan-strip --enable-rx-cksum --enable-lro \
     --txqflags=0 --no-lsc-interrupt

  # testpmd commands
  port stop 0
  port config all rx-cksum off
  port start 0
  quit

Without the patchset, the debug patch displays a leak of mbufs allocated
from virtio_dev_rx_queue_setup_finish() and a leak of rte_mallocs allocated
from virtio_init_queue().

After the patchset, only one malloc is remaining after the device close,
but this is normal, because this area is freed in dev_uninit().

v2 -> v3:
- fix queue flushing with vector Rx enabled (Tiwei).
  The patch is slightly changed compared to what was proposed by mail,
  due to a checkpatch warning.

v1 -> v2:
- move the vq != NULL check from the refactor patch to the memory
  leak fix patch (Yuanhan)
- new patch to fix queue flushing with vector Rx enabled (Tiwei)
- move the backportable fixes at the beginning of the patchset (Yuanhan)
- remove Cc stable on the typo fix (Yuanhan)
  note: I finally kept it in the patchset


Olivier Matz (4):
  net/virtio: fix queue flushing with vector Rx enabled
  net/virtio: fix memory leak when reinitializing device
  net/virtio: rationalize queue flushing
  net/virtio: fix typo in function name

 drivers/net/virtio/virtio_ethdev.c | 71 +++++++++++++++++---------------------
 drivers/net/virtio/virtqueue.c     | 30 +++++++++++++---
 drivers/net/virtio/virtqueue.h     | 13 ++++++-
 3 files changed, 69 insertions(+), 45 deletions(-)

-- 
2.11.0

[PATCH v3 1/4] net/virtio: fix queue flushing with vector Rx enabled

[Olivier Matz]

When using vector Rx mode (use_simple_rx = 1), vq->vq_descx[] is not
kept up to date. To properly detach the mbufs in this case, browse
sw_ring[] instead, as it's done in virtqueue_rxvq_flush().

Since we need virtio_get_queue_type(), also move this function in
virtqueue.h as a static inline.

Fixes: fc3d66212fed ("virtio: add vector Rx")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 11 -----------
 drivers/net/virtio/virtqueue.c     | 28 +++++++++++++++++++++++++---
 drivers/net/virtio/virtqueue.h     | 11 +++++++++++
 3 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 17ac04931..24ffbeeb2 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -279,17 +279,6 @@ virtio_dev_queue_release(void *queue __rte_unused)
 	/* do nothing */
 }
 
-static int
-virtio_get_queue_type(struct virtio_hw *hw, uint16_t vtpci_queue_idx)
-{
-	if (vtpci_queue_idx == hw->max_queue_pairs * 2)
-		return VTNET_CQ;
-	else if (vtpci_queue_idx % 2 == 0)
-		return VTNET_RQ;
-	else
-		return VTNET_TQ;
-}
-
 static uint16_t
 virtio_get_nr_vq(struct virtio_hw *hw)
 {
diff --git a/drivers/net/virtio/virtqueue.c b/drivers/net/virtio/virtqueue.c
index 1ada4fe08..5a8b21204 100644
--- a/drivers/net/virtio/virtqueue.c
+++ b/drivers/net/virtio/virtqueue.c
@@ -19,16 +19,38 @@ struct rte_mbuf *
 virtqueue_detatch_unused(struct virtqueue *vq)
 {
 	struct rte_mbuf *cookie;
-	int idx;
+	struct virtio_hw *hw;
+	uint16_t start, end;
+	int type, idx;
 
-	if (vq != NULL)
-		for (idx = 0; idx < vq->vq_nentries; idx++) {
+	if (vq == NULL)
+		return NULL;
+
+	hw = vq->hw;
+	type = virtio_get_queue_type(hw, vq->vq_queue_index);
+	start = vq->vq_avail_idx & (vq->vq_nentries - 1);
+	end = (vq->vq_avail_idx + vq->vq_free_cnt) & (vq->vq_nentries - 1);
+
+	for (idx = 0; idx < vq->vq_nentries; idx++) {
+		if (hw->use_simple_rx && type == VTNET_RQ) {
+			if (start <= end && idx >= start && idx < end)
+				continue;
+			if (start > end && (idx >= start || idx < end))
+				continue;
+			cookie = vq->sw_ring[idx];
+			if (cookie != NULL) {
+				vq->sw_ring[idx] = NULL;
+				return cookie;
+			}
+		} else {
 			cookie = vq->vq_descx[idx].cookie;
 			if (cookie != NULL) {
 				vq->vq_descx[idx].cookie = NULL;
 				return cookie;
 			}
 		}
+	}
+
 	return NULL;
 }
 
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index 2d9599218..e46de1583 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -292,6 +292,17 @@ virtqueue_full(const struct virtqueue *vq)
 	return vq->vq_free_cnt == 0;
 }
 
+static inline int
+virtio_get_queue_type(struct virtio_hw *hw, uint16_t vtpci_queue_idx)
+{
+	if (vtpci_queue_idx == hw->max_queue_pairs * 2)
+		return VTNET_CQ;
+	else if (vtpci_queue_idx % 2 == 0)
+		return VTNET_RQ;
+	else
+		return VTNET_TQ;
+}
+
 #define VIRTQUEUE_NUSED(vq) ((uint16_t)((vq)->vq_ring.used->idx - (vq)->vq_used_cons_idx))
 
 void vq_ring_free_chain(struct virtqueue *vq, uint16_t desc_idx);
-- 
2.11.0

[PATCH v3 2/4] net/virtio: fix memory leak when reinitializing device

[Olivier Matz]

Free the previous queues and the attached mbufs before initializing new
ones.

The function virtio_dev_free_mbufs() is now called when reconfiguring the
device, so we also need to add a check to ensure that it won't crash for
uninitialized queues.

Cc: stable@dpdk.org
Fixes: 60e6f4707ef2 ("net/virtio: reinitialize device when configuring")

Signed-off-by: Zijie Pan <zijie.pan@6wind.com>
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 24ffbeeb2..500556a05 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1471,6 +1471,11 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 	/* Reset the device although not necessary at startup */
 	vtpci_reset(hw);
 
+	if (hw->vqs) {
+		virtio_dev_free_mbufs(eth_dev);
+		virtio_free_queues(hw);
+	}
+
 	/* Tell the host we've noticed this device. */
 	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
 
@@ -1968,6 +1973,9 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		struct virtnet_rx *rxvq = dev->data->rx_queues[i];
 
+		if (rxvq == NULL || rxvq->vq == NULL)
+			continue;
+
 		PMD_INIT_LOG(DEBUG,
 			     "Before freeing rxq[%d] used and unused buf", i);
 		VIRTQUEUE_DUMP(rxvq->vq);
@@ -1987,6 +1995,9 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
 	for (i = 0; i < dev->data->nb_tx_queues; i++) {
 		struct virtnet_tx *txvq = dev->data->tx_queues[i];
 
+		if (txvq == NULL || txvq->vq == NULL)
+			continue;
+
 		PMD_INIT_LOG(DEBUG,
 			     "Before freeing txq[%d] used and unused bufs",
 			     i);
-- 
2.11.0

[PATCH v3 3/4] net/virtio: rationalize queue flushing

[Olivier Matz]

Use the same kind of loop than in virtio_free_queues() and factorize
common code.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 57 ++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 33 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 500556a05..260179598 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1967,53 +1967,44 @@ virtio_dev_start(struct rte_eth_dev *dev)
 
 static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
 {
+	struct virtio_hw *hw = dev->data->dev_private;
+	uint16_t nr_vq = virtio_get_nr_vq(hw);
+	const char *type __rte_unused;
+	unsigned int i, mbuf_num = 0;
+	struct virtqueue *vq;
 	struct rte_mbuf *buf;
-	int i, mbuf_num = 0;
-
-	for (i = 0; i < dev->data->nb_rx_queues; i++) {
-		struct virtnet_rx *rxvq = dev->data->rx_queues[i];
+	int queue_type;
 
-		if (rxvq == NULL || rxvq->vq == NULL)
+	for (i = 0; i < nr_vq; i++) {
+		vq = hw->vqs[i];
+		if (!vq)
 			continue;
 
-		PMD_INIT_LOG(DEBUG,
-			     "Before freeing rxq[%d] used and unused buf", i);
-		VIRTQUEUE_DUMP(rxvq->vq);
-
-		PMD_INIT_LOG(DEBUG, "rx_queues[%d]=%p", i, rxvq);
-		while ((buf = virtqueue_detatch_unused(rxvq->vq)) != NULL) {
-			rte_pktmbuf_free(buf);
-			mbuf_num++;
-		}
-
-		PMD_INIT_LOG(DEBUG, "free %d mbufs", mbuf_num);
-		PMD_INIT_LOG(DEBUG,
-			     "After freeing rxq[%d] used and unused buf", i);
-		VIRTQUEUE_DUMP(rxvq->vq);
-	}
-
-	for (i = 0; i < dev->data->nb_tx_queues; i++) {
-		struct virtnet_tx *txvq = dev->data->tx_queues[i];
-
-		if (txvq == NULL || txvq->vq == NULL)
+		queue_type = virtio_get_queue_type(hw, i);
+		if (queue_type == VTNET_RQ)
+			type = "rxq";
+		else if (queue_type == VTNET_TQ)
+			type = "txq";
+		else
 			continue;
 
 		PMD_INIT_LOG(DEBUG,
-			     "Before freeing txq[%d] used and unused bufs",
-			     i);
-		VIRTQUEUE_DUMP(txvq->vq);
+			"Before freeing %s[%d] used and unused buf",
+			type, i);
+		VIRTQUEUE_DUMP(vq);
 
-		mbuf_num = 0;
-		while ((buf = virtqueue_detatch_unused(txvq->vq)) != NULL) {
+		while ((buf = virtqueue_detatch_unused(vq)) != NULL) {
 			rte_pktmbuf_free(buf);
 			mbuf_num++;
 		}
 
-		PMD_INIT_LOG(DEBUG, "free %d mbufs", mbuf_num);
 		PMD_INIT_LOG(DEBUG,
-			     "After freeing txq[%d] used and unused buf", i);
-		VIRTQUEUE_DUMP(txvq->vq);
+			"After freeing %s[%d] used and unused buf",
+			type, i);
+		VIRTQUEUE_DUMP(vq);
 	}
+
+	PMD_INIT_LOG(DEBUG, "%d mbufs freed", mbuf_num);
 }
 
 /*
-- 
2.11.0

[PATCH v3 4/4] net/virtio: fix typo in function name

[Olivier Matz]

Fixes: c1f86306a026 ("virtio: add new driver")

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 2 +-
 drivers/net/virtio/virtqueue.c     | 2 +-
 drivers/net/virtio/virtqueue.h     | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 260179598..ce5663902 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1993,7 +1993,7 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
 			type, i);
 		VIRTQUEUE_DUMP(vq);
 
-		while ((buf = virtqueue_detatch_unused(vq)) != NULL) {
+		while ((buf = virtqueue_detach_unused(vq)) != NULL) {
 			rte_pktmbuf_free(buf);
 			mbuf_num++;
 		}
diff --git a/drivers/net/virtio/virtqueue.c b/drivers/net/virtio/virtqueue.c
index 5a8b21204..a7d0a9cbe 100644
--- a/drivers/net/virtio/virtqueue.c
+++ b/drivers/net/virtio/virtqueue.c
@@ -16,7 +16,7 @@
  * 2) mbuf that hasn't been consued by backend.
  */
 struct rte_mbuf *
-virtqueue_detatch_unused(struct virtqueue *vq)
+virtqueue_detach_unused(struct virtqueue *vq)
 {
 	struct rte_mbuf *cookie;
 	struct virtio_hw *hw;
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index e46de1583..14364f356 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -281,7 +281,7 @@ void virtqueue_dump(struct virtqueue *vq);
 /**
  *  Get all mbufs to be freed.
  */
-struct rte_mbuf *virtqueue_detatch_unused(struct virtqueue *vq);
+struct rte_mbuf *virtqueue_detach_unused(struct virtqueue *vq);
 
 /* Flush the elements in the used ring. */
 void virtqueue_rxvq_flush(struct virtqueue *vq);
-- 
2.11.0

Re: [PATCH v3 0/4] net/virtio: fix memory leak when reinitializing device

[Yuanhan Liu]

On Tue, Jan 23, 2018 at 04:54:39PM +0100, Olivier Matz wrote:
> When devops->configure() is called, the queues may be reallocated
> if the features flag changed, but the previous are not freed. This
> patchset fixes this issue.

Seires applied to dpdk-next-virtio.

Thanks.

	--yliu
> 
> To really point out the issue, I instrumented rte_malloc,
> rte_memzone_reserve, rte_mbuf_alloc to track the allocations and frees.
> For reference, the patch is available here:
> https://www.droids-corp.org/~zer0/hidden/0001-debug-malloc.patch
> Iit conflicts a bit with the patchset, but it is quite easy to solve.
> 
> To reproduce the issue:
> 
>   cd dpdk
>   make config T=x86_64-native-linuxapp-gcc
>   sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=y,' build/.config
>   make -j4
>   mkdir -p /mnt/huge
>   mount -t hugetlbfs nodev /mnt/huge
>   echo 256 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
>   modprobe uio_pci_generic
>   python usertools/dpdk-devbind.py -b uio_pci_generic 0000:00:02.0
>   ./build/app/testpmd -l 0,1 --log-level 8 -- --total-num-mbufs=16384 \
>      -i --port-topology=chained --disable-hw-vlan-filter \
>      --disable-hw-vlan-strip --enable-rx-cksum --enable-lro \
>      --txqflags=0 --no-lsc-interrupt
> 
>   # testpmd commands
>   port stop 0
>   port config all rx-cksum off
>   port start 0
>   quit
> 
> Without the patchset, the debug patch displays a leak of mbufs allocated
> from virtio_dev_rx_queue_setup_finish() and a leak of rte_mallocs allocated
> from virtio_init_queue().
> 
> After the patchset, only one malloc is remaining after the device close,
> but this is normal, because this area is freed in dev_uninit().
> 
> v2 -> v3:
> - fix queue flushing with vector Rx enabled (Tiwei).
>   The patch is slightly changed compared to what was proposed by mail,
>   due to a checkpatch warning.
> 
> v1 -> v2:
> - move the vq != NULL check from the refactor patch to the memory
>   leak fix patch (Yuanhan)
> - new patch to fix queue flushing with vector Rx enabled (Tiwei)
> - move the backportable fixes at the beginning of the patchset (Yuanhan)
> - remove Cc stable on the typo fix (Yuanhan)
>   note: I finally kept it in the patchset
> 
> 
> Olivier Matz (4):
>   net/virtio: fix queue flushing with vector Rx enabled
>   net/virtio: fix memory leak when reinitializing device
>   net/virtio: rationalize queue flushing
>   net/virtio: fix typo in function name
> 
>  drivers/net/virtio/virtio_ethdev.c | 71 +++++++++++++++++---------------------
>  drivers/net/virtio/virtqueue.c     | 30 +++++++++++++---
>  drivers/net/virtio/virtqueue.h     | 13 ++++++-
>  3 files changed, 69 insertions(+), 45 deletions(-)
> 
> -- 
> 2.11.0