CALL to eth PMD maintainers: complete closing of port
Hi all,
Since DPDK 18.11, the behaviour of the close operation is changed if RTE_ETH_DEV_CLOSE_REMOVE is enabled in the driver:
port is released (i.e. totally freed and data erased) on close. This new behaviour is enabled per driver for a migration period.
Looking at the code, you can see these comments:
/* old behaviour: only free queue arrays */ RTE_ETHDEV_LOG(DEBUG, "Port closing is using an old behaviour.\n"
"The driver %s should migrate to the new behaviour.\n",
/* new behaviour: send event + reset state + free all data */
You can find an advice in the commit:
http://git.dpdk.org/dpdk/commit/?id=23ea57a2a
"
When enabling RTE_ETH_DEV_CLOSE_REMOVE,
the PMD must free all its private resources for the port, in its dev_close function.
It is advised to call the dev_close function in the remove function in order to support removing a device without closing its ports. "
It would be great to complete this migration for the next LTS version, which will be 19.11.
It means the work should be ideally finished during the summer.
The migration to the new behaviour is done in 4 drivers:
git grep -l RTE_ETH_DEV_CLOSE_REMOVE drivers | cut -d/ -f3
ena
enic
mlx5
vmxnet3
Following drivers should be migrated:
( find drivers/net -mindepth 1 -maxdepth 1 -type d | cut -d/ -f3 ; git grep -l RTE_ETH_DEV_CLOSE_REMOVE drivers | cut -d/ -f3 ) | sort | uniq -u
af_packet af_xdp ark atlantic avp axgbe bnx2x bnxt bonding cxgbe dpaa dpaa2 e1000 enetc failsafe fm10k i40e iavf ice ifc ixgbe kni liquidio mlx4 mvneta mvpp2 netvsc nfb nfp null octeontx pcap qede ring sfc softnic szedata2 tap thunderx vdev_netvsc vhost virtio
Please let's progress smoothly on this topic, thanks.
The concerned maintainers (Cc) can be found with the following command:devtools/get-maintainer.sh $(( find drivers/net -mindepth 1 -maxdepth 1 -type d | cut -d/ -f-3 ; git grep -l RTE_ETH_DEV_CLOSE_REMOVE drivers ) | sort | uniq -u)
Hi Thomas
vdev_netvsc has no close API - should I change something there?
Quoted 93 lines
From: Thomas Monjalon <[email protected]>
Hi all,Since DPDK 18.11, the behaviour of the close operation is changed if
RTE_ETH_DEV_CLOSE_REMOVEis enabled in the driver:
port is released (i.e. totally freed and data erased) on close. This new behaviour is enabled per driver for a migration period.Looking at the code, you can see these comments:
/* old behaviour: only free queue arrays */ RTE_ETHDEV_LOG(DEBUG, "Port closing is using an old behaviour.\n"
"The driver %s should migrate to the new behaviour.\n",
/* new behaviour: send event + reset state + free all data */You can find an advice in the commit:
https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgit.dpdk.org%2Fdpdk%2Fcommit%2F%3Fid%3D23ea57a2a&data=02%7 C01%7Cmatan%40mellanox.com%7C3fea370ec31f4d22be8708d6c3ecf74b%7 Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636911819926371170&a mp;sdata=%2BHLHG6VK2gNLejSHRKLYtS4Qelqg%2FOD%2FUQbZwOIT9%2BA %3D&reserved=0
"
When enablingRTE_ETH_DEV_CLOSE_REMOVE,
the PMD must free all its private resources for the port, in itsdev_closefunction.
It is advised to call thedev_closefunction in the remove function in order to support removing a device without closing its ports.
"It would be great to complete this migration for the next LTS version, which will be 19.11.
It means the work should be ideally finished during the summer.The migration to the new behaviour is done in 4 drivers:
git grep -lRTE_ETH_DEV_CLOSE_REMOVEdrivers | cut -d/ -f3
ena
enic
mlx5
vmxnet3Following drivers should be migrated:
( finddrivers/net-mindepth 1 -maxdepth 1 -type d | cut -d/ -f3 ; git grep -lRTE_ETH_DEV_CLOSE_REMOVEdrivers | cut -d/ -f3 ) | sort | uniq -uaf_packet af_xdp ark atlantic avp axgbe bnx2x bnxt bonding cxgbe dpaa dpaa2 e1000 enetc failsafe fm10k i40e iavf ice ifc ixgbe kni liquidio mlx4 mvneta mvpp2 netvsc nfb nfp null octeontx pcap qede ring sfc softnic szedata2 tap thunderx vdev_netvsc vhost virtioPlease let's progress smoothly on this topic, thanks.
The concerned maintainers (Cc) can be found with the following command:
devtools/get-maintainer.sh$(( finddrivers/net-mindepth 1 -maxdepth 1 - type d | cut -d/ -f-3 ; git grep -lRTE_ETH_DEV_CLOSE_REMOVEdrivers ) | sort | uniq -u)
Quoted 98 lines · On Sun, 28 Apr 2019 06:57:59 +0000 Matan Azrad <[email protected]> wrote:
Hi Thomas
vdev_netvschas no close API - should I change something there?Quoted 93 lines
From: Thomas Monjalon <[email protected]>
Hi all,Since DPDK 18.11, the behaviour of the close operation is changed if
RTE_ETH_DEV_CLOSE_REMOVEis enabled in the driver:
port is released (i.e. totally freed and data erased) on close. This new behaviour is enabled per driver for a migration period.Looking at the code, you can see these comments:
/* old behaviour: only free queue arrays */ RTE_ETHDEV_LOG(DEBUG, "Port closing is using an old behaviour.\n"
"The driver %s should migrate to the new behaviour.\n",
/* new behaviour: send event + reset state + free all data */You can find an advice in the commit:
https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgit.dpdk.org%2Fdpdk%2Fcommit%2F%3Fid%3D23ea57a2a&data=02%7 C01%7Cmatan%40mellanox.com%7C3fea370ec31f4d22be8708d6c3ecf74b%7 Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636911819926371170&a mp;sdata=%2BHLHG6VK2gNLejSHRKLYtS4Qelqg%2FOD%2FUQbZwOIT9%2BA %3D&reserved=0
"
When enablingRTE_ETH_DEV_CLOSE_REMOVE,
the PMD must free all its private resources for the port, in itsdev_closefunction.
It is advised to call thedev_closefunction in the remove function in order to support removing a device without closing its ports.
"It would be great to complete this migration for the next LTS version, which will be 19.11.
It means the work should be ideally finished during the summer.The migration to the new behaviour is done in 4 drivers:
git grep -lRTE_ETH_DEV_CLOSE_REMOVEdrivers | cut -d/ -f3
ena
enic
mlx5
vmxnet3Following drivers should be migrated:
( finddrivers/net-mindepth 1 -maxdepth 1 -type d | cut -d/ -f3 ; git grep -lRTE_ETH_DEV_CLOSE_REMOVEdrivers | cut -d/ -f3 ) | sort | uniq -uaf_packet af_xdp ark atlantic avp axgbe bnx2x bnxt bonding cxgbe dpaa dpaa2 e1000 enetc failsafe fm10k i40e iavf ice ifc ixgbe kni liquidio mlx4 mvneta mvpp2 netvsc nfb nfp null octeontx pcap qede ring sfc softnic szedata2 tap thunderx vdev_netvsc vhost virtioPlease let's progress smoothly on this topic, thanks.
The concerned maintainers (Cc) can be found with the following command:
devtools/get-maintainer.sh$(( finddrivers/net-mindepth 1 -maxdepth 1 - type d | cut -d/ -f-3 ; git grep -lRTE_ETH_DEV_CLOSE_REMOVEdrivers ) | sort | uniq -u)
I have a version still testing
Quoted 23 lines · Thomas Monjalon
Hi all,
Since DPDK 18.11, the behaviour of the close operation is changed if
RTE_ETH_DEV_CLOSE_REMOVEis enabled in the driver:
port is released (i.e. totally freed and data erased) on close. This new behaviour is enabled per driver for a migration period.Looking at the code, you can see these comments:
/* old behaviour: only free queue arrays */ RTE_ETHDEV_LOG(DEBUG, "Port closing is using an old behaviour.\n"
"The driver %s should migrate to the new behaviour.\n",
/* new behaviour: send event + reset state + free all data */You can find an advice in the commit:
http://git.dpdk.org/dpdk/commit/?id=23ea57a2a"
When enablingRTE_ETH_DEV_CLOSE_REMOVE,
the PMD must free all its private resources for the port, in itsdev_closefunction.
It is advised to call thedev_closefunction in the remove function in order to support removing a device without closing its ports. "
Quoted 3 lines
It would be great to complete this migration for the next LTS version, which will be 19.11.
It means the work should be ideally finished during the summer.
I would like to detail a little more what needs to be done, mainly for the sake of the discussion, please comment if something missing/wrong.
There are two exit paths for driver:
1) Hotplug remove the device
rte_dev_remove() OR rte_eal_hotplug_remove()
2) Application exit:
rte_eth_dev_close()
(1) can be called without ethdev stop and close functions called, so the path should cover those functions.
And in the PMD entry point in this path is .remove() function. In this .remove() function PMD should:
- stop forwarding
- clean PMD private resources (dev_close() ? )
- clean ethdev generic resources (rte_eth_dev_release_port() ? )
- remove device resources, which already done by remove APIs
(2) ethdev won't be usable after "rte_eth_dev_close()" and it should clear PMD private and ethdev generic resources.
With RTE_ETH_DEV_CLOSE_REMOVE flag 'rte_eth_dev_close()' will free generic ethdev resources (rte_eth_dev_release_port()) so PMD specific '.dev_close()' should:
- stop forwarding
- clean PMD private resources
If agreed on above, the task is:
- '.dev_close()'
- stop forwarding
- clean PMD private resources
- '.remove()'
- call '.dev_close()'
A question is if application should call 'rte_eth_dev_stop()' explicitly before close or should it be part of close? For (2) it makes sense app call the stop explicitly, but for device remove it is not clear who to stop the forwarding, for this case 'dev_close()' stopping forwarding makes things easier.
29/04/2019 18:51, Ferruh Yigit:
Quoted 39 lines · Thomas Monjalon
Quoted 23 lines
Hi all,
Since DPDK 18.11, the behaviour of the close operation is changed if
RTE_ETH_DEV_CLOSE_REMOVEis enabled in the driver:
port is released (i.e. totally freed and data erased) on close. This new behaviour is enabled per driver for a migration period.Looking at the code, you can see these comments:
/* old behaviour: only free queue arrays */ RTE_ETHDEV_LOG(DEBUG, "Port closing is using an old behaviour.\n"
"The driver %s should migrate to the new behaviour.\n",
/* new behaviour: send event + reset state + free all data */You can find an advice in the commit:
http://git.dpdk.org/dpdk/commit/?id=23ea57a2a"
When enablingRTE_ETH_DEV_CLOSE_REMOVE,
the PMD must free all its private resources for the port, in itsdev_closefunction.
It is advised to call thedev_closefunction in the remove function in order to support removing a device without closing its ports. "Quoted 3 lines
It would be great to complete this migration for the next LTS version, which will be 19.11.
It means the work should be ideally finished during the summer.I would like to detail a little more what needs to be done, mainly for the sake of the discussion, please comment if something missing/wrong.
There are two exit paths for driver:
1) Hotplug remove the device
rte_dev_remove()ORrte_eal_hotplug_remove()2) Application exit:
rte_eth_dev_close()
Quoted 6 lines
(1) can be called without ethdev stop and close functions called, so the path should cover those functions.
And in the PMD entry point in this path is .remove() function. In this .remove() function PMD should:
- stop forwarding
- clean PMD private resources (dev_close()? )
yes
Quoted 1 line
- clean ethdev generic resources (
rte_eth_dev_release_port()? )
already done in dev_close thanks to RTE_ETH_DEV_CLOSE_REMOVE
Quoted 1 line
- remove device resources, which already done by remove APIs
There can be some specific PMD private resources, not cleaned when closing a port, to clean in "remove".
Quoted 6 lines
(2) ethdev won't be usable after "
rte_eth_dev_close()" and it should clear PMD private and ethdev generic resources.
WithRTE_ETH_DEV_CLOSE_REMOVEflag 'rte_eth_dev_close()' will free generic ethdev resources (rte_eth_dev_release_port()) so PMD specific '.dev_close()' should:
- stop forwarding
- clean PMD private resources
yes, but only port-related resources,
not those common with other ports or features of the device.
Quoted 7 lines
If agreed on above, the task is:
- '.dev_close()'
- stop forwarding
- clean PMD private resources- '.remove()'
- call '.dev_close()'
Yes, looks right.
Quoted 4 lines
A question is if application should call '
rte_eth_dev_stop()' explicitly before close or should it be part of close? For (2) it makes sense app call the stop explicitly, but for device remove it is not clear who to stop the forwarding, for this case 'dev_close()' stopping forwarding makes things easier.
"stop" is a prerequisite for "close" anyway.
In most cases, the app should manage "stop" itself to properly free related resources.
The question is to know whether we return an error on "close" if the port is not stopped, or we stop it implicitly.
The API says:
* Close a stopped Ethernet device. The device cannot be restarted!
We may explicit the behaviour if the port is not stopped.
Quoted 65 lines · Thomas Monjalon
29/04/2019 18:51, Ferruh Yigit:
Quoted 39 lines · Thomas Monjalon
Quoted 23 lines
Hi all,
Since DPDK 18.11, the behaviour of the close operation is changed if
RTE_ETH_DEV_CLOSE_REMOVEis enabled in the driver:
port is released (i.e. totally freed and data erased) on close. This new behaviour is enabled per driver for a migration period.Looking at the code, you can see these comments:
/* old behaviour: only free queue arrays */ RTE_ETHDEV_LOG(DEBUG, "Port closing is using an old behaviour.\n"
"The driver %s should migrate to the new behaviour.\n",
/* new behaviour: send event + reset state + free all data */You can find an advice in the commit:
http://git.dpdk.org/dpdk/commit/?id=23ea57a2a"
When enablingRTE_ETH_DEV_CLOSE_REMOVE,
the PMD must free all its private resources for the port, in itsdev_closefunction.
It is advised to call thedev_closefunction in the remove function in order to support removing a device without closing its ports. "Quoted 3 lines
It would be great to complete this migration for the next LTS version, which will be 19.11.
It means the work should be ideally finished during the summer.I would like to detail a little more what needs to be done, mainly for the sake of the discussion, please comment if something missing/wrong.
There are two exit paths for driver:
1) Hotplug remove the device
rte_dev_remove()ORrte_eal_hotplug_remove()2) Application exit:
rte_eth_dev_close()Quoted 6 lines
(1) can be called without ethdev stop and close functions called, so the path should cover those functions.
And in the PMD entry point in this path is .remove() function. In this .remove() function PMD should:
- stop forwarding
- clean PMD private resources (dev_close()? )yes
Quoted 1 line
- clean ethdev generic resources (
rte_eth_dev_release_port()? )already done in
dev_closethanks toRTE_ETH_DEV_CLOSE_REMOVEQuoted 1 line
- remove device resources, which already done by remove APIs
There can be some specific PMD private resources, not cleaned when closing a port, to clean in "remove".
Quoted 6 lines
(2) ethdev won't be usable after "
rte_eth_dev_close()" and it should clear PMD private and ethdev generic resources.
WithRTE_ETH_DEV_CLOSE_REMOVEflag 'rte_eth_dev_close()' will free generic ethdev resources (rte_eth_dev_release_port()) so PMD specific '.dev_close()' should:
- stop forwarding
- clean PMD private resources
Quoted 2 lines
yes, but only port-related resources,
not those common with other ports or features of the device.
I have a related query. Even after rte_eth_dev_close(), we have to call rte_eal_hotplug_remove() to free up common resource if any
and remove the device from application ?
And rte_eal_hotplug_remove() would still call drivers .remove function for this.
Quoted 26 lines
Quoted 8 lines
If agreed on above, the task is:
- '.dev_close()'
- stop forwarding
- clean PMD private resources- '.remove()'
- call '.dev_close()'Yes, looks right.
Quoted 4 lines
A question is if application should call '
rte_eth_dev_stop()' explicitly before close or should it be part of close? For (2) it makes sense app call the stop explicitly, but for device remove it is not clear who to stop the forwarding, for this case 'dev_close()' stopping forwarding makes things easier."stop" is a prerequisite for "close" anyway.
In most cases, the app should manage "stop" itself to properly free related resources.
The question is to know whether we return an error on "close" if the port is not stopped, or we stop it implicitly.
The API says:
* Close a stopped Ethernet device. The device cannot be restarted!
We may explicit the behaviour if the port is not stopped.
30/04/2019 14:45, Nithin Dabilpuram:
Quoted 38 lines · Thomas Monjalon
Quoted 37 lines
29/04/2019 18:51, Ferruh Yigit:
Quoted 11 lines
I would like to detail a little more what needs to be done, mainly for the sake of the discussion, please comment if something missing/wrong.
There are two exit paths for driver:
1) Hotplug remove the device
rte_dev_remove()ORrte_eal_hotplug_remove()2) Application exit:
rte_eth_dev_close()Quoted 6 lines
(1) can be called without ethdev stop and close functions called, so the path should cover those functions.
And in the PMD entry point in this path is .remove() function. In this .remove() function PMD should:
- stop forwarding
- clean PMD private resources (dev_close()? )yes
Quoted 1 line
- clean ethdev generic resources (
rte_eth_dev_release_port()? )already done in
dev_closethanks toRTE_ETH_DEV_CLOSE_REMOVEQuoted 1 line
- remove device resources, which already done by remove APIs
There can be some specific PMD private resources, not cleaned when closing a port, to clean in "remove".
Quoted 6 lines
(2) ethdev won't be usable after "
rte_eth_dev_close()" and it should clear PMD private and ethdev generic resources.
WithRTE_ETH_DEV_CLOSE_REMOVEflag 'rte_eth_dev_close()' will free generic ethdev resources (rte_eth_dev_release_port()) so PMD specific '.dev_close()' should:
- stop forwarding
- clean PMD private resources
Quoted 6 lines
Quoted 2 lines
yes, but only port-related resources,
not those common with other ports or features of the device.I have a related query. Even after
rte_eth_dev_close(), we have to callrte_eal_hotplug_remove()to free up common resource if any
and remove the device from application ?
Andrte_eal_hotplug_remove()would still call drivers .remove function for this.
Yes if you really want to free the whole device, you should call the .remove op from rte_eal_hotplug_remove() or rte_dev_remove().
18/04/2019 12:59, Thomas Monjalon:
Quoted 23 lines
Hi all,
Since DPDK 18.11, the behaviour of the close operation is changed if
RTE_ETH_DEV_CLOSE_REMOVEis enabled in the driver:
port is released (i.e. totally freed and data erased) on close. This new behaviour is enabled per driver for a migration period.Looking at the code, you can see these comments:
/* old behaviour: only free queue arrays */ RTE_ETHDEV_LOG(DEBUG, "Port closing is using an old behaviour.\n"
"The driver %s should migrate to the new behaviour.\n",
/* new behaviour: send event + reset state + free all data */You can find an advice in the commit:
http://git.dpdk.org/dpdk/commit/?id=23ea57a2a"
When enablingRTE_ETH_DEV_CLOSE_REMOVE,
the PMD must free all its private resources for the port, in itsdev_closefunction.
It is advised to call thedev_closefunction in the remove function in order to support removing a device without closing its ports. "
Quoted 2 lines
It would be great to complete this migration for the next LTS version, which will be 19.11.
For the record, it did not happen in 19.11.
Quoted 11 lines
It means the work should be ideally finished during the summer.
The migration to the new behaviour is done in 4 drivers:
git grep -lRTE_ETH_DEV_CLOSE_REMOVEdrivers | cut -d/ -f3
ena
enic
mlx5
vmxnet3Following drivers should be migrated:
( finddrivers/net-mindepth 1 -maxdepth 1 -type d | cut -d/ -f3 ; git grep -lRTE_ETH_DEV_CLOSE_REMOVEdrivers | cut -d/ -f3 ) | sort | uniq -u
[...]
The progress in April 2019 was 4 of 46 (9%).
Quoted 1 line
Please let's progress smoothly on this topic, thanks.
More than one year later, the progress is 26 of 53 (49%).
Quoted 2 lines
The concerned maintainers (Cc) can be found with the following command:
devtools/get-maintainer.sh$(( finddrivers/net-mindepth 1 -maxdepth 1 -type d | cut -d/ -f-3 ; git grep -lRTE_ETH_DEV_CLOSE_REMOVEdrivers ) | sort | uniq -u)
We cannot wait forever. Temporary cannot be longer than 2 years. I am going to send a deprecation notice to remove the "temporary" flag RTE_ETH_DEV_CLOSE_REMOVE.
It will break drivers which are not migrated.
It will probably help to find motivation in new priorities.
More details on what to do can be found in this mail thread:
http://inbox.dpdk.org/dev/1748144.UFpUr2FPnr@xps/
03/08/2020 20:50, Thomas Monjalon:
Quoted 37 lines
18/04/2019 12:59, Thomas Monjalon:
Quoted 23 lines
Hi all,
Since DPDK 18.11, the behaviour of the close operation is changed if
RTE_ETH_DEV_CLOSE_REMOVEis enabled in the driver:
port is released (i.e. totally freed and data erased) on close. This new behaviour is enabled per driver for a migration period.Looking at the code, you can see these comments:
/* old behaviour: only free queue arrays */ RTE_ETHDEV_LOG(DEBUG, "Port closing is using an old behaviour.\n"
"The driver %s should migrate to the new behaviour.\n",
/* new behaviour: send event + reset state + free all data */You can find an advice in the commit:
http://git.dpdk.org/dpdk/commit/?id=23ea57a2a"
When enablingRTE_ETH_DEV_CLOSE_REMOVE,
the PMD must free all its private resources for the port, in itsdev_closefunction.
It is advised to call thedev_closefunction in the remove function in order to support removing a device without closing its ports. "Quoted 2 lines
It would be great to complete this migration for the next LTS version, which will be 19.11.
For the record, it did not happen in 19.11.
Quoted 2 lines
Following drivers should be migrated:
( finddrivers/net-mindepth 1 -maxdepth 1 -type d | cut -d/ -f3 ; git grep -lRTE_ETH_DEV_CLOSE_REMOVEdrivers | cut -d/ -f3 ) | sort | uniq -u[...]
The progress in April 2019 was 4 of 46 (9%).
Quoted 1 line
Please let's progress smoothly on this topic, thanks.
Quoted 8 lines
More than one year later, the progress is 26 of 53 (49%).
Quoted 2 lines
The concerned maintainers (Cc) can be found with the following command:
devtools/get-maintainer.sh$(( finddrivers/net-mindepth 1 -maxdepth 1 -type d | cut -d/ -f-3 ; git grep -lRTE_ETH_DEV_CLOSE_REMOVEdrivers ) | sort | uniq -u)We cannot wait forever. Temporary cannot be longer than 2 years. I am going to send a deprecation notice to remove the "temporary" flag
RTE_ETH_DEV_CLOSE_REMOVE.
The deprecation notice was merged in 20.08:
http://mails.dpdk.org/archives/dev/2020-August/177314.html
Quoted 5 lines
It will break drivers which are not migrated.
It will probably help to find motivation in new priorities.More details on what to do can be found in this mail thread:
http://inbox.dpdk.org/dev/1748144.UFpUr2FPnr@xps/
Summary:
* The freeing of private port resources must be moved in the PMD from the ".remove(device)" function to the ".dev_close(port)" function.
* If a generic resource (.mac_addrs or .hash_mac_addrs) cannot be freed, it must be set to NULL in ".dev_close" PMD function to protect from subsequent rte_eth_dev_release_port() freeing.
* Note 1:
The generic resources are freed in rte_eth_dev_release_port(), after ".dev_close" is called in rte_eth_dev_close(), but not when calling ".dev_close" directly from the ".remove" PMD function. That's why rte_eth_dev_release_port() must still be called explicitly from ".remove(device)" after calling the ".dev_close" PMD function.
* Note 2:
If a device can have multiple ports, the common resources must be freed only in the ".remove(device)" function.
* Note 3:
The port is supposed to be in a stopped state when it is closed. If it is not the case, it is free to the PMD implementation how to react when trying to close a non-stopped port:
either try to stop it automatically or just return an error.
The patches for removing RTE_ETH_DEV_CLOSE_REMOVE are sent:
https://patches.dpdk.org/project/dpdk/list/?series=12173
11 drivers are not supporting the new behaviour correctly:
bnx2x, cxgbe, dpaa, dpaa2, enetc, ionic, ipn3ke, liquidio, nfp, pfe, qede
If you are the maintainer of one of these drivers, you can still consider fixing it in the next days.
12/09/2020 13:25, Thomas Monjalon:
Quoted 81 lines
03/08/2020 20:50, Thomas Monjalon:
Quoted 37 lines
18/04/2019 12:59, Thomas Monjalon:
Quoted 23 lines
Hi all,
Since DPDK 18.11, the behaviour of the close operation is changed if
RTE_ETH_DEV_CLOSE_REMOVEis enabled in the driver:
port is released (i.e. totally freed and data erased) on close. This new behaviour is enabled per driver for a migration period.Looking at the code, you can see these comments:
/* old behaviour: only free queue arrays */ RTE_ETHDEV_LOG(DEBUG, "Port closing is using an old behaviour.\n"
"The driver %s should migrate to the new behaviour.\n",
/* new behaviour: send event + reset state + free all data */You can find an advice in the commit:
http://git.dpdk.org/dpdk/commit/?id=23ea57a2a"
When enablingRTE_ETH_DEV_CLOSE_REMOVE,
the PMD must free all its private resources for the port, in itsdev_closefunction.
It is advised to call thedev_closefunction in the remove function in order to support removing a device without closing its ports. "Quoted 2 lines
It would be great to complete this migration for the next LTS version, which will be 19.11.
For the record, it did not happen in 19.11.
Quoted 2 lines
Following drivers should be migrated:
( finddrivers/net-mindepth 1 -maxdepth 1 -type d | cut -d/ -f3 ; git grep -lRTE_ETH_DEV_CLOSE_REMOVEdrivers | cut -d/ -f3 ) | sort | uniq -u[...]
The progress in April 2019 was 4 of 46 (9%).
Quoted 1 line
Please let's progress smoothly on this topic, thanks.
Quoted 8 lines
More than one year later, the progress is 26 of 53 (49%).
Quoted 2 lines
The concerned maintainers (Cc) can be found with the following command:
devtools/get-maintainer.sh$(( finddrivers/net-mindepth 1 -maxdepth 1 -type d | cut -d/ -f-3 ; git grep -lRTE_ETH_DEV_CLOSE_REMOVEdrivers ) | sort | uniq -u)We cannot wait forever. Temporary cannot be longer than 2 years. I am going to send a deprecation notice to remove the "temporary" flag
RTE_ETH_DEV_CLOSE_REMOVE.The deprecation notice was merged in 20.08:
http://mails.dpdk.org/archives/dev/2020-August/177314.htmlQuoted 5 lines
It will break drivers which are not migrated.
It will probably help to find motivation in new priorities.More details on what to do can be found in this mail thread:
http://inbox.dpdk.org/dev/1748144.UFpUr2FPnr@xps/Summary:
* The freeing of private port resources must be moved in the PMD from the ".remove(device)" function to the ".dev_close(port)" function.
* If a generic resource (.mac_addrs or .hash_mac_addrs) cannot be freed, it must be set to NULL in ".dev_close" PMD function to protect from subsequent
rte_eth_dev_release_port()freeing.* Note 1:
The generic resources are freed inrte_eth_dev_release_port(), after ".dev_close" is called inrte_eth_dev_close(), but not when calling ".dev_close" directly from the ".remove" PMD function. That's whyrte_eth_dev_release_port()must still be called explicitly from ".remove(device)" after calling the ".dev_close" PMD function.* Note 2:
If a device can have multiple ports, the common resources must be freed only in the ".remove(device)" function.* Note 3:
The port is supposed to be in a stopped state when it is closed. If it is not the case, it is free to the PMD implementation how to react when trying to close a non-stopped port:
either try to stop it automatically or just return an error.
Quoted 10 lines · Thomas Monjalon
The patches for removing
RTE_ETH_DEV_CLOSE_REMOVEare sent:https://patches.dpdk.org/project/dpdk/list/?series=1217311 drivers are not supporting the new behaviour correctly:
bnx2x, cxgbe, dpaa, dpaa2, enetc, ionic, ipn3ke, liquidio, nfp, pfe, qedeIf you are the maintainer of one of these drivers, you can still consider fixing it in the next days.
The RTE_ETH_DEV_CLOSE_REMOVE migration for CXGBE PMD has already been submitted two weeks ago [1].
[1] https://patches.dpdk.org/patch/76271/
Thanks,
Rahul
Quoted 86 lines
12/09/2020 13:25, Thomas Monjalon:
Quoted 81 lines
03/08/2020 20:50, Thomas Monjalon:
Quoted 37 lines
18/04/2019 12:59, Thomas Monjalon:
Quoted 23 lines
Hi all,
Since DPDK 18.11, the behaviour of the close operation is changed if
RTE_ETH_DEV_CLOSE_REMOVEis enabled in the driver:
port is released (i.e. totally freed and data erased) on close. This new behaviour is enabled per driver for a migration period.Looking at the code, you can see these comments:
/* old behaviour: only free queue arrays */ RTE_ETHDEV_LOG(DEBUG, "Port closing is using an old behaviour.\n"
"The driver %s should migrate to the new behaviour.\n",
/* new behaviour: send event + reset state + free all data */You can find an advice in the commit:
http://git.dpdk.org/dpdk/commit/?id=23ea57a2a"
When enablingRTE_ETH_DEV_CLOSE_REMOVE,
the PMD must free all its private resources for the port, in itsdev_closefunction.
It is advised to call thedev_closefunction in the remove function in order to support removing a device without closing its ports. "Quoted 2 lines
It would be great to complete this migration for the next LTS version, which will be 19.11.
For the record, it did not happen in 19.11.
Quoted 2 lines
Following drivers should be migrated:
( finddrivers/net-mindepth 1 -maxdepth 1 -type d | cut -d/ -f3 ; git grep -lRTE_ETH_DEV_CLOSE_REMOVEdrivers | cut -d/ -f3 ) | sort | uniq -u[...]
The progress in April 2019 was 4 of 46 (9%).
Quoted 1 line
Please let's progress smoothly on this topic, thanks.
Quoted 8 lines
More than one year later, the progress is 26 of 53 (49%).
Quoted 2 lines
The concerned maintainers (Cc) can be found with the following command:
devtools/get-maintainer.sh$(( finddrivers/net-mindepth 1 -maxdepth 1 -type d | cut -d/ -f-3 ; git grep -lRTE_ETH_DEV_CLOSE_REMOVEdrivers ) | sort | uniq -u)We cannot wait forever. Temporary cannot be longer than 2 years. I am going to send a deprecation notice to remove the "temporary" flag
RTE_ETH_DEV_CLOSE_REMOVE.The deprecation notice was merged in 20.08:
http://mails.dpdk.org/archives/dev/2020-August/177314.htmlQuoted 5 lines
It will break drivers which are not migrated.
It will probably help to find motivation in new priorities.More details on what to do can be found in this mail thread:
http://inbox.dpdk.org/dev/1748144.UFpUr2FPnr@xps/Summary:
* The freeing of private port resources must be moved in the PMD from the ".remove(device)" function to the ".dev_close(port)" function.
* If a generic resource (.mac_addrs or .hash_mac_addrs) cannot be freed, it must be set to NULL in ".dev_close" PMD function to protect from subsequent
rte_eth_dev_release_port()freeing.* Note 1:
The generic resources are freed inrte_eth_dev_release_port(), after ".dev_close" is called inrte_eth_dev_close(), but not when calling ".dev_close" directly from the ".remove" PMD function. That's whyrte_eth_dev_release_port()must still be called explicitly from ".remove(device)" after calling the ".dev_close" PMD function.* Note 2:
If a device can have multiple ports, the common resources must be freed only in the ".remove(device)" function.* Note 3:
The port is supposed to be in a stopped state when it is closed. If it is not the case, it is free to the PMD implementation how to react when trying to close a non-stopped port:
either try to stop it automatically or just return an error.
Hi Thomas,
Ipn3ke is based on ifpga_bus, and all ethdevs created from ipn3ke are representors. So it's no need to add RTE_ETH_DEV_CLOSE_REMOVE flags into ipn3ke driver.
Thanks,
Rosen
Quoted 155 lines
-----Original Message-----
From: Thomas Monjalon <[email protected]>
Sent: Monday, September 14, 2020 6:16
To: John W. Linville <[email protected]>; Zhang, Qi Z <[email protected]>; Shepard Siegel <[email protected]>; Ed Czeck <[email protected]>; John Miller <[email protected]>; Igor Russkikh <[email protected]>; Pavel Belous <[email protected]>; Matt Peters <[email protected]>; Rasesh Mody <[email protected]>; Shahed Shaikh <[email protected]>; Ajit Khaparde <[email protected]>; Somnath Kotur <[email protected]>; Chas Williams <[email protected]>; Rahul Lakkireddy <[email protected]>; Hemant Agrawal <[email protected]>; Marcin Wojtas <[email protected]>; Michal Krawczyk <[email protected]>; Guy Tzalik <[email protected]>; Evgeny Schemeilin <[email protected]>; Gagandeep Singh <[email protected]>; John Daley <[email protected]>; Hyong Youb Kim <[email protected]>; Gaetan Rivet <[email protected]>; Wang, Xiao W <[email protected]>; Yang, Qiming <[email protected]>; Yigit, Ferruh <[email protected]>; Shijith Thotton <[email protected]>; Srisivasubramanian Srinivasan <[email protected]>; Matan Azrad <[email protected]>; Shahaf Shuler <[email protected]>; Zyta Szpak <[email protected]>; Liron Himi <[email protected]>; Stephen Hemminger <[email protected]>; K. Y. Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>; Jerin Jacob <[email protected]>; Richardson, Bruce <[email protected]>; Andrew Rybchenko <[email protected]>; Singh, Jasvinder <[email protected]>; Dumitrescu, Cristian <[email protected]>; Wiles, Keith <[email protected]>; Maciej Czekaj <[email protected]>; Maxime Coquelin <[email protected]>; Wang, Zhihong <[email protected]>; Yong Wang <[email protected]>; Burakov, Anatoly <[email protected]>; Xing, Beilei <[email protected]>; Wu, Jingjing <[email protected]>; [email protected]
Cc: Xu, Rosen <[email protected]>; [email protected]; Loftus, Ciara <[email protected]>; Steven Webster <[email protected]>; Somalapuram Amaranath <[email protected]>; [email protected]; Sachin Saxena <[email protected]>; Zhao1, Wei <[email protected]>; Guo, Jia <[email protected]>; Igor Chauskin <[email protected]>; Ziyang Xuan <[email protected]>; Xiaoyun Wang <[email protected]>; Guoyang Zhou <[email protected]>; Min Hu (Connor) <[email protected]>; Yisen Zhuang <[email protected]>; Alfredo Cardigliano <[email protected]>; Jakub Grajciar <[email protected]>; Viacheslav Ovsiienko <[email protected]>; Long Li <[email protected]>; Martin Spinler <[email protected]>; Heinrich Kuhn <[email protected]>; Harman Kalra <[email protected]>; Nithin Dabilpuram <[email protected]>; Kiran Kumar K <[email protected]>; Akhil Goyal <[email protected]>; Yigit, Ferruh <[email protected]>; Thomas Monjalon <[email protected]>
Subject: Re: [dpdk-dev] CALL to eth PMD maintainers: complete closing of portThe patches for removing
RTE_ETH_DEV_CLOSE_REMOVEare sent:https://patches.dpdk.org/project/dpdk/list/?series=1217311 drivers are not supporting the new behaviour correctly:
bnx2x, cxgbe, dpaa, dpaa2, enetc, ionic, ipn3ke, liquidio, nfp, pfe, qedeIf you are the maintainer of one of these drivers, you can still consider fixing it in the next days.
12/09/2020 13:25, Thomas Monjalon:
Quoted 19 lines
03/08/2020 20:50, Thomas Monjalon:
Quoted 18 lines
18/04/2019 12:59, Thomas Monjalon:
Quoted 17 lines
Hi all,
Since DPDK 18.11, the behaviour of the close operation is changed if
RTE_ETH_DEV_CLOSE_REMOVEis enabled in the driver:
port is released (i.e. totally freed and data erased) on close. This new behaviour is enabled per driver for a migration period.Looking at the code, you can see these comments:
/* old behaviour: only free queue arrays */ RTE_ETHDEV_LOG(DEBUG, "Port closing is using an old behaviour.\n"
"The driver %s should migrate to the new behaviour.\n",
/* new behaviour: send event + reset state + free all data */You can find an advice in the commit:
http://git.dpdk.org/dpdk/commit/?id=23ea57a2a"
When enablingRTE_ETH_DEV_CLOSE_REMOVE, the PMD must free allits
Quoted 16 lines
Quoted 16 lines
Quoted 7 lines
private resources for the port, in its
dev_closefunction.
It is advised to call thedev_closefunction in the remove function in order to support removing a device without closing its ports. "It would be great to complete this migration for the next LTS version, which will be 19.11.
For the record, it did not happen in 19.11.
Quoted 4 lines
Following drivers should be migrated:
( finddrivers/net-mindepth 1 -maxdepth 1 -type d | cut -d/ -f3 ; git grep -lRTE_ETH_DEV_CLOSE_REMOVEdrivers | cut -d/ -f3 ) | sort | uniq -u[...]
Quoted 7 lines
Quoted 7 lines
The progress in April 2019 was 4 of 46 (9%).
Quoted 1 line
Please let's progress smoothly on this topic, thanks.
More than one year later, the progress is 26 of 53 (49%).
Quoted 1 line
The concerned maintainers (Cc) can be found with the following
command:
Quoted 42 lines
Quoted 7 lines
Quoted 3 lines
devtools/get-maintainer.sh$(( finddrivers/net-mindepth 1 -maxdepth 1 -type d | cut -d/ -f-3 ; git grep -lRTE_ETH_DEV_CLOSE_REMOVEdrivers ) | sort | uniq -u)We cannot wait forever. Temporary cannot be longer than 2 years. I am going to send a deprecation notice to remove the "temporary" flag
RTE_ETH_DEV_CLOSE_REMOVE.The deprecation notice was merged in 20.08:
http://mails.dpdk.org/archives/dev/2020-August/177314.htmlQuoted 5 lines
It will break drivers which are not migrated.
It will probably help to find motivation in new priorities.More details on what to do can be found in this mail thread:
http://inbox.dpdk.org/dev/1748144.UFpUr2FPnr@xps/Summary:
* The freeing of private port resources must be moved in the PMD from the ".remove(device)" function to the ".dev_close(port)" function.
* If a generic resource (.mac_addrs or .hash_mac_addrs) cannot be freed, it must be set to NULL in ".dev_close" PMD function to protect from subsequent
rte_eth_dev_release_port()freeing.* Note 1:
The generic resources are freed inrte_eth_dev_release_port(), after ".dev_close" is called inrte_eth_dev_close(), but not when calling ".dev_close" directly from the ".remove" PMD function. That's whyrte_eth_dev_release_port()must still be called explicitly from ".remove(device)" after calling the ".dev_close" PMD function.* Note 2:
If a device can have multiple ports, the common resources must be freed only in the ".remove(device)" function.* Note 3:
The port is supposed to be in a stopped state when it is closed. If it is not the case, it is free to the PMD implementation how to react when trying to close a non-stopped port:
either try to stop it automatically or just return an error.
22/09/2020 05:04, Xu, Rosen:
Quoted 4 lines
Hi Thomas,
Ipn3ke is based on
ifpga_bus, and all ethdevs created from ipn3ke are representors. So it's no need to addRTE_ETH_DEV_CLOSE_REMOVEflags into ipn3ke driver.
I don't understand how it is related.
Do you mean it will work fine with the new close behaviour?
Hi,
Quoted 18 lines
-----Original Message-----
From: Thomas Monjalon <[email protected]>
Sent: Tuesday, September 22, 2020 15:29
To: Xu, Rosen <[email protected]>
Cc: Zhang, Qi Z <[email protected]>; Wang, Xiao W <[email protected]>; Yang, Qiming <[email protected]>; Yigit, Ferruh <[email protected]>; Richardson, Bruce <[email protected]>; Andrew Rybchenko <[email protected]>; Wang, Zhihong <[email protected]>; Xing, Beilei <[email protected]>; Wu, Jingjing <[email protected]>; [email protected]; Zhao1, Wei <[email protected]>; Guo, Jia <[email protected]>; Yigit, Ferruh <[email protected]>
Subject: Re: [dpdk-dev] CALL to eth PMD maintainers: complete closing of port22/09/2020 05:04, Xu, Rosen:
Quoted 2 lines
Hi Thomas,
Quoted 7 lines
Quoted 1 line
Ipn3ke is based on
ifpga_bus, and all ethdevs created from ipn3ke arerepresentors.
Quoted 1 line
So it's no need to add
RTE_ETH_DEV_CLOSE_REMOVEflags into ipn3kedriver.
I don't understand how it is related.
Do you mean it will work fine with the new close behaviour?
yes
Hi Thomas,
Quoted 11 lines
From: dev <[email protected]> On Behalf Of Thomas Monjalon
Sent: Sunday, September 13, 2020 3:16 PMThe patches for removing
RTE_ETH_DEV_CLOSE_REMOVEare sent:
https://urldefense.proofpoint.com/v2/url?u=https-
3A__patches.dpdk.org_project_dpdk_list_-3Fseries- 3D12173&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=Vhi2FR3R84xPMUtUhj NPxoiMSxcj1IW0xDKEoZ0F00o&m=9C_CX6dJtCkd6pU6mUDB8eV- EorzP6ErRLFuCc-dtTg&s=8L7RxyHhOuIXf3MBurNOeqvGa-gGhH1NY9vnoj- fXM4&e=
Quoted 7 lines
11 drivers are not supporting the new behaviour correctly:
bnx2x, cxgbe, dpaa, dpaa2, enetc, ionic, ipn3ke, liquidio, nfp, pfe, qedeIf you are the maintainer of one of these drivers, you can still consider fixing it in the next days.
The fixes for bnx2x and qede PMDs are posted.
https://patches.dpdk.org/patch/78779/
https://patches.dpdk.org/patch/78780/
Thanks!
-Rasesh
Quoted 115 lines
12/09/2020 13:25, Thomas Monjalon:
Quoted 19 lines
03/08/2020 20:50, Thomas Monjalon:
Quoted 18 lines
18/04/2019 12:59, Thomas Monjalon:
Quoted 17 lines
Hi all,
Since DPDK 18.11, the behaviour of the close operation is changed if
RTE_ETH_DEV_CLOSE_REMOVEis enabled in the driver:
port is released (i.e. totally freed and data erased) on close. This new behaviour is enabled per driver for a migration period.Looking at the code, you can see these comments:
/* old behaviour: only free queue arrays */ RTE_ETHDEV_LOG(DEBUG, "Port closing is using an old behaviour.\n"
"The driver %s should migrate to the new behaviour.\n",
/* new behaviour: send event + reset state + free all data */You can find an advice in the commit:
https://urldefense.proofpoint.com/v2/url?u=http-3A__git.dpdk.org_d
pdk_commit_-3Fid-3D23ea57a2a&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=V
Quoted 1 line
Quoted 1 line
Quoted 1 line
hi2FR3R84xPMUtUhjNPxoiMSxcj1IW0xDKEoZ0F00o&m=9C_CX6dJtCkd6pU6m UDB8
Quoted 1 line
Quoted 1 line
Quoted 1 line
eV-EorzP6ErRLFuCc-
dtTg&s=23dp27AoThshb5NflUTFh3HH3M_KPwa8Svd2TOeN9
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
gs&e=
"
When enablingRTE_ETH_DEV_CLOSE_REMOVE, the PMD must free allits
Quoted 16 lines
Quoted 16 lines
Quoted 7 lines
private resources for the port, in its
dev_closefunction.
It is advised to call thedev_closefunction in the remove function in order to support removing a device without closing its ports. "It would be great to complete this migration for the next LTS version, which will be 19.11.
For the record, it did not happen in 19.11.
Quoted 4 lines
Following drivers should be migrated:
( finddrivers/net-mindepth 1 -maxdepth 1 -type d | cut -d/ -f3 ; git grep -lRTE_ETH_DEV_CLOSE_REMOVEdrivers | cut -d/ -f3 ) | sort | uniq -u[...]
Quoted 7 lines
Quoted 7 lines
The progress in April 2019 was 4 of 46 (9%).
Quoted 1 line
Please let's progress smoothly on this topic, thanks.
More than one year later, the progress is 26 of 53 (49%).
Quoted 1 line
The concerned maintainers (Cc) can be found with the following
command:
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
devtools/get-maintainer.sh$(( finddrivers/net-mindepth 1 -maxdepth 1 -type d | cut -d/ -f-3 ; git grep -lRTE_ETH_DEV_CLOSE_REMOVEdrivers ) | sort | uniq -u)Quoted 8 lines
Quoted 3 lines
We cannot wait forever. Temporary cannot be longer than 2 years. I am going to send a deprecation notice to remove the "temporary" flag
RTE_ETH_DEV_CLOSE_REMOVE.The deprecation notice was merged in 20.08:
https://urldefense.proofpoint.com/v2/url?u=http-3A__mails.dpdk.org_arc
hives_dev_2020-2DAugust_177314.html&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&
Quoted 1 line
r=Vhi2FR3R84xPMUtUhjNPxoiMSxcj1IW0xDKEoZ0F00o&m=9C_CX6dJtCkd6p U6mUDB8e
Quoted 1 line
V-EorzP6ErRLFuCc-dtTg&s=PRJ-
nWVm0NO6WrpQDWaI0ocSwaIDowsMQNWrif0FDgg&e=
Quoted 8 lines
Quoted 8 lines
It will break drivers which are not migrated.
It will probably help to find motivation in new priorities.More details on what to do can be found in this mail thread:
https://urldefense.proofpoint.com/v2/url?u=http-3A__inbox.dpdk.org_d
ev_1748144.UFpUr2FPnr-40xps_&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=Vhi
Quoted 1 line
Quoted 1 line
2FR3R84xPMUtUhjNPxoiMSxcj1IW0xDKEoZ0F00o&m=9C_CX6dJtCkd6pU6mU DB8eV-E
Quoted 1 line
Quoted 1 line
orzP6ErRLFuCc-
dtTg&s=00JuVYCjq_EZW27VmszpI4E8Yq_dN9oGjewjBTaCZ9s&e=
Quoted 26 lines
Summary:
* The freeing of private port resources must be moved in the PMD from the ".remove(device)" function to the ".dev_close(port)" function.
* If a generic resource (.mac_addrs or .hash_mac_addrs) cannot be freed, it must be set to NULL in ".dev_close" PMD function to protect from subsequent
rte_eth_dev_release_port()freeing.* Note 1:
The generic resources are freed inrte_eth_dev_release_port(), after ".dev_close" is called inrte_eth_dev_close(), but not when calling ".dev_close" directly from the ".remove" PMD function. That's whyrte_eth_dev_release_port()must still be called explicitly from ".remove(device)" after calling the ".dev_close" PMD function.* Note 2:
If a device can have multiple ports, the common resources must be freed only in the ".remove(device)" function.* Note 3:
The port is supposed to be in a stopped state when it is closed. If it is not the case, it is free to the PMD implementation how to react when trying to close a non-stopped port:
either try to stop it automatically or just return an error.