[dpdk-dev] [PATCH] net/tap: release port upon close

[dpdk-dev] [PATCH] net/tap: release port upon close

[wangyunjian]

From: Yunjian Wang <wangyunjian@huawei.com>

Set RTE_ETH_DEV_CLOSE_REMOVE upon probe so all the private
resources for the port can be freed by rte_eth_dev_close().

Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
---
 drivers/net/tap/rte_eth_tap.c | 42 ++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 339f24bf8..e059f7f2f 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -72,6 +72,10 @@
 
 static int tap_devices_count;
 
+static const char *tuntap_types[ETH_TUNTAP_TYPE_MAX] = {
+	"UNKNOWN", "TUN", "TAP"
+};
+
 static const char *valid_arguments[] = {
 	ETH_TAP_IFACE_ARG,
 	ETH_TAP_REMOTE_ARG,
@@ -1040,6 +1044,9 @@ tap_dev_close(struct rte_eth_dev *dev)
 	struct pmd_process_private *process_private = dev->process_private;
 	struct rx_queue *rxq;
 
+	if (process_private == NULL)
+		return;
+
 	tap_link_set_down(dev);
 	if (internals->nlsk_fd != -1) {
 		tap_flow_flush(dev, NULL);
@@ -1078,6 +1085,23 @@ tap_dev_close(struct rte_eth_dev *dev)
 	 * Since TUN device has no more opened file descriptors
 	 * it will be removed from kernel
 	 */
+
+	/* mac_addrs must not be freed alone because part of dev_private */
+	dev->data->mac_addrs = NULL;
+
+	internals = dev->data->dev_private;
+	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
+		tuntap_types[internals->type], rte_socket_id());
+
+	if (internals->ioctl_sock != -1) {
+		close(internals->ioctl_sock);
+		internals->ioctl_sock = -1;
+	}
+	rte_free(dev->process_private);
+	dev->process_private = NULL;
+	if (tap_devices_count == 1)
+		rte_mp_action_unregister(TAP_MP_KEY);
+	tap_devices_count--;
 }
 
 static void
@@ -1800,10 +1824,6 @@ static const struct eth_dev_ops ops = {
 	.filter_ctrl            = tap_dev_filter_ctrl,
 };
 
-static const char *tuntap_types[ETH_TUNTAP_TYPE_MAX] = {
-	"UNKNOWN", "TUN", "TAP"
-};
-
 static int
 eth_dev_tap_create(struct rte_vdev_device *vdev, const char *tap_name,
 		   char *remote_iface, struct rte_ether_addr *mac_addr,
@@ -1854,7 +1874,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, const char *tap_name,
 	/* Setup some default values */
 	data = dev->data;
 	data->dev_private = pmd;
-	data->dev_flags = RTE_ETH_DEV_INTR_LSC;
+	data->dev_flags = RTE_ETH_DEV_INTR_LSC | RTE_ETH_DEV_CLOSE_REMOVE;
 	data->numa_node = numa_node;
 
 	data->dev_link = pmd_link;
@@ -2446,12 +2466,11 @@ static int
 rte_pmd_tap_remove(struct rte_vdev_device *dev)
 {
 	struct rte_eth_dev *eth_dev = NULL;
-	struct pmd_internals *internals;
 
 	/* find the ethdev entry */
 	eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(dev));
 	if (!eth_dev)
-		return -ENODEV;
+		return 0;
 
 	/* mac_addrs must not be freed alone because part of dev_private */
 	eth_dev->data->mac_addrs = NULL;
@@ -2461,15 +2480,6 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
 
 	tap_dev_close(eth_dev);
 
-	internals = eth_dev->data->dev_private;
-	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
-		tuntap_types[internals->type], rte_socket_id());
-
-	close(internals->ioctl_sock);
-	rte_free(eth_dev->process_private);
-	if (tap_devices_count == 1)
-		rte_mp_action_unregister(TAP_MP_KEY);
-	tap_devices_count--;
 	rte_eth_dev_release_port(eth_dev);
 
 	return 0;
-- 
2.23.0

Re: [dpdk-dev] [PATCH] net/tap: release port upon close

[Thomas Monjalon]

