[dpdk-dev,2/3] net/virtio: rationalize queue flushing

  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(-)
  

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
  

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
  

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
  

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
>
  

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
  

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
  

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.