diff mbox series

[10/20] net/null: release port upon close

Message ID 20200913220711.3768597-11-thomas@monjalon.net (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series cleanup ethdev close operation |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Thomas Monjalon Sept. 13, 2020, 10:07 p.m. UTC 
  The flag RTE_ETH_DEV_CLOSE_REMOVE is set so all port resources
can be freed by rte_eth_dev_close().

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 drivers/net/null/rte_eth_null.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

Comments

Ferruh Yigit Sept. 23, 2020, 4:44 p.m. UTC | #1 
On 9/13/2020 11:07 PM, Thomas Monjalon wrote:
> The flag RTE_ETH_DEV_CLOSE_REMOVE is set so all port resources
> can be freed by rte_eth_dev_close().
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>   drivers/net/null/rte_eth_null.c | 24 ++++++++++++++++--------
>   1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
> index 0ce073fa4b..33997013e4 100644
> --- a/drivers/net/null/rte_eth_null.c
> +++ b/drivers/net/null/rte_eth_null.c
> @@ -458,7 +458,20 @@ eth_mac_address_set(__rte_unused struct rte_eth_dev *dev,
>   	return 0;
>   }
>   
> +static int
> +eth_dev_close(struct rte_eth_dev *dev)
> +{
> +	PMD_LOG(INFO, "Closing null ethdev on NUMA socket %u",
> +			rte_socket_id());
> +
> +	/* mac_addrs must not be freed alone because part of dev_private */
> +	dev->data->mac_addrs = NULL;
> +
> +	return 0;
> +}

should check 'RTE_PROC_PRIMARY' in 'eth_dev_close()'?
Thomas Monjalon Sept. 23, 2020, 8:47 p.m. UTC | #2 
23/09/2020 18:44, Ferruh Yigit:
> On 9/13/2020 11:07 PM, Thomas Monjalon wrote:
> > The flag RTE_ETH_DEV_CLOSE_REMOVE is set so all port resources
> > can be freed by rte_eth_dev_close().
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> > --- a/drivers/net/null/rte_eth_null.c
> > +++ b/drivers/net/null/rte_eth_null.c
> > +static int
> > +eth_dev_close(struct rte_eth_dev *dev)
> > +{
> > +	PMD_LOG(INFO, "Closing null ethdev on NUMA socket %u",
> > +			rte_socket_id());
> > +
> > +	/* mac_addrs must not be freed alone because part of dev_private */
> > +	dev->data->mac_addrs = NULL;
> > +
> > +	return 0;
> > +}
> 
> should check 'RTE_PROC_PRIMARY' in 'eth_dev_close()'?

Yes, looks to be a miss for this new function.
Thomas Monjalon Sept. 24, 2020, 9:58 p.m. UTC | #3 
23/09/2020 22:47, Thomas Monjalon:
> 23/09/2020 18:44, Ferruh Yigit:
> > On 9/13/2020 11:07 PM, Thomas Monjalon wrote:
> > > The flag RTE_ETH_DEV_CLOSE_REMOVE is set so all port resources
> > > can be freed by rte_eth_dev_close().
> > > 
> > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > ---
> > > --- a/drivers/net/null/rte_eth_null.c
> > > +++ b/drivers/net/null/rte_eth_null.c
> > > +static int
> > > +eth_dev_close(struct rte_eth_dev *dev)
> > > +{
> > > +	PMD_LOG(INFO, "Closing null ethdev on NUMA socket %u",
> > > +			rte_socket_id());
> > > +
> > > +	/* mac_addrs must not be freed alone because part of dev_private */
> > > +	dev->data->mac_addrs = NULL;
> > > +
> > > +	return 0;
> > > +}
> > 
> > should check 'RTE_PROC_PRIMARY' in 'eth_dev_close()'?
> 
> Yes, looks to be a miss for this new function.

Sorry no, this function is not freeing any shared data,
so no restriction on secondary process.
Ferruh Yigit Sept. 25, 2020, 8:52 a.m. UTC | #4 
On 9/24/2020 10:58 PM, Thomas Monjalon wrote:
> 23/09/2020 22:47, Thomas Monjalon:
>> 23/09/2020 18:44, Ferruh Yigit:
>>> On 9/13/2020 11:07 PM, Thomas Monjalon wrote:
>>>> The flag RTE_ETH_DEV_CLOSE_REMOVE is set so all port resources
>>>> can be freed by rte_eth_dev_close().
>>>>
>>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>>>> ---
>>>> --- a/drivers/net/null/rte_eth_null.c
>>>> +++ b/drivers/net/null/rte_eth_null.c
>>>> +static int
>>>> +eth_dev_close(struct rte_eth_dev *dev)
>>>> +{
>>>> +	PMD_LOG(INFO, "Closing null ethdev on NUMA socket %u",
>>>> +			rte_socket_id());
>>>> +
>>>> +	/* mac_addrs must not be freed alone because part of dev_private */
>>>> +	dev->data->mac_addrs = NULL;
>>>> +
>>>> +	return 0;
>>>> +}
>>>
>>> should check 'RTE_PROC_PRIMARY' in 'eth_dev_close()'?
>>
>> Yes, looks to be a miss for this new function.
> 
> Sorry no, this function is not freeing any shared data,
> so no restriction on secondary process.
> 

"dev->data->mac_addrs = NULL", won't this change the shared data?
Thomas Monjalon Sept. 25, 2020, 1:13 p.m. UTC | #5 
25/09/2020 10:52, Ferruh Yigit:
> On 9/24/2020 10:58 PM, Thomas Monjalon wrote:
> > 23/09/2020 22:47, Thomas Monjalon:
> >> 23/09/2020 18:44, Ferruh Yigit:
> >>> On 9/13/2020 11:07 PM, Thomas Monjalon wrote:
> >>>> The flag RTE_ETH_DEV_CLOSE_REMOVE is set so all port resources
> >>>> can be freed by rte_eth_dev_close().
> >>>>
> >>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> >>>> ---
> >>>> --- a/drivers/net/null/rte_eth_null.c
> >>>> +++ b/drivers/net/null/rte_eth_null.c
> >>>> +static int
> >>>> +eth_dev_close(struct rte_eth_dev *dev)
> >>>> +{
> >>>> +	PMD_LOG(INFO, "Closing null ethdev on NUMA socket %u",
> >>>> +			rte_socket_id());
> >>>> +
> >>>> +	/* mac_addrs must not be freed alone because part of dev_private */
> >>>> +	dev->data->mac_addrs = NULL;
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>
> >>> should check 'RTE_PROC_PRIMARY' in 'eth_dev_close()'?
> >>
> >> Yes, looks to be a miss for this new function.
> > 
> > Sorry no, this function is not freeing any shared data,
> > so no restriction on secondary process.
> 
> "dev->data->mac_addrs = NULL", won't this change the shared data?

Absolutely, you're right.
Will fix in v2.
diff mbox series

Patch

diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
index 0ce073fa4b..33997013e4 100644
--- a/drivers/net/null/rte_eth_null.c
+++ b/drivers/net/null/rte_eth_null.c
@@ -458,7 +458,20 @@  eth_mac_address_set(__rte_unused struct rte_eth_dev *dev,
 	return 0;
 }
 
+static int
+eth_dev_close(struct rte_eth_dev *dev)
+{
+	PMD_LOG(INFO, "Closing null ethdev on NUMA socket %u",
+			rte_socket_id());
+
+	/* mac_addrs must not be freed alone because part of dev_private */
+	dev->data->mac_addrs = NULL;
+
+	return 0;
+}
+
 static const struct eth_dev_ops ops = {
+	.dev_close = eth_dev_close,
 	.dev_start = eth_dev_start,
 	.dev_stop = eth_dev_stop,
 	.dev_configure = eth_dev_configure,
@@ -532,6 +545,7 @@  eth_dev_null_create(struct rte_vdev_device *dev, struct pmd_options *args)
 	data->mac_addrs = &internals->eth_addr;
 	data->promiscuous = 1;
 	data->all_multicast = 1;
+	data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
 
 	eth_dev->dev_ops = &ops;
 
@@ -701,18 +715,12 @@  rte_pmd_null_remove(struct rte_vdev_device *dev)
 	if (!dev)
 		return -EINVAL;
 
-	PMD_LOG(INFO, "Closing null ethdev on numa socket %u",
-			rte_socket_id());
-
 	/* find the ethdev entry */
 	eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(dev));
 	if (eth_dev == NULL)
-		return -1;
-
-	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
-		/* mac_addrs must not be freed alone because part of dev_private */
-		eth_dev->data->mac_addrs = NULL;
+		return 0; /* port already released */
 
+	eth_dev_close(eth_dev);
 	rte_eth_dev_release_port(eth_dev);
 
 	return 0;