[dpdk-dev] [PATCH 20.05] ethdev: clear struct on releasing port

[dpdk-dev] [PATCH 20.05] ethdev: clear struct on releasing port

[Thomas Monjalon]

The function rte_eth_dev_release_port() is called by drivers
when closing or removing a device.
Its main usage should be via rte_eth_dev_close() by up-to-date
drivers which are compliant with RTE_ETH_DEV_CLOSE_REMOVE flag.

When a port is released, the data (rte_eth_dev_data) are cleared,
but the pointers in rte_eth_dev were not cleared.
It may cause issues with code paths trying to use dangling pointers
(e.g. the .device pointer which may reference a removed rte_device).
Everything is now cleared to 0 when releasing.
The state is explicitly set to RTE_ETH_DEV_UNUSED which is 0 anyway.

Using this patch may reveal bugs in some code paths:
	- device pointer must be saved before closing a port if needed
	- drivers must close ports on device remove for consistent cleanup

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_ethdev/rte_ethdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 774c721b34..2a43a9abe9 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -553,8 +553,6 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 
 	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
 
-	eth_dev->state = RTE_ETH_DEV_UNUSED;
-
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 		rte_free(eth_dev->data->rx_queues);
 		rte_free(eth_dev->data->tx_queues);
@@ -563,6 +561,8 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 		rte_free(eth_dev->data->dev_private);
 		memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
 	}
+	memset(eth_dev, 0, sizeof(struct rte_eth_dev));
+	eth_dev->state = RTE_ETH_DEV_UNUSED;
 
 	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
 
-- 
2.25.0

Re: [dpdk-dev] [PATCH 20.05] ethdev: clear struct on releasing port

[Ferruh Yigit]

On 2/13/2020 2:33 PM, Thomas Monjalon wrote:
> The function rte_eth_dev_release_port() is called by drivers
> when closing or removing a device.
> Its main usage should be via rte_eth_dev_close() by up-to-date
> drivers which are compliant with RTE_ETH_DEV_CLOSE_REMOVE flag.
> 
> When a port is released, the data (rte_eth_dev_data) are cleared,
> but the pointers in rte_eth_dev were not cleared.
> It may cause issues with code paths trying to use dangling pointers
> (e.g. the .device pointer which may reference a removed rte_device).
> Everything is now cleared to 0 when releasing.
> The state is explicitly set to RTE_ETH_DEV_UNUSED which is 0 anyway.
> 
> Using this patch may reveal bugs in some code paths:
> 	- device pointer must be saved before closing a port if needed

Is this saving should be done in application code or will be done by ethdev?

> 	- drivers must close ports on device remove for consistent cleanup
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  lib/librte_ethdev/rte_ethdev.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 774c721b34..2a43a9abe9 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -553,8 +553,6 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
>  
>  	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
>  
> -	eth_dev->state = RTE_ETH_DEV_UNUSED;
> -
>  	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>  		rte_free(eth_dev->data->rx_queues);
>  		rte_free(eth_dev->data->tx_queues);
> @@ -563,6 +561,8 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
>  		rte_free(eth_dev->data->dev_private);
>  		memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
>  	}
> +	memset(eth_dev, 0, sizeof(struct rte_eth_dev));
> +	eth_dev->state = RTE_ETH_DEV_UNUSED;
>  
>  	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
>  
> 

Re: [dpdk-dev] [PATCH 20.05] ethdev: clear struct on releasing port

[Thomas Monjalon]

