net/tap: release port upon close
Checks
Commit Message
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(-)
Comments
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;
>
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;
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;
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?
> -----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
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.
> -----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
> -----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;
>
>
@@ -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;