diff mbox series

[v3,2/2] ethdev: complete closing of port

Message ID 20181014232020.12114-3-thomas@monjalon.net (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev port freeing |

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Thomas Monjalon Oct. 14, 2018, 11:20 p.m. UTC 
  After closing a port, it cannot be restarted.
So there is no reason to not free all associated resources.

The last step was done with rte_eth_dev_detach() which is deprecated.
Instead of blindly removing the associated rte_device, the driver should
check if no more port (ethdev, cryptodev, etc) is open for the device.

The last ethdev freeing which were done by rte_eth_dev_detach(),
are now done at the end of rte_eth_dev_close().

If the driver is trying to free the port again, the function
rte_eth_dev_release_port() will abort with -ENODEV error.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 drivers/net/bnxt/bnxt_ethdev.c    | 4 ----
 drivers/net/vhost/rte_eth_vhost.c | 4 ----
 lib/librte_ethdev/rte_ethdev.c    | 9 +++------
 lib/librte_ethdev/rte_ethdev.h    | 3 +--
 4 files changed, 4 insertions(+), 16 deletions(-)

Comments

Andrew Rybchenko Oct. 16, 2018, 11:24 a.m. UTC | #1 
On 10/15/18 2:20 AM, Thomas Monjalon wrote:
> After closing a port, it cannot be restarted.
> So there is no reason to not free all associated resources.
>
> The last step was done with rte_eth_dev_detach() which is deprecated.
> Instead of blindly removing the associated rte_device, the driver should
> check if no more port (ethdev, cryptodev, etc) is open for the device.
>
> The last ethdev freeing which were done by rte_eth_dev_detach(),
> are now done at the end of rte_eth_dev_close().
>
> If the driver is trying to free the port again, the function
> rte_eth_dev_release_port() will abort with -ENODEV error.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>   drivers/net/bnxt/bnxt_ethdev.c    | 4 ----
>   drivers/net/vhost/rte_eth_vhost.c | 4 ----
>   lib/librte_ethdev/rte_ethdev.c    | 9 +++------
>   lib/librte_ethdev/rte_ethdev.h    | 3 +--
>   4 files changed, 4 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> index ff41cb0fa..183b40821 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -712,10 +712,6 @@ static void bnxt_dev_close_op(struct rte_eth_dev *eth_dev)
>   	if (bp->dev_stopped == 0)
>   		bnxt_dev_stop_op(eth_dev);
>   
> -	if (eth_dev->data->mac_addrs != NULL) {
> -		rte_free(eth_dev->data->mac_addrs);
> -		eth_dev->data->mac_addrs = NULL;
> -	}
>   	if (bp->grp_info != NULL) {
>   		rte_free(bp->grp_info);
>   		bp->grp_info = NULL;
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index 986bf9633..b9cefbbe5 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -998,12 +998,8 @@ eth_dev_close(struct rte_eth_dev *dev)
>   		for (i = 0; i < dev->data->nb_tx_queues; i++)
>   			rte_free(dev->data->tx_queues[i]);
>   
> -	rte_free(dev->data->mac_addrs);
>   	free(internal->dev_name);
>   	free(internal->iface_name);
> -	rte_free(internal);
> -
> -	dev->data->dev_private = NULL;
>   }
>   
>   static int

It looks like above snippets should be a part of previous changeset.

> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 178800a5b..987ba5ab1 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -367,6 +367,8 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
>   {
>   	if (eth_dev == NULL)
>   		return -EINVAL;
> +	if (eth_dev->state == RTE_ETH_DEV_UNUSED)
> +		return -ENODEV;
>   
>   	rte_eth_dev_shared_data_prepare();
>   
> @@ -1384,12 +1386,7 @@ rte_eth_dev_close(uint16_t port_id)
>   	dev->data->dev_started = 0;
>   	(*dev->dev_ops->dev_close)(dev);
>   
> -	dev->data->nb_rx_queues = 0;
> -	rte_free(dev->data->rx_queues);
> -	dev->data->rx_queues = NULL;
> -	dev->data->nb_tx_queues = 0;
> -	rte_free(dev->data->tx_queues);
> -	dev->data->tx_queues = NULL;
> +	rte_eth_dev_release_port(dev);
>   }
>   
>   int

Right now it introduces resource leak for resources which are
freed by PCI drivers in uninit.
Thomas Monjalon Oct. 16, 2018, 12:25 p.m. UTC | #2 
16/10/2018 13:24, Andrew Rybchenko:
> On 10/15/18 2:20 AM, Thomas Monjalon wrote:
> > After closing a port, it cannot be restarted.
> > So there is no reason to not free all associated resources.
> >
> > The last step was done with rte_eth_dev_detach() which is deprecated.
> > Instead of blindly removing the associated rte_device, the driver should
> > check if no more port (ethdev, cryptodev, etc) is open for the device.
> >
> > The last ethdev freeing which were done by rte_eth_dev_detach(),
> > are now done at the end of rte_eth_dev_close().
> >
> > If the driver is trying to free the port again, the function
> > rte_eth_dev_release_port() will abort with -ENODEV error.
> >
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> >   drivers/net/bnxt/bnxt_ethdev.c    | 4 ----
> >   drivers/net/vhost/rte_eth_vhost.c | 4 ----
> >   lib/librte_ethdev/rte_ethdev.c    | 9 +++------
> >   lib/librte_ethdev/rte_ethdev.h    | 3 +--
> >   4 files changed, 4 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> > index ff41cb0fa..183b40821 100644
> > --- a/drivers/net/bnxt/bnxt_ethdev.c
> > +++ b/drivers/net/bnxt/bnxt_ethdev.c
> > @@ -712,10 +712,6 @@ static void bnxt_dev_close_op(struct rte_eth_dev *eth_dev)
> >   	if (bp->dev_stopped == 0)
> >   		bnxt_dev_stop_op(eth_dev);
> >   
> > -	if (eth_dev->data->mac_addrs != NULL) {
> > -		rte_free(eth_dev->data->mac_addrs);
> > -		eth_dev->data->mac_addrs = NULL;
> > -	}
> >   	if (bp->grp_info != NULL) {
> >   		rte_free(bp->grp_info);
> >   		bp->grp_info = NULL;
> > diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> > index 986bf9633..b9cefbbe5 100644
> > --- a/drivers/net/vhost/rte_eth_vhost.c
> > +++ b/drivers/net/vhost/rte_eth_vhost.c
> > @@ -998,12 +998,8 @@ eth_dev_close(struct rte_eth_dev *dev)
> >   		for (i = 0; i < dev->data->nb_tx_queues; i++)
> >   			rte_free(dev->data->tx_queues[i]);
> >   
> > -	rte_free(dev->data->mac_addrs);
> >   	free(internal->dev_name);
> >   	free(internal->iface_name);
> > -	rte_free(internal);
> > -
> > -	dev->data->dev_private = NULL;
> >   }
> >   
> >   static int
> 
> It looks like above snippets should be a part of previous changeset.

No, this is related to close function.

> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -367,6 +367,8 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
> >   {
> >   	if (eth_dev == NULL)
> >   		return -EINVAL;
> > +	if (eth_dev->state == RTE_ETH_DEV_UNUSED)
> > +		return -ENODEV;
> >   
> >   	rte_eth_dev_shared_data_prepare();
> >   
> > @@ -1384,12 +1386,7 @@ rte_eth_dev_close(uint16_t port_id)
> >   	dev->data->dev_started = 0;
> >   	(*dev->dev_ops->dev_close)(dev);
> >   
> > -	dev->data->nb_rx_queues = 0;
> > -	rte_free(dev->data->rx_queues);
> > -	dev->data->rx_queues = NULL;
> > -	dev->data->nb_tx_queues = 0;
> > -	rte_free(dev->data->tx_queues);
> > -	dev->data->tx_queues = NULL;
> > +	rte_eth_dev_release_port(dev);
> >   }
> >   
> >   int
> 
> Right now it introduces resource leak for resources which are
> freed by PCI drivers in uninit.

No, these resources are freed by rte_eth_dev_release_port.
diff mbox series

Patch

diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index ff41cb0fa..183b40821 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -712,10 +712,6 @@  static void bnxt_dev_close_op(struct rte_eth_dev *eth_dev)
 	if (bp->dev_stopped == 0)
 		bnxt_dev_stop_op(eth_dev);
 
-	if (eth_dev->data->mac_addrs != NULL) {
-		rte_free(eth_dev->data->mac_addrs);
-		eth_dev->data->mac_addrs = NULL;
-	}
 	if (bp->grp_info != NULL) {
 		rte_free(bp->grp_info);
 		bp->grp_info = NULL;
diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 986bf9633..b9cefbbe5 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -998,12 +998,8 @@  eth_dev_close(struct rte_eth_dev *dev)
 		for (i = 0; i < dev->data->nb_tx_queues; i++)
 			rte_free(dev->data->tx_queues[i]);
 
-	rte_free(dev->data->mac_addrs);
 	free(internal->dev_name);
 	free(internal->iface_name);
-	rte_free(internal);
-
-	dev->data->dev_private = NULL;
 }
 
 static int
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 178800a5b..987ba5ab1 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -367,6 +367,8 @@  rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 {
 	if (eth_dev == NULL)
 		return -EINVAL;
+	if (eth_dev->state == RTE_ETH_DEV_UNUSED)
+		return -ENODEV;
 
 	rte_eth_dev_shared_data_prepare();
 
@@ -1384,12 +1386,7 @@  rte_eth_dev_close(uint16_t port_id)
 	dev->data->dev_started = 0;
 	(*dev->dev_ops->dev_close)(dev);
 
-	dev->data->nb_rx_queues = 0;
-	rte_free(dev->data->rx_queues);
-	dev->data->rx_queues = NULL;
-	dev->data->nb_tx_queues = 0;
-	rte_free(dev->data->tx_queues);
-	dev->data->tx_queues = NULL;
+	rte_eth_dev_release_port(dev);
 }
 
 int
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index fb40c89e0..dcdeb184b 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1802,8 +1802,7 @@  int rte_eth_dev_set_link_down(uint16_t port_id);
 
 /**
  * Close a stopped Ethernet device. The device cannot be restarted!
- * The function frees all resources except for needed by the
- * closed state. To free these resources, call rte_eth_dev_detach().
+ * The function frees all port resources.
  *
  * @param port_id
  *   The port identifier of the Ethernet device.