[dpdk-dev,4/5] virtio: free queue memory in virtio_dev_close()

Message ID 1436879459-18400-5-git-send-email-bernard.iremonger@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Iremonger, Bernard July 14, 2015, 1:10 p.m. UTC
  Add function virtio_free_queues() and call from virtio_dev_close()
Use virtio_dev_rx_queue_release() and virtio_dev_tx_queue_release()

Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
  

Comments

Stephen Hemminger July 14, 2015, 6:28 p.m. UTC | #1
On Tue, 14 Jul 2015 14:10:58 +0100
Bernard Iremonger <bernard.iremonger@intel.com> wrote:

>  static void
> +virtio_free_queues(struct rte_eth_dev *dev)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> +		virtio_dev_rx_queue_release(dev->data->rx_queues[i]);
> +		dev->data->rx_queues[i] = NULL;
> +	}
> +	dev->data->nb_rx_queues = 0;
> +
> +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
> +		virtio_dev_tx_queue_release(dev->data->tx_queues[i]);
> +		dev->data->tx_queues[i] = NULL;
> +	}
> +	dev->data->nb_tx_queues = 0;
> +}
> +

Where does command queue get freed?
  
Ouyang Changchun July 15, 2015, 1:36 a.m. UTC | #2
> -----Original Message-----
> From: Iremonger, Bernard
> Sent: Tuesday, July 14, 2015 9:11 PM
> To: dev@dpdk.org
> Cc: Ouyang, Changchun; stephen@networkplumber.org; Iremonger, Bernard
> Subject: [PATCH 4/5] virtio: free queue memory in virtio_dev_close()
> 
> Add function virtio_free_queues() and call from virtio_dev_close() Use
> virtio_dev_rx_queue_release() and virtio_dev_tx_queue_release()
> 
> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> index b32b3e9..4676ab1 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -438,6 +438,24 @@ virtio_dev_cq_queue_setup(struct rte_eth_dev
> *dev, uint16_t vtpci_queue_idx,  }
> 
>  static void
> +virtio_free_queues(struct rte_eth_dev *dev) {
> +	unsigned int i;
> +
> +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> +		virtio_dev_rx_queue_release(dev->data->rx_queues[i]);
> +		dev->data->rx_queues[i] = NULL;
> +	}
> +	dev->data->nb_rx_queues = 0;
> +
> +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
> +		virtio_dev_tx_queue_release(dev->data->tx_queues[i]);
> +		dev->data->tx_queues[i] = NULL;
> +	}
> +	dev->data->nb_tx_queues = 0;
> +}
> +
> +static void
>  virtio_dev_close(struct rte_eth_dev *dev)  {
>  	struct virtio_hw *hw = dev->data->dev_private; @@ -451,6 +469,7
> @@ virtio_dev_close(struct rte_eth_dev *dev)
>  	vtpci_reset(hw);
>  	hw->started = 0;
>  	virtio_dev_free_mbufs(dev);
> +	virtio_free_queues(dev);

Validate it with vhost sample or not for this change?

>  }
> 
>  static void
> --
> 1.9.1
  
Iremonger, Bernard July 15, 2015, 8:01 a.m. UTC | #3
Hi  Ouyang,

<snip>

> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -438,6 +438,24 @@ virtio_dev_cq_queue_setup(struct rte_eth_dev
> > *dev, uint16_t vtpci_queue_idx,  }
> >
> >  static void
> > +virtio_free_queues(struct rte_eth_dev *dev) {
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > +		virtio_dev_rx_queue_release(dev->data->rx_queues[i]);
> > +		dev->data->rx_queues[i] = NULL;
> > +	}
> > +	dev->data->nb_rx_queues = 0;
> > +
> > +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > +		virtio_dev_tx_queue_release(dev->data->tx_queues[i]);
> > +		dev->data->tx_queues[i] = NULL;
> > +	}
> > +	dev->data->nb_tx_queues = 0;
> > +}
> > +
> > +static void
> >  virtio_dev_close(struct rte_eth_dev *dev)  {
> >  	struct virtio_hw *hw = dev->data->dev_private; @@ -451,6 +469,7
> @@
> > virtio_dev_close(struct rte_eth_dev *dev)
> >  	vtpci_reset(hw);
> >  	hw->started = 0;
> >  	virtio_dev_free_mbufs(dev);
> > +	virtio_free_queues(dev);
> 
> Validate it with vhost sample or not for this change?

I have tested this change with testpmd on a Fedora VM.

Regards,

Bernard.

> 
> >  }
> >
> >  static void
> > --
> > 1.9.1
  