13/02/2020 16:16, Ferruh Yigit:
> On 2/13/2020 2:33 PM, Thomas Monjalon wrote:
> > The function rte_eth_dev_release_port() is called by drivers
> > when closing or removing a device.
> > Its main usage should be via rte_eth_dev_close() by up-to-date
> > drivers which are compliant with RTE_ETH_DEV_CLOSE_REMOVE flag.
> > 
> > When a port is released, the data (rte_eth_dev_data) are cleared,
> > but the pointers in rte_eth_dev were not cleared.
> > It may cause issues with code paths trying to use dangling pointers
> > (e.g. the .device pointer which may reference a removed rte_device).
> > Everything is now cleared to 0 when releasing.
> > The state is explicitly set to RTE_ETH_DEV_UNUSED which is 0 anyway.
> > 
> > Using this patch may reveal bugs in some code paths:
> > 	- device pointer must be saved before closing a port if needed
> 
> Is this saving should be done in application code or will be done by ethdev?

by the application
From ethdev point of view, when a port is closed, there is nothing more to do next.

Re: [dpdk-dev] [PATCH 20.05] ethdev: clear struct on releasing port

[Ferruh Yigit]

On 2/13/2020 3:27 PM, Thomas Monjalon wrote:
> 13/02/2020 16:16, Ferruh Yigit:
>> On 2/13/2020 2:33 PM, Thomas Monjalon wrote:
>>> The function rte_eth_dev_release_port() is called by drivers
>>> when closing or removing a device.
>>> Its main usage should be via rte_eth_dev_close() by up-to-date
>>> drivers which are compliant with RTE_ETH_DEV_CLOSE_REMOVE flag.
>>>
>>> When a port is released, the data (rte_eth_dev_data) are cleared,
>>> but the pointers in rte_eth_dev were not cleared.
>>> It may cause issues with code paths trying to use dangling pointers
>>> (e.g. the .device pointer which may reference a removed rte_device).
>>> Everything is now cleared to 0 when releasing.
>>> The state is explicitly set to RTE_ETH_DEV_UNUSED which is 0 anyway.
>>>
>>> Using this patch may reveal bugs in some code paths:
>>> 	- device pointer must be saved before closing a port if needed
>>
>> Is this saving should be done in application code or will be done by ethdev?
> 
> by the application
> From ethdev point of view, when a port is closed, there is nothing more to do next.
> 

That sound reasonable from ethdev perspective, but it will push dealing with
rte_device level details to the application.

Like this change will be forcing each application to store the rte_device
pointer before close(), in case device can be detached later.

Re: [dpdk-dev] [PATCH 20.05] ethdev: clear struct on releasing port

[Thomas Monjalon]

13/02/2020 17:05, Ferruh Yigit:
> On 2/13/2020 3:27 PM, Thomas Monjalon wrote:
> > 13/02/2020 16:16, Ferruh Yigit:
> >> On 2/13/2020 2:33 PM, Thomas Monjalon wrote:
> >>> The function rte_eth_dev_release_port() is called by drivers
> >>> when closing or removing a device.
> >>> Its main usage should be via rte_eth_dev_close() by up-to-date
> >>> drivers which are compliant with RTE_ETH_DEV_CLOSE_REMOVE flag.
> >>>
> >>> When a port is released, the data (rte_eth_dev_data) are cleared,
> >>> but the pointers in rte_eth_dev were not cleared.
> >>> It may cause issues with code paths trying to use dangling pointers
> >>> (e.g. the .device pointer which may reference a removed rte_device).
> >>> Everything is now cleared to 0 when releasing.
> >>> The state is explicitly set to RTE_ETH_DEV_UNUSED which is 0 anyway.
> >>>
> >>> Using this patch may reveal bugs in some code paths:
> >>> 	- device pointer must be saved before closing a port if needed
> >>
> >> Is this saving should be done in application code or will be done by ethdev?
> > 
> > by the application
> > From ethdev point of view, when a port is closed, there is nothing more to do next.
> > 
> 
> That sound reasonable from ethdev perspective, but it will push dealing with
> rte_device level details to the application.
> 
> Like this change will be forcing each application to store the rte_device
> pointer before close(), in case device can be detached later.

Yes
I think PMDs could release rte_device when closing the last port of the device.
I am not sure what is the benefit of explicit detach.
That's something we should discuss, agree and write in the API doc.