* Re: [dpdk-dev] [PATCH] net/tap: release port upon close
2020-08-28 12:37 [dpdk-dev] [PATCH] net/tap: release port upon close wangyunjian
@ 2020-09-01 10:59 ` Thomas Monjalon
2020-09-13 9:16 ` Thomas Monjalon
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Thomas Monjalon @ 2020-09-01 10:59 UTC (permalink / raw)
To: wangyunjian
Cc: dev, ferruh.yigit, jerry.lilijun, xudingke, Yunjian Wang, Keith Wiles
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;
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [dpdk-dev] [PATCH] net/tap: release port upon close
2020-08-28 12:37 [dpdk-dev] [PATCH] net/tap: release port upon close wangyunjian
2020-09-01 10:59 ` Thomas Monjalon
@ 2020-09-13 9:16 ` Thomas Monjalon
2020-09-13 22:27 ` Thomas Monjalon
2020-09-17 12:35 ` wangyunjian
2020-09-15 14:52 ` Ferruh Yigit
2020-09-17 11:47 ` [dpdk-dev] [PATCH v2] " wangyunjian
3 siblings, 2 replies; 12+ messages in thread
From: Thomas Monjalon @ 2020-09-13 9:16 UTC (permalink / raw)
To: Yunjian Wang; +Cc: dev, ferruh.yigit, jerry.lilijun, xudingke, keith.wiles
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;
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [dpdk-dev] [PATCH] net/tap: release port upon close
2020-09-13 9:16 ` Thomas Monjalon
@ 2020-09-13 22:27 ` Thomas Monjalon
2020-09-17 12:35 ` wangyunjian
1 sibling, 0 replies; 12+ messages in thread
From: Thomas Monjalon @ 2020-09-13 22:27 UTC (permalink / raw)
To: Yunjian Wang; +Cc: dev, ferruh.yigit, jerry.lilijun, xudingke, keith.wiles
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;
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [dpdk-dev] [PATCH] net/tap: release port upon close
2020-09-13 9:16 ` Thomas Monjalon
2020-09-13 22:27 ` Thomas Monjalon
@ 2020-09-17 12:35 ` wangyunjian
1 sibling, 0 replies; 12+ messages in thread
From: wangyunjian @ 2020-09-17 12:35 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, ferruh.yigit, Lilijun (Jerry), xudingke, keith.wiles
> -----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;
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH] net/tap: release port upon close
2020-08-28 12:37 [dpdk-dev] [PATCH] net/tap: release port upon close wangyunjian
2020-09-01 10:59 ` Thomas Monjalon
2020-09-13 9:16 ` Thomas Monjalon
@ 2020-09-15 14:52 ` Ferruh Yigit
2020-09-16 7:20 ` wangyunjian
2020-09-17 11:47 ` [dpdk-dev] [PATCH v2] " wangyunjian
3 siblings, 1 reply; 12+ messages in thread
From: Ferruh Yigit @ 2020-09-15 14:52 UTC (permalink / raw)
To: wangyunjian, dev; +Cc: thomas, jerry.lilijun, xudingke
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?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH] net/tap: release port upon close
2020-09-15 14:52 ` Ferruh Yigit
@ 2020-09-16 7:20 ` wangyunjian
2020-09-16 15:48 ` Ferruh Yigit
0 siblings, 1 reply; 12+ messages in thread
From: wangyunjian @ 2020-09-16 7:20 UTC (permalink / raw)
To: Ferruh Yigit, dev; +Cc: thomas, Lilijun (Jerry), xudingke
> -----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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH] net/tap: release port upon close
2020-09-16 7:20 ` wangyunjian
@ 2020-09-16 15:48 ` Ferruh Yigit
2020-09-17 9:18 ` wangyunjian
0 siblings, 1 reply; 12+ messages in thread
From: Ferruh Yigit @ 2020-09-16 15:48 UTC (permalink / raw)
To: wangyunjian, dev; +Cc: thomas, Lilijun (Jerry), xudingke
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH] net/tap: release port upon close
2020-09-16 15:48 ` Ferruh Yigit
@ 2020-09-17 9:18 ` wangyunjian
0 siblings, 0 replies; 12+ messages in thread
From: wangyunjian @ 2020-09-17 9:18 UTC (permalink / raw)
To: Ferruh Yigit, dev; +Cc: thomas, Lilijun (Jerry), xudingke
> -----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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [dpdk-dev] [PATCH v2] net/tap: release port upon close
2020-08-28 12:37 [dpdk-dev] [PATCH] net/tap: release port upon close wangyunjian
` (2 preceding siblings ...)
2020-09-15 14:52 ` Ferruh Yigit
@ 2020-09-17 11:47 ` wangyunjian
2020-09-29 16:56 ` Ferruh Yigit
3 siblings, 1 reply; 12+ messages in thread
From: wangyunjian @ 2020-09-17 11:47 UTC (permalink / raw)
To: dev
Cc: keith.wiles, ferruh.yigit, thomas, jerry.lilijun, xudingke, Yunjian Wang
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [dpdk-dev] [PATCH v2] net/tap: release port upon close
2020-09-17 11:47 ` [dpdk-dev] [PATCH v2] " wangyunjian
@ 2020-09-29 16:56 ` Ferruh Yigit
2020-09-30 0:58 ` wangyunjian
0 siblings, 1 reply; 12+ messages in thread
From: Ferruh Yigit @ 2020-09-29 16:56 UTC (permalink / raw)
To: wangyunjian, dev; +Cc: keith.wiles, thomas, jerry.lilijun, xudingke
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/tap: release port upon close
2020-09-29 16:56 ` Ferruh Yigit
@ 2020-09-30 0:58 ` wangyunjian
0 siblings, 0 replies; 12+ messages in thread
From: wangyunjian @ 2020-09-30 0:58 UTC (permalink / raw)
To: Ferruh Yigit, dev; +Cc: keith.wiles, thomas, Lilijun (Jerry), xudingke
> -----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
^ permalink raw reply [flat|nested] 12+ messages in thread