Iremonger, Bernard July 15, 2015, 8:27 a.m. UTC | #4
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, July 14, 2015 7:28 PM
> To: Iremonger, Bernard
> Cc: dev@dpdk.org; Ouyang, Changchun
> Subject: Re: [PATCH 4/5] virtio: free queue memory in virtio_dev_close()
> 
> On Tue, 14 Jul 2015 14:10:58 +0100
> Bernard Iremonger <bernard.iremonger@intel.com> wrote:
> 
> >  static void
> > +virtio_free_queues(struct rte_eth_dev *dev) {
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > +		virtio_dev_rx_queue_release(dev->data->rx_queues[i]);
> > +		dev->data->rx_queues[i] = NULL;
> > +	}
> > +	dev->data->nb_rx_queues = 0;
> > +
> > +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > +		virtio_dev_tx_queue_release(dev->data->tx_queues[i]);
> > +		dev->data->tx_queues[i] = NULL;
> > +	}
> > +	dev->data->nb_tx_queues = 0;
> > +}
> > +
> 
> Where does command queue get freed?

The command queue is set up in the eth_virtio_dev_init() function and  freed in the eth_virtio_dev_uninit() function.

Regards,

Bernard.
  
Ouyang Changchun July 15, 2015, 8:36 a.m. UTC | #5
Hi, Bernard

> -----Original Message-----
> From: Iremonger, Bernard
> Sent: Wednesday, July 15, 2015 4:02 PM
> To: Ouyang, Changchun; dev@dpdk.org
> Cc: Xu, Qian Q; stephen@networkplumber.org
> Subject: RE: [PATCH 4/5] virtio: free queue memory in virtio_dev_close()
> 
> Hi  Ouyang,
> 
> <snip>
> 
> > > --- a/drivers/net/virtio/virtio_ethdev.c
> > > +++ b/drivers/net/virtio/virtio_ethdev.c
> > > @@ -438,6 +438,24 @@ virtio_dev_cq_queue_setup(struct rte_eth_dev
> > > *dev, uint16_t vtpci_queue_idx,  }
> > >
> > >  static void
> > > +virtio_free_queues(struct rte_eth_dev *dev) {
> > > +	unsigned int i;
> > > +
> > > +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > > +		virtio_dev_rx_queue_release(dev->data->rx_queues[i]);
> > > +		dev->data->rx_queues[i] = NULL;
> > > +	}
> > > +	dev->data->nb_rx_queues = 0;
> > > +
> > > +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > > +		virtio_dev_tx_queue_release(dev->data->tx_queues[i]);
> > > +		dev->data->tx_queues[i] = NULL;
> > > +	}
> > > +	dev->data->nb_tx_queues = 0;
> > > +}
> > > +
> > > +static void
> > >  virtio_dev_close(struct rte_eth_dev *dev)  {
> > >  	struct virtio_hw *hw = dev->data->dev_private; @@ -451,6 +469,7
> > @@
> > > virtio_dev_close(struct rte_eth_dev *dev)
> > >  	vtpci_reset(hw);
> > >  	hw->started = 0;
> > >  	virtio_dev_free_mbufs(dev);
> > > +	virtio_free_queues(dev);
> >
> > Validate it with vhost sample or not for this change?
> 
> I have tested this change with testpmd on a Fedora VM.

I think we should make sure it will not break any current test case for virtio,
So before applying it, it needs use vhost sample on host and test the virtio driver on guest.

Thanks
Changchun
  
Ouyang Changchun July 15, 2015, 8:38 a.m. UTC | #6
> -----Original Message-----
> From: Iremonger, Bernard
> Sent: Wednesday, July 15, 2015 4:27 PM
> To: Stephen Hemminger
> Cc: dev@dpdk.org; Ouyang, Changchun
> Subject: RE: [PATCH 4/5] virtio: free queue memory in virtio_dev_close()
> 
> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Tuesday, July 14, 2015 7:28 PM
> > To: Iremonger, Bernard
> > Cc: dev@dpdk.org; Ouyang, Changchun
> > Subject: Re: [PATCH 4/5] virtio: free queue memory in virtio_dev_close()
> >
> > On Tue, 14 Jul 2015 14:10:58 +0100
> > Bernard Iremonger <bernard.iremonger@intel.com> wrote:
> >
> > >  static void
> > > +virtio_free_queues(struct rte_eth_dev *dev) {
> > > +	unsigned int i;
> > > +
> > > +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > > +		virtio_dev_rx_queue_release(dev->data->rx_queues[i]);
> > > +		dev->data->rx_queues[i] = NULL;
> > > +	}
> > > +	dev->data->nb_rx_queues = 0;
> > > +
> > > +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > > +		virtio_dev_tx_queue_release(dev->data->tx_queues[i]);
> > > +		dev->data->tx_queues[i] = NULL;
> > > +	}
> > > +	dev->data->nb_tx_queues = 0;
> > > +}
> > > +
> >
> > Where does command queue get freed?
> 
> The command queue is set up in the eth_virtio_dev_init() function and
> freed in the eth_virtio_dev_uninit() function.
> 

