From: Thomas Monjalon <thomas@monjalon.net>
To: Yunjian Wang <wangyunjian@huawei.com>
Cc: dev@dpdk.org, ferruh.yigit@intel.com, jerry.lilijun@huawei.com,
xudingke@huawei.com, keith.wiles@intel.com
Subject: Re: [dpdk-dev] [PATCH] net/tap: release port upon close
Date: Sun, 13 Sep 2020 11:16:36 +0200 [thread overview]
Message-ID: <2500283.dqYlU2TMh5@thomas> (raw)
In-Reply-To: <9a94f272bc7c1e5650295ddf3e02fb19b15e7951.1598596042.git.wangyunjian@huawei.com>
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;
next prev parent reply other threads:[~2020-09-13 9:16 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-28 12:37 wangyunjian
2020-09-01 10:59 ` Thomas Monjalon
2020-09-13 9:16 ` Thomas Monjalon [this message]
2020-09-13 22:27 ` Thomas Monjalon
2020-09-17 12:35 ` wangyunjian
2020-09-15 14:52 ` Ferruh Yigit
2020-09-16 7:20 ` wangyunjian
2020-09-16 15:48 ` Ferruh Yigit
2020-09-17 9:18 ` wangyunjian
2020-09-17 11:47 ` [dpdk-dev] [PATCH v2] " wangyunjian
2020-09-29 16:56 ` Ferruh Yigit
2020-09-30 0:58 ` wangyunjian
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2500283.dqYlU2TMh5@thomas \
--to=thomas@monjalon.net \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=jerry.lilijun@huawei.com \
--cc=keith.wiles@intel.com \
--cc=wangyunjian@huawei.com \
--cc=xudingke@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).