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

[dpdk-stable] [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

Re: [dpdk-stable] [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: [dpdk-stable] [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: [dpdk-stable] [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

[dpdk-stable] [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

Re: [dpdk-stable] [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: [dpdk-stable] [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: [dpdk-stable] [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: [dpdk-stable] [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: [dpdk-stable] [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: [dpdk-stable] [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: [dpdk-stable] [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.

[dpdk-stable] [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

[dpdk-stable] [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

Re: [dpdk-stable] [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: [dpdk-stable] [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: [dpdk-stable] [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

[dpdk-stable] [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

[dpdk-stable] [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

[dpdk-stable] [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