Do you mean control vq?

> Regards,
> 
> Bernard.
  
Iremonger, Bernard July 15, 2015, 8:50 a.m. UTC | #7
> -----Original Message-----
> From: Ouyang, Changchun
> Sent: Wednesday, July 15, 2015 9:39 AM
> To: Iremonger, Bernard; Stephen Hemminger
> Cc: dev@dpdk.org; Ouyang, Changchun
> Subject: RE: [PATCH 4/5] virtio: free queue memory in virtio_dev_close()
> 
> 
> 
> > -----Original Message-----
> > From: Iremonger, Bernard
> > Sent: Wednesday, July 15, 2015 4:27 PM
> > To: Stephen Hemminger
> > Cc: dev@dpdk.org; Ouyang, Changchun
> > Subject: RE: [PATCH 4/5] virtio: free queue memory in
> > virtio_dev_close()
> >
> > > -----Original Message-----
> > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > Sent: Tuesday, July 14, 2015 7:28 PM
> > > To: Iremonger, Bernard
> > > Cc: dev@dpdk.org; Ouyang, Changchun
> > > Subject: Re: [PATCH 4/5] virtio: free queue memory in
> > > virtio_dev_close()
> > >
> > > On Tue, 14 Jul 2015 14:10:58 +0100
> > > Bernard Iremonger <bernard.iremonger@intel.com> wrote:
> > >
> > > >  static void
> > > > +virtio_free_queues(struct rte_eth_dev *dev) {
> > > > +	unsigned int i;
> > > > +
> > > > +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > > > +		virtio_dev_rx_queue_release(dev->data->rx_queues[i]);
> > > > +		dev->data->rx_queues[i] = NULL;
> > > > +	}
> > > > +	dev->data->nb_rx_queues = 0;
> > > > +
> > > > +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > > > +		virtio_dev_tx_queue_release(dev->data->tx_queues[i]);
> > > > +		dev->data->tx_queues[i] = NULL;
> > > > +	}
> > > > +	dev->data->nb_tx_queues = 0;
> > > > +}
> > > > +
> > >
> > > Where does command queue get freed?
> >
> > The command queue is set up in the eth_virtio_dev_init() function and
> > freed in the eth_virtio_dev_uninit() function.
> >
> 
> Do you mean control vq?

I am referring to hw->cvq.

Regards,

Bernard.
  

Patch

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index b32b3e9..4676ab1 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -438,6 +438,24 @@  virtio_dev_cq_queue_setup(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx,
 }
 
 static void
+virtio_free_queues(struct rte_eth_dev *dev)
+{
+	unsigned int i;
+
+	for (i = 0; i < dev->data->nb_rx_queues; i++) {
+		virtio_dev_rx_queue_release(dev->data->rx_queues[i]);
+		dev->data->rx_queues[i] = NULL;
+	}
+	dev->data->nb_rx_queues = 0;
+
+	for (i = 0; i < dev->data->nb_tx_queues; i++) {
+		virtio_dev_tx_queue_release(dev->data->tx_queues[i]);
+		dev->data->tx_queues[i] = NULL;
+	}
+	dev->data->nb_tx_queues = 0;
+}
+
+static void
 virtio_dev_close(struct rte_eth_dev *dev)
 {
 	struct virtio_hw *hw = dev->data->dev_private;
@@ -451,6 +469,7 @@  virtio_dev_close(struct rte_eth_dev *dev)
 	vtpci_reset(hw);
 	hw->started = 0;
 	virtio_dev_free_mbufs(dev);
+	virtio_free_queues(dev);
 }
 
 static void