Please next time, use --cc-cmd devtools/get-maintainer.sh
so you will send it to Keith (Cc'ed) for review.


28/08/2020 14:37, wangyunjian:
> From: Yunjian Wang <wangyunjian@huawei.com>
> 
> Set RTE_ETH_DEV_CLOSE_REMOVE upon probe so all the private
> resources for the port can be freed by rte_eth_dev_close().
> 
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> ---
>  drivers/net/tap/rte_eth_tap.c | 42 ++++++++++++++++++++++-------------
>  1 file changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 339f24bf8..e059f7f2f 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -72,6 +72,10 @@
>  
>  static int tap_devices_count;
>  
> +static const char *tuntap_types[ETH_TUNTAP_TYPE_MAX] = {
> +	"UNKNOWN", "TUN", "TAP"
> +};
> +
>  static const char *valid_arguments[] = {
>  	ETH_TAP_IFACE_ARG,
>  	ETH_TAP_REMOTE_ARG,
> @@ -1040,6 +1044,9 @@ tap_dev_close(struct rte_eth_dev *dev)
>  	struct pmd_process_private *process_private = dev->process_private;
>  	struct rx_queue *rxq;
>  
> +	if (process_private == NULL)
> +		return;
> +
>  	tap_link_set_down(dev);
>  	if (internals->nlsk_fd != -1) {
>  		tap_flow_flush(dev, NULL);
> @@ -1078,6 +1085,23 @@ tap_dev_close(struct rte_eth_dev *dev)
>  	 * Since TUN device has no more opened file descriptors
>  	 * it will be removed from kernel
>  	 */
> +
> +	/* mac_addrs must not be freed alone because part of dev_private */
> +	dev->data->mac_addrs = NULL;
> +
> +	internals = dev->data->dev_private;
> +	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
> +		tuntap_types[internals->type], rte_socket_id());
> +
> +	if (internals->ioctl_sock != -1) {
> +		close(internals->ioctl_sock);
> +		internals->ioctl_sock = -1;
> +	}
> +	rte_free(dev->process_private);
> +	dev->process_private = NULL;
> +	if (tap_devices_count == 1)
> +		rte_mp_action_unregister(TAP_MP_KEY);
> +	tap_devices_count--;
>  }
>  
>  static void
> @@ -1800,10 +1824,6 @@ static const struct eth_dev_ops ops = {
>  	.filter_ctrl            = tap_dev_filter_ctrl,
>  };
>  
> -static const char *tuntap_types[ETH_TUNTAP_TYPE_MAX] = {
> -	"UNKNOWN", "TUN", "TAP"
> -};
> -
>  static int
>  eth_dev_tap_create(struct rte_vdev_device *vdev, const char *tap_name,
>  		   char *remote_iface, struct rte_ether_addr *mac_addr,
> @@ -1854,7 +1874,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, const char *tap_name,
>  	/* Setup some default values */
>  	data = dev->data;
>  	data->dev_private = pmd;
> -	data->dev_flags = RTE_ETH_DEV_INTR_LSC;
> +	data->dev_flags = RTE_ETH_DEV_INTR_LSC | RTE_ETH_DEV_CLOSE_REMOVE;
>  	data->numa_node = numa_node;
>  
>  	data->dev_link = pmd_link;
> @@ -2446,12 +2466,11 @@ static int
>  rte_pmd_tap_remove(struct rte_vdev_device *dev)
>  {
>  	struct rte_eth_dev *eth_dev = NULL;
> -	struct pmd_internals *internals;
>  
>  	/* find the ethdev entry */
>  	eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(dev));
>  	if (!eth_dev)
> -		return -ENODEV;
> +		return 0;
>  
>  	/* mac_addrs must not be freed alone because part of dev_private */
>  	eth_dev->data->mac_addrs = NULL;
> @@ -2461,15 +2480,6 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
>  
>  	tap_dev_close(eth_dev);
>  
> -	internals = eth_dev->data->dev_private;
> -	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
> -		tuntap_types[internals->type], rte_socket_id());
> -
> -	close(internals->ioctl_sock);
> -	rte_free(eth_dev->process_private);
> -	if (tap_devices_count == 1)
> -		rte_mp_action_unregister(TAP_MP_KEY);
> -	tap_devices_count--;
>  	rte_eth_dev_release_port(eth_dev);
>  
>  	return 0;
> 

Re: [dpdk-dev] [PATCH] net/tap: release port upon close

[Thomas Monjalon]

Hi,

28/08/2020 14:37, wangyunjian:
> @@ -1078,6 +1085,23 @@ tap_dev_close(struct rte_eth_dev *dev)
> +
> +	/* mac_addrs must not be freed alone because part of dev_private */
> +	dev->data->mac_addrs = NULL;
> +
> +	internals = dev->data->dev_private;
> +	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
> +		tuntap_types[internals->type], rte_socket_id());
> +
> +	if (internals->ioctl_sock != -1) {
> +		close(internals->ioctl_sock);
> +		internals->ioctl_sock = -1;
> +	}
> +	rte_free(dev->process_private);
> +	dev->process_private = NULL;
> +	if (tap_devices_count == 1)
> +		rte_mp_action_unregister(TAP_MP_KEY);
> +	tap_devices_count--;
>  }
[...]
> @@ -2446,12 +2466,11 @@ static int
>  rte_pmd_tap_remove(struct rte_vdev_device *dev)
>  {
>  
>         struct rte_eth_dev *eth_dev = NULL;
> 
> -       struct pmd_internals *internals;
> 
>         /* find the ethdev entry */
>         eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(dev));
>         if (!eth_dev)
> 
> -               return -ENODEV;
> +               return 0;
> 
>         /* mac_addrs must not be freed alone because part of dev_private */
>         eth_dev->data->mac_addrs = NULL;

