[v5,6/6] ethdev: complete closing of port
Checks
Commit Message
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 <thomas@monjalon.net>
---
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(-)
Comments
On 10/18/18 4:24 AM, Thomas Monjalon wrote:
> 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 <thomas@monjalon.net>
I've tested the patch series together with [1].
As I expected it makes problems and resource leaks if
rte_eth_dev_close() is used.
Everything is OK if I do port stop and detach (with net/sfc
patch which does close from uninit).
If I do port stop, close and detach, the last one returns
error since the device already released and net/sfc uninit
is never called.
Basically it should be one function which is called in both
cases: dev_close or pci_device remove. Similar changes
should be done in many PCI drivers.
If I drop the patch, everything seems to be work fine
from the first sight.
May be it should be removed from the patchset and
considered separately.
[1] http://patches.dpdk.org/project/dpdk/list/?series=1966
18/10/2018 10:33, Andrew Rybchenko:
> On 10/18/18 4:24 AM, Thomas Monjalon wrote:
> > 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 <thomas@monjalon.net>
>
> I've tested the patch series together with [1].
There are 2 use cases to consider:
1/ rte_eth_dev_close + rte_dev_remove
2/ rte_dev_remove without prior close
> As I expected it makes problems and resource leaks if
> rte_eth_dev_close() is used.
>
> Everything is OK if I do port stop and detach (with net/sfc
> patch which does close from uninit).
This is the case #2.
It requires some PMD updates to avoid the leaks.
It was buggy in previous releases too, because it was not managing
devices with multiple ports.
I think we should continue recommending the use case #1 in 18.11,
and let time for PMDs to be fixed for use case #2.
It PMDs can be fixed in 18.11, it is even better.
> If I do port stop, close and detach, the last one returns
> error since the device already released and net/sfc uninit
> is never called.
This is the case #1.
The bug is assuming that the port is not freed when removing the device.
We should just skip closed ports without any error.
I can fix it in a v6.
> Basically it should be one function which is called in both
> cases: dev_close or pci_device remove. Similar changes
> should be done in many PCI drivers.
Yes, dev_close must be called for all non-closed ports of a PCI device
to remove.
Usually, PMDs have an implementation of dev_close which is different
of the uninit function called with rte_eth_dev_pci_generic_remove()
or rte_eth_dev_destroy(). They have no good reason to be different:
typedef int (*ethdev_uninit_t)(struct rte_eth_dev *ethdev);
typedef void (*eth_dev_close_t)(struct rte_eth_dev *dev);
> If I drop the patch, everything seems to be work fine
> from the first sight.
> May be it should be removed from the patchset and
> considered separately.
No, I think I should just fix the use case #1,
and let PMDs fixing the use case #2 when they have a chance.
In my opinion, the use case #2 was never advertised before,
so it is just opening the way for the support of this use case.
> [1] http://patches.dpdk.org/project/dpdk/list/?series=1966
@@ -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;
@@ -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
@@ -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
@@ -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;
@@ -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);
}
@@ -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);
@@ -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 = {
@@ -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 = {
@@ -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
@@ -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;
}
@@ -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
@@ -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
@@ -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
@@ -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.