From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id E84807CEB for ; Thu, 18 Oct 2018 03:24:11 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 94894220FD; Wed, 17 Oct 2018 21:24:11 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Wed, 17 Oct 2018 21:24:11 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; s=mesmtp; bh=KE0dGl98z0 VrZzZvGEzwdKx9Qev7ImSMKQmPZgdayjA=; b=HpYbYm3Lu+b6HRdHzN2oK6IprJ R/PfmTWZ8n4McV08122StLfg+DqSgaAzaL2acpmAd6pZDt9aqAl0/+cXv+AmWEf0 P6qMDeFeooc/gAR3AznKCnHeWUz8seZH4p0oI69pizxGuzlKWnt3qCpn01cyPGDR aAjGL6824N4YqgAQQ= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:date:from :in-reply-to:message-id:mime-version:references:subject:to :x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm1; bh=KE0dGl98z0VrZzZvGEzwdKx9Qev7ImSMKQmPZgdayjA=; b=rDOcDT2m I8cp3t9ybXdoNGu1nKV6jat32eeSQa9Tf5/o95ffZFP8c/of5SWeZAb2jf3aqbij nhTt5aLLRILTMDwyCAXodq4aT6bE57EgsFK211OlV63k0ahhAI88Ddevs2s3drQo Qbse8ySHUJG4AXyaWo/MwDc0wmEWaDUQUpbfWM1hSyCY3lpL0HvaEMLlzadhcCfR YD3NjRLVHmWnVf+z6+ytSWfkLbZHjuxE/bjueWeDcHU/IhpZevxqQd1srO1dnTQJ rGxLpVw/a1tr+gThoUf6UlbDH4d8w6eTwNx7eWwXtkmbeDJMkdBcgStl3vjS5zki uXg7z6rN5SsYXQ== X-ME-Sender: X-ME-Proxy: Received: from xps.monjalon.net (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 78871102DE; Wed, 17 Oct 2018 21:24:10 -0400 (EDT) From: Thomas Monjalon To: ferruh.yigit@intel.com, arybchenko@solarflare.com Cc: dev@dpdk.org, ophirmu@mellanox.com, bernard.iremonger@intel.com, rahul.lakkireddy@chelsio.com Date: Thu, 18 Oct 2018 03:24:02 +0200 Message-Id: <20181018012402.1240-7-thomas@monjalon.net> X-Mailer: git-send-email 2.19.0 In-Reply-To: <20181018012402.1240-1-thomas@monjalon.net> References: <20180907233929.21950-1-thomas@monjalon.net> <20181018012402.1240-1-thomas@monjalon.net> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [dpdk-dev] [PATCH v5 6/6] ethdev: complete closing of port X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 18 Oct 2018 01:24:12 -0000 After closing a port, it cannot be restarted. So there is no reason to not free all associated resources. The last step was done with rte_eth_dev_detach() which is deprecated. Instead of blindly removing the associated rte_device, the driver should check if no more port (ethdev, cryptodev, etc) is open for the device. The last ethdev freeing which were done by rte_eth_dev_detach(), are now done at the end of rte_eth_dev_close(). Some drivers does not allocate MAC addresses dynamically or separately. In those cases, the pointer is set to NULL, in order to avoid wrongly freeing them in rte_eth_dev_release_port(). A closed port will have the state RTE_ETH_DEV_UNUSED which is considered as invalid by rte_eth_dev_is_valid_port(). So validity is not checked anymore for closed ports in testpmd. If the driver is trying to free the port again, the function rte_eth_dev_release_port() will abort with -ENODEV error. Signed-off-by: Thomas Monjalon --- app/test-pmd/testpmd.c | 16 +++------------- doc/guides/rel_notes/release_18_11.rst | 4 ++++ drivers/net/af_packet/rte_eth_af_packet.c | 4 +++- drivers/net/bnxt/bnxt_ethdev.c | 4 ---- drivers/net/failsafe/failsafe_ops.c | 2 ++ drivers/net/mlx4/mlx4.c | 2 ++ drivers/net/mlx5/mlx5.c | 2 ++ drivers/net/netvsc/hn_ethdev.c | 5 ++++- drivers/net/pcap/rte_eth_pcap.c | 5 +++++ drivers/net/softnic/rte_eth_softnic.c | 3 ++- drivers/net/tap/rte_eth_tap.c | 3 +++ drivers/net/vhost/rte_eth_vhost.c | 4 ---- lib/librte_ethdev/rte_ethdev.c | 9 +++------ lib/librte_ethdev/rte_ethdev.h | 3 +-- 14 files changed, 34 insertions(+), 32 deletions(-) diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index 31aadec63..2bd3fd986 100644 --- a/app/test-pmd/testpmd.c +++ b/app/test-pmd/testpmd.c @@ -1938,18 +1938,6 @@ port_is_started(portid_t port_id) return 1; } -static int -port_is_closed(portid_t port_id) -{ - if (port_id_is_invalid(port_id, ENABLED_WARN)) - return 0; - - if (ports[port_id].port_status != RTE_PORT_CLOSED) - return 0; - - return 1; -} - int start_port(portid_t pid) { @@ -2252,6 +2240,8 @@ close_port(portid_t pid) port_flow_flush(pi); rte_eth_dev_close(pi); + remove_unused_fwd_ports(); + if (rte_atomic16_cmpset(&(port->port_status), RTE_PORT_HANDLING, RTE_PORT_CLOSED) == 0) printf("Port %d cannot be set to closed\n", pi); @@ -2342,7 +2332,7 @@ detach_port(portid_t port_id) printf("Detaching a port...\n"); - if (!port_is_closed(port_id)) { + if (ports[port_id].port_status != RTE_PORT_CLOSED) { if (ports[port_id].port_status != RTE_PORT_STOPPED) { printf("Port not stopped\n"); return; diff --git a/doc/guides/rel_notes/release_18_11.rst b/doc/guides/rel_notes/release_18_11.rst index 2467d04e2..b982ca80e 100644 --- a/doc/guides/rel_notes/release_18_11.rst +++ b/doc/guides/rel_notes/release_18_11.rst @@ -215,6 +215,10 @@ API Changes functions were deprecated since 17.05 and are replaced by ``rte_mbuf_raw_free()`` and ``rte_pktmbuf_prefree_seg()``. +* ethdev: A call to ``rte_eth_dev_release_port()`` has been added in + ``rte_eth_dev_close()``. As a consequence, a closed port is freed + and seen as invalid because of its state ``RTE_ETH_DEV_UNUSED``. + * A new device flag, RTE_ETH_DEV_NOLIVE_MAC_ADDR, changes the order of actions inside rte_eth_dev_start regarding MAC set. Some NICs do not support MAC changes once the port has started and with this new device diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c index 95a98c6b8..2678335f6 100644 --- a/drivers/net/af_packet/rte_eth_af_packet.c +++ b/drivers/net/af_packet/rte_eth_af_packet.c @@ -362,8 +362,10 @@ eth_stats_reset(struct rte_eth_dev *dev) } static void -eth_dev_close(struct rte_eth_dev *dev __rte_unused) +eth_dev_close(struct rte_eth_dev *dev) { + /* mac_addrs must not be freed alone because part of dev_private */ + dev->data->mac_addrs = NULL; } static void diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c index 801c6ffad..141bd6376 100644 --- a/drivers/net/bnxt/bnxt_ethdev.c +++ b/drivers/net/bnxt/bnxt_ethdev.c @@ -712,10 +712,6 @@ static void bnxt_dev_close_op(struct rte_eth_dev *eth_dev) if (bp->dev_stopped == 0) bnxt_dev_stop_op(eth_dev); - if (eth_dev->data->mac_addrs != NULL) { - rte_free(eth_dev->data->mac_addrs); - eth_dev->data->mac_addrs = NULL; - } if (bp->grp_info != NULL) { rte_free(bp->grp_info); bp->grp_info = NULL; diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c index 7f8bcd4c6..f10433b30 100644 --- a/drivers/net/failsafe/failsafe_ops.c +++ b/drivers/net/failsafe/failsafe_ops.c @@ -331,6 +331,8 @@ fs_dev_close(struct rte_eth_dev *dev) sdev->state = DEV_ACTIVE - 1; } fs_dev_free_queues(dev); + /* mac_addrs must not be freed alone because part of dev_private */ + dev->data->mac_addrs = NULL; fs_unlock(dev, 0); } diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c index 7f07b8dc0..4a7b5e220 100644 --- a/drivers/net/mlx4/mlx4.c +++ b/drivers/net/mlx4/mlx4.c @@ -209,6 +209,8 @@ mlx4_dev_close(struct rte_eth_dev *dev) mlx4_rx_queue_release(dev->data->rx_queues[i]); for (i = 0; i != dev->data->nb_tx_queues; ++i) mlx4_tx_queue_release(dev->data->tx_queues[i]); + /* mac_addrs must not be freed because part of dev_private */ + dev->data->mac_addrs = NULL; mlx4_mr_release(dev); if (priv->pd != NULL) { assert(priv->ctx != NULL); diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index b2be74b51..82dd7ad41 100644 --- a/drivers/net/mlx5/mlx5.c +++ b/drivers/net/mlx5/mlx5.c @@ -337,6 +337,8 @@ mlx5_dev_close(struct rte_eth_dev *dev) } memset(priv, 0, sizeof(*priv)); priv->domain_id = RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID; + /* mac_addrs must not be freed alone because part of dev_private */ + dev->data->mac_addrs = NULL; } const struct eth_dev_ops mlx5_dev_ops = { diff --git a/drivers/net/netvsc/hn_ethdev.c b/drivers/net/netvsc/hn_ethdev.c index aa38ee7a3..5a7463628 100644 --- a/drivers/net/netvsc/hn_ethdev.c +++ b/drivers/net/netvsc/hn_ethdev.c @@ -630,11 +630,14 @@ hn_dev_stop(struct rte_eth_dev *dev) } static void -hn_dev_close(struct rte_eth_dev *dev __rte_unused) +hn_dev_close(struct rte_eth_dev *dev) { PMD_INIT_LOG(DEBUG, "close"); hn_vf_close(dev); + + /* mac_addrs must not be freed alone because part of dev_private */ + dev->data->mac_addrs = NULL; } static const struct eth_dev_ops hn_eth_dev_ops = { diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c index 7bbe72e25..bfc003663 100644 --- a/drivers/net/pcap/rte_eth_pcap.c +++ b/drivers/net/pcap/rte_eth_pcap.c @@ -623,6 +623,11 @@ eth_stats_reset(struct rte_eth_dev *dev) static void eth_dev_close(struct rte_eth_dev *dev __rte_unused) { + struct pmd_internals *internals = dev->data->dev_private; + + if (internals != NULL && internals->phy_mac == 0) + /* not dynamically allocated, must not be freed */ + dev->data->mac_addrs = NULL; } static void diff --git a/drivers/net/softnic/rte_eth_softnic.c b/drivers/net/softnic/rte_eth_softnic.c index 9a2418438..083519b38 100644 --- a/drivers/net/softnic/rte_eth_softnic.c +++ b/drivers/net/softnic/rte_eth_softnic.c @@ -194,8 +194,9 @@ pmd_dev_stop(struct rte_eth_dev *dev) } static void -pmd_dev_close(struct rte_eth_dev *dev __rte_unused) +pmd_dev_close(struct rte_eth_dev *dev) { + dev->data->mac_addrs = NULL; /* statically allocated */ return; } diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c index 53d37b3cb..32f2f888d 100644 --- a/drivers/net/tap/rte_eth_tap.c +++ b/drivers/net/tap/rte_eth_tap.c @@ -1000,6 +1000,9 @@ 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; } static void diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c index 0cd1a4642..d3e78503b 100644 --- a/drivers/net/vhost/rte_eth_vhost.c +++ b/drivers/net/vhost/rte_eth_vhost.c @@ -998,12 +998,8 @@ eth_dev_close(struct rte_eth_dev *dev) for (i = 0; i < dev->data->nb_tx_queues; i++) rte_free(dev->data->tx_queues[i]); - rte_free(dev->data->mac_addrs); free(internal->dev_name); free(internal->iface_name); - rte_free(internal); - - dev->data->dev_private = NULL; } static int diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c index 178800a5b..987ba5ab1 100644 --- a/lib/librte_ethdev/rte_ethdev.c +++ b/lib/librte_ethdev/rte_ethdev.c @@ -367,6 +367,8 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev) { if (eth_dev == NULL) return -EINVAL; + if (eth_dev->state == RTE_ETH_DEV_UNUSED) + return -ENODEV; rte_eth_dev_shared_data_prepare(); @@ -1384,12 +1386,7 @@ rte_eth_dev_close(uint16_t port_id) dev->data->dev_started = 0; (*dev->dev_ops->dev_close)(dev); - dev->data->nb_rx_queues = 0; - rte_free(dev->data->rx_queues); - dev->data->rx_queues = NULL; - dev->data->nb_tx_queues = 0; - rte_free(dev->data->tx_queues); - dev->data->tx_queues = NULL; + rte_eth_dev_release_port(dev); } int diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h index fb40c89e0..dcdeb184b 100644 --- a/lib/librte_ethdev/rte_ethdev.h +++ b/lib/librte_ethdev/rte_ethdev.h @@ -1802,8 +1802,7 @@ int rte_eth_dev_set_link_down(uint16_t port_id); /** * Close a stopped Ethernet device. The device cannot be restarted! - * The function frees all resources except for needed by the - * closed state. To free these resources, call rte_eth_dev_detach(). + * The function frees all port resources. * * @param port_id * The port identifier of the Ethernet device. -- 2.19.0