The line above can be removed because mac_addrs is not freed
in secondary process, and it is redundant with tap_dev_close().

>  
> 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>  		return rte_eth_dev_release_port(eth_dev);

There is an inconsistency in secondary process
if tap_dev_close() is not called from .remove (unplug)
but can be called from .dev_close (rte_eth_dev_close).

I think the process type check must be done inside tap_dev_close(),
so the process private resources can be freed.

> 	
>  	tap_dev_close(eth_dev);
>  

This blank line can be removed in my opinion.

> -	internals = eth_dev->data->dev_private;
> -	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
> -		tuntap_types[internals->type], rte_socket_id());
> -
> -	close(internals->ioctl_sock);
> -	rte_free(eth_dev->process_private);
> -	if (tap_devices_count == 1)
> -		rte_mp_action_unregister(TAP_MP_KEY);
> -	tap_devices_count--;
>  	rte_eth_dev_release_port(eth_dev);
>  
>  	return 0;

Re: [dpdk-dev] [PATCH] net/tap: release port upon close

[Thomas Monjalon]

As you may notice, I have included a slightly modified version
of this patch in my series in order to cover the full picture:
	https://patches.dpdk.org/patch/77566/

Feel free to continue improving your patch in this thread or the other,
as you prefer, as long as the secondary process issue is adressed.

Thanks


13/09/2020 11:16, Thomas Monjalon:
> Hi,
> 
> 28/08/2020 14:37, wangyunjian:
> > @@ -1078,6 +1085,23 @@ tap_dev_close(struct rte_eth_dev *dev)
> > +
> > +	/* mac_addrs must not be freed alone because part of dev_private */
> > +	dev->data->mac_addrs = NULL;
> > +
> > +	internals = dev->data->dev_private;
> > +	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
> > +		tuntap_types[internals->type], rte_socket_id());
> > +
> > +	if (internals->ioctl_sock != -1) {
> > +		close(internals->ioctl_sock);
> > +		internals->ioctl_sock = -1;
> > +	}
> > +	rte_free(dev->process_private);
> > +	dev->process_private = NULL;
> > +	if (tap_devices_count == 1)
> > +		rte_mp_action_unregister(TAP_MP_KEY);
> > +	tap_devices_count--;
> >  }
> [...]
> > @@ -2446,12 +2466,11 @@ static int
> >  rte_pmd_tap_remove(struct rte_vdev_device *dev)
> >  {
> >  
> >         struct rte_eth_dev *eth_dev = NULL;
> > 
> > -       struct pmd_internals *internals;
> > 
> >         /* find the ethdev entry */
> >         eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(dev));
> >         if (!eth_dev)
> > 
> > -               return -ENODEV;
> > +               return 0;
> > 
> >         /* mac_addrs must not be freed alone because part of dev_private */
> >         eth_dev->data->mac_addrs = NULL;
> 
> The line above can be removed because mac_addrs is not freed
> in secondary process, and it is redundant with tap_dev_close().
> 
> >  
> > 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> >  		return rte_eth_dev_release_port(eth_dev);
> 
> There is an inconsistency in secondary process
> if tap_dev_close() is not called from .remove (unplug)
> but can be called from .dev_close (rte_eth_dev_close).
> 
> I think the process type check must be done inside tap_dev_close(),
> so the process private resources can be freed.
> 
> > 	
> >  	tap_dev_close(eth_dev);
> >  
> 
> This blank line can be removed in my opinion.
> 
> > -	internals = eth_dev->data->dev_private;
> > -	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
> > -		tuntap_types[internals->type], rte_socket_id());
> > -
> > -	close(internals->ioctl_sock);
> > -	rte_free(eth_dev->process_private);
> > -	if (tap_devices_count == 1)
> > -		rte_mp_action_unregister(TAP_MP_KEY);
> > -	tap_devices_count--;
> >  	rte_eth_dev_release_port(eth_dev);
> >  
> >  	return 0;

Re: [dpdk-dev] [PATCH] net/tap: release port upon close

[Ferruh Yigit]

On 8/28/2020 1:37 PM, wangyunjian wrote:
> From: Yunjian Wang <wangyunjian@huawei.com>
> 
> Set RTE_ETH_DEV_CLOSE_REMOVE upon probe so all the private
> resources for the port can be freed by rte_eth_dev_close().
> 
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>

<...>

> @@ -1040,6 +1044,9 @@ tap_dev_close(struct rte_eth_dev *dev)
>  	struct pmd_process_private *process_private = dev->process_private;
>  	struct rx_queue *rxq;
>  
> +	if (process_private == NULL)
> +		return;

Why this check is required?

Re: [dpdk-dev] [PATCH] net/tap: release port upon close

[wangyunjian]

> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Tuesday, September 15, 2020 10:53 PM
> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; Lilijun (Jerry) <jerry.lilijun@huawei.com>; xudingke
> <xudingke@huawei.com>
> Subject: Re: [dpdk-dev] [PATCH] net/tap: release port upon close
> 
> On 8/28/2020 1:37 PM, wangyunjian wrote:
> > From: Yunjian Wang <wangyunjian@huawei.com>
> >
> > Set RTE_ETH_DEV_CLOSE_REMOVE upon probe so all the private resources
> > for the port can be freed by rte_eth_dev_close().
> >
> > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> 
> <...>
> 
> > @@ -1040,6 +1044,9 @@ tap_dev_close(struct rte_eth_dev *dev)
> >  	struct pmd_process_private *process_private = dev->process_private;
> >  	struct rx_queue *rxq;
> >
> > +	if (process_private == NULL)
> > +		return;
> 
> Why this check is required?

When user first call 'close()' and later call 'remove()' the tap PMD, in this case,
the tap_dev_close() will be called twice. The second call of tap_dev_close()
shouldn't do any process, we can use this check to return immediately.

Thanks,
Yunjian

Re: [dpdk-dev] [PATCH] net/tap: release port upon close

[Ferruh Yigit]

On 9/16/2020 8:20 AM, wangyunjian wrote:
>> -----Original Message-----
>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>> Sent: Tuesday, September 15, 2020 10:53 PM
>> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org
>> Cc: thomas@monjalon.net; Lilijun (Jerry) <jerry.lilijun@huawei.com>; xudingke
>> <xudingke@huawei.com>
>> Subject: Re: [dpdk-dev] [PATCH] net/tap: release port upon close
>>
>> On 8/28/2020 1:37 PM, wangyunjian wrote:
>>> From: Yunjian Wang <wangyunjian@huawei.com>
>>>
>>> Set RTE_ETH_DEV_CLOSE_REMOVE upon probe so all the private resources
>>> for the port can be freed by rte_eth_dev_close().
>>>
>>> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
>>
>> <...>
>>
>>> @@ -1040,6 +1044,9 @@ tap_dev_close(struct rte_eth_dev *dev)
>>>   	struct pmd_process_private *process_private = dev->process_private;
>>>   	struct rx_queue *rxq;
>>>
>>> +	if (process_private == NULL)
>>> +		return;
>>
>> Why this check is required?
> 
> When user first call 'close()' and later call 'remove()' the tap PMD, in this case,
> the tap_dev_close() will be called twice. The second call of tap_dev_close()
> shouldn't do any process, we can use this check to return immediately.
> 


When first call is 'close()', it memset the 'eth_dev->data', so the in 
the later 'remove()' call, 'rte_eth_dev_allocated()' will always return 
NULL and 'remove()' will exit without calling 'close()'.

Also multiple 'close()' calls look safe, because of 
'RTE_ETH_VALID_PORTID_OR_RET()' checks is 'rte_eth_dev_close()'.

So, as far as I can see this additional check is not needed, but please 
double check.

Re: [dpdk-dev] [PATCH] net/tap: release port upon close

[wangyunjian]

> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Wednesday, September 16, 2020 11:48 PM
> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; Lilijun (Jerry) <jerry.lilijun@huawei.com>; xudingke
> <xudingke@huawei.com>
> Subject: Re: [dpdk-dev] [PATCH] net/tap: release port upon close
> 
> On 9/16/2020 8:20 AM, wangyunjian wrote:
> >> -----Original Message-----
> >> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> >> Sent: Tuesday, September 15, 2020 10:53 PM
> >> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org
> >> Cc: thomas@monjalon.net; Lilijun (Jerry) <jerry.lilijun@huawei.com>;
> >> xudingke <xudingke@huawei.com>
> >> Subject: Re: [dpdk-dev] [PATCH] net/tap: release port upon close
> >>
> >> On 8/28/2020 1:37 PM, wangyunjian wrote:
> >>> From: Yunjian Wang <wangyunjian@huawei.com>
> >>>
> >>> Set RTE_ETH_DEV_CLOSE_REMOVE upon probe so all the private
> resources
> >>> for the port can be freed by rte_eth_dev_close().
> >>>
> >>> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> >>
> >> <...>
> >>
> >>> @@ -1040,6 +1044,9 @@ tap_dev_close(struct rte_eth_dev *dev)
> >>>   	struct pmd_process_private *process_private =
> dev->process_private;
> >>>   	struct rx_queue *rxq;
> >>>
> >>> +	if (process_private == NULL)
> >>> +		return;
> >>
> >> Why this check is required?
> >
> > When user first call 'close()' and later call 'remove()' the tap PMD,
> > in this case, the tap_dev_close() will be called twice. The second
> > call of tap_dev_close() shouldn't do any process, we can use this check to
> return immediately.
> >
> 
> 
> When first call is 'close()', it memset the 'eth_dev->data', so the in the later
> 'remove()' call, 'rte_eth_dev_allocated()' will always return NULL and 'remove()'
> will exit without calling 'close()'.
> 
> Also multiple 'close()' calls look safe, because of
> 'RTE_ETH_VALID_PORTID_OR_RET()' checks is 'rte_eth_dev_close()'.
> 
> So, as far as I can see this additional check is not needed, but please double
> check.

I have checked, you're right. This additional check is not needed.
I will remove it in V2.

Thanks,
Yunjian

[dpdk-dev] [PATCH v2] net/tap: release port upon close

[wangyunjian]

From: Yunjian Wang <wangyunjian@huawei.com>

Set RTE_ETH_DEV_CLOSE_REMOVE upon probe so all the private
resources for the port can be freed by rte_eth_dev_close().

Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
---
v2:
   Remove redundant check and assignment suggested
   by Ferruh Yigit and Thomas Monjalon
---
 drivers/net/tap/rte_eth_tap.c | 43 +++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 339f24bf8..3844994a3 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -72,6 +72,10 @@
 
 static int tap_devices_count;
 
+static const char *tuntap_types[ETH_TUNTAP_TYPE_MAX] = {
+	"UNKNOWN", "TUN", "TAP"
+};
+
 static const char *valid_arguments[] = {
 	ETH_TAP_IFACE_ARG,
 	ETH_TAP_REMOTE_ARG,
@@ -1074,6 +1078,23 @@ tap_dev_close(struct rte_eth_dev *dev)
 		close(internals->ka_fd);
 		internals->ka_fd = -1;
 	}
+
+	/* mac_addrs must not be freed alone because part of dev_private */
+	dev->data->mac_addrs = NULL;
+
+	internals = dev->data->dev_private;
+	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
+		tuntap_types[internals->type], rte_socket_id());
+
+	if (internals->ioctl_sock != -1) {
+		close(internals->ioctl_sock);
+		internals->ioctl_sock = -1;
+	}
+	rte_free(dev->process_private);
+	dev->process_private = NULL;
+	if (tap_devices_count == 1)
+		rte_mp_action_unregister(TAP_MP_KEY);
+	tap_devices_count--;
 	/*
 	 * Since TUN device has no more opened file descriptors
 	 * it will be removed from kernel
@@ -1800,10 +1821,6 @@ static const struct eth_dev_ops ops = {
 	.filter_ctrl            = tap_dev_filter_ctrl,
 };
 
-static const char *tuntap_types[ETH_TUNTAP_TYPE_MAX] = {
-	"UNKNOWN", "TUN", "TAP"
-};
-
 static int
 eth_dev_tap_create(struct rte_vdev_device *vdev, const char *tap_name,
 		   char *remote_iface, struct rte_ether_addr *mac_addr,
@@ -1854,7 +1871,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, const char *tap_name,
 	/* Setup some default values */
 	data = dev->data;
 	data->dev_private = pmd;
-	data->dev_flags = RTE_ETH_DEV_INTR_LSC;
+	data->dev_flags = RTE_ETH_DEV_INTR_LSC | RTE_ETH_DEV_CLOSE_REMOVE;
 	data->numa_node = numa_node;
 
 	data->dev_link = pmd_link;
@@ -2446,30 +2463,16 @@ static int
 rte_pmd_tap_remove(struct rte_vdev_device *dev)
 {
 	struct rte_eth_dev *eth_dev = NULL;
-	struct pmd_internals *internals;
 
 	/* find the ethdev entry */
 	eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(dev));
 	if (!eth_dev)
-		return -ENODEV;
-
-	/* mac_addrs must not be freed alone because part of dev_private */
-	eth_dev->data->mac_addrs = NULL;
+		return 0;
 
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return rte_eth_dev_release_port(eth_dev);
 
 	tap_dev_close(eth_dev);
-
-	internals = eth_dev->data->dev_private;
-	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
-		tuntap_types[internals->type], rte_socket_id());
-
-	close(internals->ioctl_sock);
-	rte_free(eth_dev->process_private);
-	if (tap_devices_count == 1)
-		rte_mp_action_unregister(TAP_MP_KEY);
-	tap_devices_count--;
 	rte_eth_dev_release_port(eth_dev);
 
 	return 0;
-- 
2.23.0

Re: [dpdk-dev] [PATCH] net/tap: release port upon close

[wangyunjian]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Sunday, September 13, 2020 5:17 PM
> To: wangyunjian <wangyunjian@huawei.com>
> Cc: dev@dpdk.org; ferruh.yigit@intel.com; Lilijun (Jerry)
> <jerry.lilijun@huawei.com>; xudingke <xudingke@huawei.com>;
> keith.wiles@intel.com
> Subject: Re: [dpdk-dev] [PATCH] net/tap: release port upon close
> 
> Hi,
> 
> 28/08/2020 14:37, wangyunjian:
> > @@ -1078,6 +1085,23 @@ tap_dev_close(struct rte_eth_dev *dev)
> > +
> > +	/* mac_addrs must not be freed alone because part of dev_private */
> > +	dev->data->mac_addrs = NULL;
> > +
> > +	internals = dev->data->dev_private;
> > +	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
> > +		tuntap_types[internals->type], rte_socket_id());
> > +
> > +	if (internals->ioctl_sock != -1) {
> > +		close(internals->ioctl_sock);
> > +		internals->ioctl_sock = -1;
> > +	}
> > +	rte_free(dev->process_private);
> > +	dev->process_private = NULL;
> > +	if (tap_devices_count == 1)
> > +		rte_mp_action_unregister(TAP_MP_KEY);
> > +	tap_devices_count--;
> >  }
> [...]
> > @@ -2446,12 +2466,11 @@ static int
> >  rte_pmd_tap_remove(struct rte_vdev_device *dev)  {
> >
> >         struct rte_eth_dev *eth_dev = NULL;
> >
> > -       struct pmd_internals *internals;
> >
> >         /* find the ethdev entry */
> >         eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(dev));
> >         if (!eth_dev)
> >
> > -               return -ENODEV;
> > +               return 0;
> >
> >         /* mac_addrs must not be freed alone because part of dev_private
> */
> >         eth_dev->data->mac_addrs = NULL;
> 
> The line above can be removed because mac_addrs is not freed in secondary
> process, and it is redundant with tap_dev_close().

Thanks for the suggestion.
I have fixed it in v2.
https://patchwork.dpdk.org/patch/78048/

> 
> >
> > 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> >  		return rte_eth_dev_release_port(eth_dev);
> 
> There is an inconsistency in secondary process if tap_dev_close() is not called
> from .remove (unplug) but can be called from .dev_close (rte_eth_dev_close).
> 
> I think the process type check must be done inside tap_dev_close(), so the
> process private resources can be freed.
> 
> >
> >  	tap_dev_close(eth_dev);
> >
> 
> This blank line can be removed in my opinion.

OK

> 
> > -	internals = eth_dev->data->dev_private;
> > -	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
> > -		tuntap_types[internals->type], rte_socket_id());
> > -
> > -	close(internals->ioctl_sock);
> > -	rte_free(eth_dev->process_private);
> > -	if (tap_devices_count == 1)
> > -		rte_mp_action_unregister(TAP_MP_KEY);
> > -	tap_devices_count--;
> >  	rte_eth_dev_release_port(eth_dev);
> >
> >  	return 0;
> 
> 

Re: [dpdk-dev] [PATCH v2] net/tap: release port upon close

[Ferruh Yigit]

On 9/17/2020 12:47 PM, wangyunjian wrote:
> From: Yunjian Wang <wangyunjian@huawei.com>
> 
> Set RTE_ETH_DEV_CLOSE_REMOVE upon probe so all the private
> resources for the port can be freed by rte_eth_dev_close().
> 
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> ---
> v2:
>     Remove redundant check and assignment suggested
>     by Ferruh Yigit and Thomas Monjalon

Hi Yunjian,

There is a newer version of the tap close in the patchset related to the ethdev 
close updates:
https://patches.dpdk.org/patch/79086/

I will update this one as 'Superseded', can you please check the above one?

Thanks,
ferruh

Re: [dpdk-dev] [PATCH v2] net/tap: release port upon close

[wangyunjian]

> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Wednesday, September 30, 2020 12:56 AM
> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org
> Cc: keith.wiles@intel.com; thomas@monjalon.net; Lilijun (Jerry)
> <jerry.lilijun@huawei.com>; xudingke <xudingke@huawei.com>
> Subject: Re: [dpdk-dev] [PATCH v2] net/tap: release port upon close
> 
> On 9/17/2020 12:47 PM, wangyunjian wrote:
> > From: Yunjian Wang <wangyunjian@huawei.com>
> >
> > Set RTE_ETH_DEV_CLOSE_REMOVE upon probe so all the private resources
> > for the port can be freed by rte_eth_dev_close().
> >
> > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> > ---
> > v2:
> >     Remove redundant check and assignment suggested
> >     by Ferruh Yigit and Thomas Monjalon
> 
> Hi Yunjian,
> 
> There is a newer version of the tap close in the patchset related to the ethdev
> close updates:
> https://patches.dpdk.org/patch/79086/
> 
> I will update this one as 'Superseded', can you please check the above one?
> 

OK, I will check and test it.

Thanks,
Yunjian

> Thanks,
> ferruh