| Message ID | 20191219115436.10354-1-elibr@mellanox.com |
|---|---|
| Headers | show |
| Series | netdev datapath actions offload | expand |
Ilya, are there more issues to be addressed or can we merge it? Thanks, Eli On 12/19/2019 1:54 PM, Eli Britstein wrote: > Currently, netdev datapath offload only accelerates the flow match > sequence by associating a mark per flow. This series introduces the full > offload of netdev datapath flows by having the HW also perform the flow > actions. > > This series adds HW offload for output, drop, set MAC, set IPv4, set > TCP/UDP ports and tunnel push actions using the DPDK rte_flow API. > > v1 Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F614511472&data=02%7C01%7Celibr%40mellanox.com%7C1352d3096c2b498d410d08d7847a452c%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637123532988228514&sdata=%2BJkAZAGl23vyOKcI1tCBXYIiiv5HVWPvwx7z5Sq8nSk%3D&reserved=0 > v2 Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F619262148&data=02%7C01%7Celibr%40mellanox.com%7C1352d3096c2b498d410d08d7847a452c%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637123532988228514&sdata=O0FfNv9g%2BOAvciIdqA7DlPmeKmy0gvRV49rwZLQ74kM%3D&reserved=0 > v3 Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F622253870&data=02%7C01%7Celibr%40mellanox.com%7C1352d3096c2b498d410d08d7847a452c%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637123532988238510&sdata=ZunPNPTqZ3m1En93FKTtq9DdlI14089ztGWGOnulXo8%3D&reserved=0 > v4 Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F625654160&data=02%7C01%7Celibr%40mellanox.com%7C1352d3096c2b498d410d08d7847a452c%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637123532988238510&sdata=C5fisZiFmNlR4Et%2BA9ErKPsLAr8m7xoLQBv2OT%2Bq1hk%3D&reserved=0 > v5 Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F626692202&data=02%7C01%7Celibr%40mellanox.com%7C1352d3096c2b498d410d08d7847a452c%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637123532988238510&sdata=M7mF64Hehjjkk1IvZG8GjmPSIiK2wGfJL6Kh0x9RuO0%3D&reserved=0 > v6 Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F627132194&data=02%7C01%7Celibr%40mellanox.com%7C1352d3096c2b498d410d08d7847a452c%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637123532988238510&sdata=15VexGKFktTA%2BVYvLuir%2BjbAjxmrbT4quS67SlaLZ4o%3D&reserved=0 > > v1-v2: > - fallback to mark/rss in case of any failure of action offload. > - dp_layer changed to "in_hw" in case the flow is offloaded. > - using netdev_ports_get instead of dp_netdev_lookup_port in stats reading. > - using flow_dump_next API instead of a new API. > - validity checks for last action of output and clone. > - count action should be done for drop as well. > - validity check for output action. > - added doc in Documentation/howto/dpdk.rst. > v2-v3: > - removed refactoring for netdev-offload-dpdk-flow.c file. > - dropped flush commits. > - dbg prints rate-limited, and outside lock. > - added validity check for output action - same flow_api handle for both netdevs. > - added a mutex to protect possible concurrent deletion/querying of a flow. > - statistics are collected using flow_get API. > - added more documentation in Documentation/howto/dpdk.rst. > v3-v4: > - debug prints moved to netdev-offload-dpdk.c. > - flow get returns full stats, not diff. > - removed additional fields from offload_info and dp_netdev_flow. > - read HW stats during dpif_netdev_flow_get as well as > dpif_netdev_flow_dump_next. > - moved actions offload framework just before support commit. > - fixed memory leak in rewrite code. > - dropped clone commit - will be posted in next series. > v4-v5: > - statistics collecting changed to populate dp_layer and offloaded > attributes. dp_layer changed to "dpdk" for fully offloaded by dpdk. > - display offloaded:partial for partially offloaded flows. > - support filter types "dpdk" and "partially-offloaded" in > dpctl/dump-flows type=X. > - simplify code readability of rewrite commits. > v5-v6: > - debug prints improvement for partial offload installations. dbg prints > for failed actions, warn for other failure. > - fixed memory leak in case retrieved dpdk port-id is invalid. > > Eli Britstein (17): > netdev-offload-dpdk: Refactor flow patterns > netdev-offload-dpdk: Refactor action items freeing scheme > netdev-offload-dpdk: Dynamically allocate pattern items > netdev-offload-dpdk: Improve HW offload flow debuggability > netdev-dpdk: Introduce rte flow query function > netdev-offload-dpdk: Return UFID-rte_flow entry in find method > netdev-offload-dpdk: Framework for actions offload > netdev-offload-dpdk: Implement flow get method > dpctl: Support dump-flows filters "dpdk" and "partially-offloaded" > dpif-netdev: Populate dpif class field in offload struct > netdev-dpdk: Getter function for dpdk port id API > netdev-offload: Introduce a function to validate same flow api handle > netdev-offload-dpdk: Support offload of output action > netdev-offload-dpdk: Support offload of drop action > netdev-offload-dpdk: Support offload of set MAC actions > netdev-offload-dpdk: Support offload of set IPv4 actions > netdev-offload-dpdk: Support offload of set TCP/UDP ports actions > > Ophir Munk (1): > dpif-netdev: Update offloaded flows statistics > > Documentation/howto/dpdk.rst | 21 +- > NEWS | 5 + > lib/dpctl.c | 97 +++-- > lib/dpctl.man | 2 + > lib/dpif-netdev.c | 81 +++- > lib/netdev-dpdk.c | 48 +++ > lib/netdev-dpdk.h | 8 + > lib/netdev-offload-dpdk.c | 964 ++++++++++++++++++++++++++++++++---------- > lib/netdev-offload-provider.h | 1 + > lib/netdev-offload.c | 12 + > 10 files changed, 972 insertions(+), 267 deletions(-) >
On 06.01.2020 10:23, Eli Britstein wrote:
> Ilya, are there more issues to be addressed or can we merge it?
Hi. Sorry, I've been distracted by other issues this week.
I'm going to review and test this series today and tomorrow.
Best regards, Ilya Maximets.
On 19.12.2019 12:54, Eli Britstein wrote: > Currently, netdev datapath offload only accelerates the flow match > sequence by associating a mark per flow. This series introduces the full > offload of netdev datapath flows by having the HW also perform the flow > actions. > > This series adds HW offload for output, drop, set MAC, set IPv4, set > TCP/UDP ports and tunnel push actions using the DPDK rte_flow API. > > v1 Travis passed: https://travis-ci.org/elibritstein/OVS/builds/614511472 > v2 Travis passed: https://travis-ci.org/elibritstein/OVS/builds/619262148 > v3 Travis passed: https://travis-ci.org/elibritstein/OVS/builds/622253870 > v4 Travis passed: https://travis-ci.org/elibritstein/OVS/builds/625654160 > v5 Travis passed: https://travis-ci.org/elibritstein/OVS/builds/626692202 > v6 Travis passed: https://travis-ci.org/elibritstein/OVS/builds/627132194 > > v1-v2: > - fallback to mark/rss in case of any failure of action offload. > - dp_layer changed to "in_hw" in case the flow is offloaded. > - using netdev_ports_get instead of dp_netdev_lookup_port in stats reading. > - using flow_dump_next API instead of a new API. > - validity checks for last action of output and clone. > - count action should be done for drop as well. > - validity check for output action. > - added doc in Documentation/howto/dpdk.rst. > v2-v3: > - removed refactoring for netdev-offload-dpdk-flow.c file. > - dropped flush commits. > - dbg prints rate-limited, and outside lock. > - added validity check for output action - same flow_api handle for both netdevs. > - added a mutex to protect possible concurrent deletion/querying of a flow. > - statistics are collected using flow_get API. > - added more documentation in Documentation/howto/dpdk.rst. > v3-v4: > - debug prints moved to netdev-offload-dpdk.c. > - flow get returns full stats, not diff. > - removed additional fields from offload_info and dp_netdev_flow. > - read HW stats during dpif_netdev_flow_get as well as > dpif_netdev_flow_dump_next. > - moved actions offload framework just before support commit. > - fixed memory leak in rewrite code. > - dropped clone commit - will be posted in next series. > v4-v5: > - statistics collecting changed to populate dp_layer and offloaded > attributes. dp_layer changed to "dpdk" for fully offloaded by dpdk. > - display offloaded:partial for partially offloaded flows. > - support filter types "dpdk" and "partially-offloaded" in > dpctl/dump-flows type=X. > - simplify code readability of rewrite commits. > v5-v6: > - debug prints improvement for partial offload installations. dbg prints > for failed actions, warn for other failure. > - fixed memory leak in case retrieved dpdk port-id is invalid. > > Eli Britstein (17): > netdev-offload-dpdk: Refactor flow patterns > netdev-offload-dpdk: Refactor action items freeing scheme > netdev-offload-dpdk: Dynamically allocate pattern items > netdev-offload-dpdk: Improve HW offload flow debuggability > netdev-dpdk: Introduce rte flow query function > netdev-offload-dpdk: Return UFID-rte_flow entry in find method > netdev-offload-dpdk: Framework for actions offload > netdev-offload-dpdk: Implement flow get method > dpctl: Support dump-flows filters "dpdk" and "partially-offloaded" > dpif-netdev: Populate dpif class field in offload struct > netdev-dpdk: Getter function for dpdk port id API > netdev-offload: Introduce a function to validate same flow api handle > netdev-offload-dpdk: Support offload of output action > netdev-offload-dpdk: Support offload of drop action > netdev-offload-dpdk: Support offload of set MAC actions > netdev-offload-dpdk: Support offload of set IPv4 actions > netdev-offload-dpdk: Support offload of set TCP/UDP ports actions > > Ophir Munk (1): > dpif-netdev: Update offloaded flows statistics > > Documentation/howto/dpdk.rst | 21 +- > NEWS | 5 + > lib/dpctl.c | 97 +++-- > lib/dpctl.man | 2 + > lib/dpif-netdev.c | 81 +++- > lib/netdev-dpdk.c | 48 +++ > lib/netdev-dpdk.h | 8 + > lib/netdev-offload-dpdk.c | 964 ++++++++++++++++++++++++++++++++---------- > lib/netdev-offload-provider.h | 1 + > lib/netdev-offload.c | 12 + > 10 files changed, 972 insertions(+), 267 deletions(-) > Hi. I still didn't reach the actual code review, but I applied the series and tried to make some tests. Few things: 1. Unfortunately, with this patch-set applied, "make check-offloads" doesn't work at all. I didn't investigate yet, but it seems that flow dumping is not working as expected. 2. patch-set needs a rebase on current master. 3. Since commit a13a0209750c ("userspace: Improved packet drop statistics.") you need to add support for explicit drop action as I told previously (OVS_ACTION_ATTR_DROP). You may start fixing/implementing this. Meanwhile, I'll continue my review and investigation. Best regards, Ilya Maximets.
On 1/7/2020 8:46 PM, Ilya Maximets wrote: > On 19.12.2019 12:54, Eli Britstein wrote: >> Currently, netdev datapath offload only accelerates the flow match >> sequence by associating a mark per flow. This series introduces the full >> offload of netdev datapath flows by having the HW also perform the flow >> actions. >> >> This series adds HW offload for output, drop, set MAC, set IPv4, set >> TCP/UDP ports and tunnel push actions using the DPDK rte_flow API. >> >> v1 Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F614511472&data=02%7C01%7Celibr%40mellanox.com%7Cfb80a09d7b9a4b5dc7da08d793a1fac6%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637140196234511900&sdata=UqEGA9tXfFErL6KKI7P5X7Zvm0Krzq9KKTfvyL42T1Q%3D&reserved=0 >> v2 Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F619262148&data=02%7C01%7Celibr%40mellanox.com%7Cfb80a09d7b9a4b5dc7da08d793a1fac6%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637140196234511900&sdata=mCB5n2eGbdhDBax7izO4UdIvdVhjZBgu6hOtF65EfCk%3D&reserved=0 >> v3 Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F622253870&data=02%7C01%7Celibr%40mellanox.com%7Cfb80a09d7b9a4b5dc7da08d793a1fac6%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637140196234511900&sdata=UFfB8mkbOJkCMdk7QRa6CgXbHjlEGVPDx5KAx%2B%2FlLcI%3D&reserved=0 >> v4 Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F625654160&data=02%7C01%7Celibr%40mellanox.com%7Cfb80a09d7b9a4b5dc7da08d793a1fac6%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637140196234511900&sdata=EkGoIBwCfTOb9d2EE5qDf9Akkbr6cK9JD%2BoUhK8kjrY%3D&reserved=0 >> v5 Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F626692202&data=02%7C01%7Celibr%40mellanox.com%7Cfb80a09d7b9a4b5dc7da08d793a1fac6%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637140196234511900&sdata=4UXr57%2BFYhSo4Ujtc5L5F14TligA%2BTQCiM8O7WtheaE%3D&reserved=0 >> v6 Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F627132194&data=02%7C01%7Celibr%40mellanox.com%7Cfb80a09d7b9a4b5dc7da08d793a1fac6%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637140196234511900&sdata=FvyoWaNsKKQR%2FDlM%2BjAWbberGIkKFi1076Z5lQOOmN4%3D&reserved=0 >> >> v1-v2: >> - fallback to mark/rss in case of any failure of action offload. >> - dp_layer changed to "in_hw" in case the flow is offloaded. >> - using netdev_ports_get instead of dp_netdev_lookup_port in stats reading. >> - using flow_dump_next API instead of a new API. >> - validity checks for last action of output and clone. >> - count action should be done for drop as well. >> - validity check for output action. >> - added doc in Documentation/howto/dpdk.rst. >> v2-v3: >> - removed refactoring for netdev-offload-dpdk-flow.c file. >> - dropped flush commits. >> - dbg prints rate-limited, and outside lock. >> - added validity check for output action - same flow_api handle for both netdevs. >> - added a mutex to protect possible concurrent deletion/querying of a flow. >> - statistics are collected using flow_get API. >> - added more documentation in Documentation/howto/dpdk.rst. >> v3-v4: >> - debug prints moved to netdev-offload-dpdk.c. >> - flow get returns full stats, not diff. >> - removed additional fields from offload_info and dp_netdev_flow. >> - read HW stats during dpif_netdev_flow_get as well as >> dpif_netdev_flow_dump_next. >> - moved actions offload framework just before support commit. >> - fixed memory leak in rewrite code. >> - dropped clone commit - will be posted in next series. >> v4-v5: >> - statistics collecting changed to populate dp_layer and offloaded >> attributes. dp_layer changed to "dpdk" for fully offloaded by dpdk. >> - display offloaded:partial for partially offloaded flows. >> - support filter types "dpdk" and "partially-offloaded" in >> dpctl/dump-flows type=X. >> - simplify code readability of rewrite commits. >> v5-v6: >> - debug prints improvement for partial offload installations. dbg prints >> for failed actions, warn for other failure. >> - fixed memory leak in case retrieved dpdk port-id is invalid. >> >> Eli Britstein (17): >> netdev-offload-dpdk: Refactor flow patterns >> netdev-offload-dpdk: Refactor action items freeing scheme >> netdev-offload-dpdk: Dynamically allocate pattern items >> netdev-offload-dpdk: Improve HW offload flow debuggability >> netdev-dpdk: Introduce rte flow query function >> netdev-offload-dpdk: Return UFID-rte_flow entry in find method >> netdev-offload-dpdk: Framework for actions offload >> netdev-offload-dpdk: Implement flow get method >> dpctl: Support dump-flows filters "dpdk" and "partially-offloaded" >> dpif-netdev: Populate dpif class field in offload struct >> netdev-dpdk: Getter function for dpdk port id API >> netdev-offload: Introduce a function to validate same flow api handle >> netdev-offload-dpdk: Support offload of output action >> netdev-offload-dpdk: Support offload of drop action >> netdev-offload-dpdk: Support offload of set MAC actions >> netdev-offload-dpdk: Support offload of set IPv4 actions >> netdev-offload-dpdk: Support offload of set TCP/UDP ports actions >> >> Ophir Munk (1): >> dpif-netdev: Update offloaded flows statistics >> >> Documentation/howto/dpdk.rst | 21 +- >> NEWS | 5 + >> lib/dpctl.c | 97 +++-- >> lib/dpctl.man | 2 + >> lib/dpif-netdev.c | 81 +++- >> lib/netdev-dpdk.c | 48 +++ >> lib/netdev-dpdk.h | 8 + >> lib/netdev-offload-dpdk.c | 964 ++++++++++++++++++++++++++++++++---------- >> lib/netdev-offload-provider.h | 1 + >> lib/netdev-offload.c | 12 + >> 10 files changed, 972 insertions(+), 267 deletions(-) >> > Hi. I still didn't reach the actual code review, but I applied the series > and tried to make some tests. Few things: > 1. Unfortunately, with this patch-set applied, "make check-offloads" doesn't > work at all. I didn't investigate yet, but it seems that flow dumping is > not working as expected. I see it doesn't work on master as well. I'll look for the offending commit. > 2. patch-set needs a rebase on current master. > 3. Since commit a13a0209750c ("userspace: Improved packet drop statistics.") > you need to add support for explicit drop action as I told previously > (OVS_ACTION_ATTR_DROP). OK. > > You may start fixing/implementing this. Meanwhile, I'll continue my review > and investigation. > > Best regards, Ilya Maximets.
On 07.01.2020 20:28, Eli Britstein wrote: > > On 1/7/2020 8:46 PM, Ilya Maximets wrote: >> On 19.12.2019 12:54, Eli Britstein wrote: >>> Currently, netdev datapath offload only accelerates the flow match >>> sequence by associating a mark per flow. This series introduces the full >>> offload of netdev datapath flows by having the HW also perform the flow >>> actions. >>> >>> This series adds HW offload for output, drop, set MAC, set IPv4, set >>> TCP/UDP ports and tunnel push actions using the DPDK rte_flow API. >>> >>> v1 Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F614511472&data=02%7C01%7Celibr%40mellanox.com%7Cfb80a09d7b9a4b5dc7da08d793a1fac6%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637140196234511900&sdata=UqEGA9tXfFErL6KKI7P5X7Zvm0Krzq9KKTfvyL42T1Q%3D&reserved=0 >>> v2 Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F619262148&data=02%7C01%7Celibr%40mellanox.com%7Cfb80a09d7b9a4b5dc7da08d793a1fac6%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637140196234511900&sdata=mCB5n2eGbdhDBax7izO4UdIvdVhjZBgu6hOtF65EfCk%3D&reserved=0 >>> v3 Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F622253870&data=02%7C01%7Celibr%40mellanox.com%7Cfb80a09d7b9a4b5dc7da08d793a1fac6%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637140196234511900&sdata=UFfB8mkbOJkCMdk7QRa6CgXbHjlEGVPDx5KAx%2B%2FlLcI%3D&reserved=0 >>> v4 Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F625654160&data=02%7C01%7Celibr%40mellanox.com%7Cfb80a09d7b9a4b5dc7da08d793a1fac6%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637140196234511900&sdata=EkGoIBwCfTOb9d2EE5qDf9Akkbr6cK9JD%2BoUhK8kjrY%3D&reserved=0 >>> v5 Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F626692202&data=02%7C01%7Celibr%40mellanox.com%7Cfb80a09d7b9a4b5dc7da08d793a1fac6%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637140196234511900&sdata=4UXr57%2BFYhSo4Ujtc5L5F14TligA%2BTQCiM8O7WtheaE%3D&reserved=0 >>> v6 Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F627132194&data=02%7C01%7Celibr%40mellanox.com%7Cfb80a09d7b9a4b5dc7da08d793a1fac6%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637140196234511900&sdata=FvyoWaNsKKQR%2FDlM%2BjAWbberGIkKFi1076Z5lQOOmN4%3D&reserved=0 >>> >>> v1-v2: >>> - fallback to mark/rss in case of any failure of action offload. >>> - dp_layer changed to "in_hw" in case the flow is offloaded. >>> - using netdev_ports_get instead of dp_netdev_lookup_port in stats reading. >>> - using flow_dump_next API instead of a new API. >>> - validity checks for last action of output and clone. >>> - count action should be done for drop as well. >>> - validity check for output action. >>> - added doc in Documentation/howto/dpdk.rst. >>> v2-v3: >>> - removed refactoring for netdev-offload-dpdk-flow.c file. >>> - dropped flush commits. >>> - dbg prints rate-limited, and outside lock. >>> - added validity check for output action - same flow_api handle for both netdevs. >>> - added a mutex to protect possible concurrent deletion/querying of a flow. >>> - statistics are collected using flow_get API. >>> - added more documentation in Documentation/howto/dpdk.rst. >>> v3-v4: >>> - debug prints moved to netdev-offload-dpdk.c. >>> - flow get returns full stats, not diff. >>> - removed additional fields from offload_info and dp_netdev_flow. >>> - read HW stats during dpif_netdev_flow_get as well as >>> dpif_netdev_flow_dump_next. >>> - moved actions offload framework just before support commit. >>> - fixed memory leak in rewrite code. >>> - dropped clone commit - will be posted in next series. >>> v4-v5: >>> - statistics collecting changed to populate dp_layer and offloaded >>> attributes. dp_layer changed to "dpdk" for fully offloaded by dpdk. >>> - display offloaded:partial for partially offloaded flows. >>> - support filter types "dpdk" and "partially-offloaded" in >>> dpctl/dump-flows type=X. >>> - simplify code readability of rewrite commits. >>> v5-v6: >>> - debug prints improvement for partial offload installations. dbg prints >>> for failed actions, warn for other failure. >>> - fixed memory leak in case retrieved dpdk port-id is invalid. >>> >>> Eli Britstein (17): >>> netdev-offload-dpdk: Refactor flow patterns >>> netdev-offload-dpdk: Refactor action items freeing scheme >>> netdev-offload-dpdk: Dynamically allocate pattern items >>> netdev-offload-dpdk: Improve HW offload flow debuggability >>> netdev-dpdk: Introduce rte flow query function >>> netdev-offload-dpdk: Return UFID-rte_flow entry in find method >>> netdev-offload-dpdk: Framework for actions offload >>> netdev-offload-dpdk: Implement flow get method >>> dpctl: Support dump-flows filters "dpdk" and "partially-offloaded" >>> dpif-netdev: Populate dpif class field in offload struct >>> netdev-dpdk: Getter function for dpdk port id API >>> netdev-offload: Introduce a function to validate same flow api handle >>> netdev-offload-dpdk: Support offload of output action >>> netdev-offload-dpdk: Support offload of drop action >>> netdev-offload-dpdk: Support offload of set MAC actions >>> netdev-offload-dpdk: Support offload of set IPv4 actions >>> netdev-offload-dpdk: Support offload of set TCP/UDP ports actions >>> >>> Ophir Munk (1): >>> dpif-netdev: Update offloaded flows statistics >>> >>> Documentation/howto/dpdk.rst | 21 +- >>> NEWS | 5 + >>> lib/dpctl.c | 97 +++-- >>> lib/dpctl.man | 2 + >>> lib/dpif-netdev.c | 81 +++- >>> lib/netdev-dpdk.c | 48 +++ >>> lib/netdev-dpdk.h | 8 + >>> lib/netdev-offload-dpdk.c | 964 ++++++++++++++++++++++++++++++++---------- >>> lib/netdev-offload-provider.h | 1 + >>> lib/netdev-offload.c | 12 + >>> 10 files changed, 972 insertions(+), 267 deletions(-) >>> >> Hi. I still didn't reach the actual code review, but I applied the series >> and tried to make some tests. Few things: >> 1. Unfortunately, with this patch-set applied, "make check-offloads" doesn't >> work at all. I didn't investigate yet, but it seems that flow dumping is >> not working as expected. > I see it doesn't work on master as well. I'll look for the offending commit. It works on master (a13a02097) for me. Doesn't work with patch-set applied. >> 2. patch-set needs a rebase on current master. >> 3. Since commit a13a0209750c ("userspace: Improved packet drop statistics.") >> you need to add support for explicit drop action as I told previously >> (OVS_ACTION_ATTR_DROP). > OK. >> >> You may start fixing/implementing this. Meanwhile, I'll continue my review >> and investigation. >> >> Best regards, Ilya Maximets.
Ilya and Eli, I have some additional comments on the v6 series. 1. The description says that tunnel push action is supported by this series but v3-v4 note says that - dropped clone commit - will be posted in the next series. In that case, should the description of patcheset be updated to not include generic tunnel push action? 2. Can you outline the procedure for testing output action for VF-representors? How will this patchset be used in conjunction with VF-representors? It is not clear from the description. Hemal On Tue, Jan 7, 2020 at 10:47 AM Ilya Maximets <i.maximets@ovn.org> wrote: > On 19.12.2019 12:54, Eli Britstein wrote: > > Currently, netdev datapath offload only accelerates the flow match > > sequence by associating a mark per flow. This series introduces the full > > offload of netdev datapath flows by having the HW also perform the flow > > actions. > > > > This series adds HW offload for output, drop, set MAC, set IPv4, set > > TCP/UDP ports and tunnel push actions using the DPDK rte_flow API. > > > > v1 Travis passed: > https://travis-ci.org/elibritstein/OVS/builds/614511472 > > v2 Travis passed: > https://travis-ci.org/elibritstein/OVS/builds/619262148 > > v3 Travis passed: > https://travis-ci.org/elibritstein/OVS/builds/622253870 > > v4 Travis passed: > https://travis-ci.org/elibritstein/OVS/builds/625654160 > > v5 Travis passed: > https://travis-ci.org/elibritstein/OVS/builds/626692202 > > v6 Travis passed: > https://travis-ci.org/elibritstein/OVS/builds/627132194 > > > > v1-v2: > > - fallback to mark/rss in case of any failure of action offload. > > - dp_layer changed to "in_hw" in case the flow is offloaded. > > - using netdev_ports_get instead of dp_netdev_lookup_port in stats > reading. > > - using flow_dump_next API instead of a new API. > > - validity checks for last action of output and clone. > > - count action should be done for drop as well. > > - validity check for output action. > > - added doc in Documentation/howto/dpdk.rst. > > v2-v3: > > - removed refactoring for netdev-offload-dpdk-flow.c file. > > - dropped flush commits. > > - dbg prints rate-limited, and outside lock. > > - added validity check for output action - same flow_api handle for both > netdevs. > > - added a mutex to protect possible concurrent deletion/querying of a > flow. > > - statistics are collected using flow_get API. > > - added more documentation in Documentation/howto/dpdk.rst. > > v3-v4: > > - debug prints moved to netdev-offload-dpdk.c. > > - flow get returns full stats, not diff. > > - removed additional fields from offload_info and dp_netdev_flow. > > - read HW stats during dpif_netdev_flow_get as well as > > dpif_netdev_flow_dump_next. > > - moved actions offload framework just before support commit. > > - fixed memory leak in rewrite code. > > - dropped clone commit - will be posted in next series. > > v4-v5: > > - statistics collecting changed to populate dp_layer and offloaded > > attributes. dp_layer changed to "dpdk" for fully offloaded by dpdk. > > - display offloaded:partial for partially offloaded flows. > > - support filter types "dpdk" and "partially-offloaded" in > > dpctl/dump-flows type=X. > > - simplify code readability of rewrite commits. > > v5-v6: > > - debug prints improvement for partial offload installations. dbg prints > > for failed actions, warn for other failure. > > - fixed memory leak in case retrieved dpdk port-id is invalid. > > > > Eli Britstein (17): > > netdev-offload-dpdk: Refactor flow patterns > > netdev-offload-dpdk: Refactor action items freeing scheme > > netdev-offload-dpdk: Dynamically allocate pattern items > > netdev-offload-dpdk: Improve HW offload flow debuggability > > netdev-dpdk: Introduce rte flow query function > > netdev-offload-dpdk: Return UFID-rte_flow entry in find method > > netdev-offload-dpdk: Framework for actions offload > > netdev-offload-dpdk: Implement flow get method > > dpctl: Support dump-flows filters "dpdk" and "partially-offloaded" > > dpif-netdev: Populate dpif class field in offload struct > > netdev-dpdk: Getter function for dpdk port id API > > netdev-offload: Introduce a function to validate same flow api handle > > netdev-offload-dpdk: Support offload of output action > > netdev-offload-dpdk: Support offload of drop action > > netdev-offload-dpdk: Support offload of set MAC actions > > netdev-offload-dpdk: Support offload of set IPv4 actions > > netdev-offload-dpdk: Support offload of set TCP/UDP ports actions > > > > Ophir Munk (1): > > dpif-netdev: Update offloaded flows statistics > > > > Documentation/howto/dpdk.rst | 21 +- > > NEWS | 5 + > > lib/dpctl.c | 97 +++-- > > lib/dpctl.man | 2 + > > lib/dpif-netdev.c | 81 +++- > > lib/netdev-dpdk.c | 48 +++ > > lib/netdev-dpdk.h | 8 + > > lib/netdev-offload-dpdk.c | 964 > ++++++++++++++++++++++++++++++++---------- > > lib/netdev-offload-provider.h | 1 + > > lib/netdev-offload.c | 12 + > > 10 files changed, 972 insertions(+), 267 deletions(-) > > > > Hi. I still didn't reach the actual code review, but I applied the series > and tried to make some tests. Few things: > 1. Unfortunately, with this patch-set applied, "make check-offloads" > doesn't > work at all. I didn't investigate yet, but it seems that flow dumping > is > not working as expected. > 2. patch-set needs a rebase on current master. > 3. Since commit a13a0209750c ("userspace: Improved packet drop > statistics.") > you need to add support for explicit drop action as I told previously > (OVS_ACTION_ATTR_DROP). > > You may start fixing/implementing this. Meanwhile, I'll continue my review > and investigation. > > Best regards, Ilya Maximets. > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 1/8/2020 7:54 PM, Hemal Shah wrote: Ilya and Eli, I have some additional comments on the v6 series. 1. The description says that tunnel push action is supported by this series but v3-v4 note says that - dropped clone commit - will be posted in the next series. In that case, should the description of patcheset be updated to not include generic tunnel push action? Right. Sorry. I'll remove this description in v7. 1. Can you outline the procedure for testing output action for VF-representors? How will this patchset be used in conjunction with VF-representors? It is not clear from the description. Yes. You should attach the representors as OVS ports, and set hw-offload=true. VFs can be used by VMs, for example. When a packet is placed on a VF (for example by a VM), it is received by OVS by the representor, according to the default miss rule in HW. OVS configure flows for its ports (i.e. representors), and those are configured to the HW by rte_flow (in this series). Next packets in those flows are handled by HW, and not received by the representors, so transparent to OVS. Still, OVS query the flow statistics, in order to keep the flow alive as long as there is traffic, and age it out after a timeout with no traffic. Hemal On Tue, Jan 7, 2020 at 10:47 AM Ilya Maximets <i.maximets@ovn.org<mailto:i.maximets@ovn.org>> wrote: On 19.12.2019 12:54, Eli Britstein wrote: > Currently, netdev datapath offload only accelerates the flow match > sequence by associating a mark per flow. This series introduces the full > offload of netdev datapath flows by having the HW also perform the flow > actions. > > This series adds HW offload for output, drop, set MAC, set IPv4, set > TCP/UDP ports and tunnel push actions using the DPDK rte_flow API. > > v1 Travis passed: https://travis-ci.org/elibritstein/OVS/builds/614511472<https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F614511472&data=02%7C01%7Celibr%40mellanox.com%7C7e3d545c199242cbc75708d79463f349%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637141029321186533&sdata=DcB8XePoHhA6S672Au0Bi6NEldY%2FG06u0uf9uRYr%2Fn8%3D&reserved=0> > v2 Travis passed: https://travis-ci.org/elibritstein/OVS/builds/619262148<https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F619262148&data=02%7C01%7Celibr%40mellanox.com%7C7e3d545c199242cbc75708d79463f349%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637141029321186533&sdata=d4sZKYxZw0KFTlBSqehapVMWzxB4EixdPtAlB1%2Ft2ko%3D&reserved=0> > v3 Travis passed: https://travis-ci.org/elibritstein/OVS/builds/622253870<https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F622253870&data=02%7C01%7Celibr%40mellanox.com%7C7e3d545c199242cbc75708d79463f349%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637141029321196529&sdata=AOObeX2bdEWw1Cj7XK%2B3wxm%2F%2F0n6eziX5IPXuZXjw40%3D&reserved=0> > v4 Travis passed: https://travis-ci.org/elibritstein/OVS/builds/625654160<https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F625654160&data=02%7C01%7Celibr%40mellanox.com%7C7e3d545c199242cbc75708d79463f349%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637141029321196529&sdata=3TXMD65NiGFT2njPnaJPQZ2CTFEl51uYslTlzhR4t4E%3D&reserved=0> > v5 Travis passed: https://travis-ci.org/elibritstein/OVS/builds/626692202<https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F626692202&data=02%7C01%7Celibr%40mellanox.com%7C7e3d545c199242cbc75708d79463f349%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637141029321196529&sdata=GBF4RlpU8qnXPSFt0gEH0Ps8h1Z47Dr5U7bQanvuOwQ%3D&reserved=0> > v6 Travis passed: https://travis-ci.org/elibritstein/OVS/builds/627132194<https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F627132194&data=02%7C01%7Celibr%40mellanox.com%7C7e3d545c199242cbc75708d79463f349%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637141029321206521&sdata=DYlhJxQgxX6xQ%2Bv4VxViFn9sgovT2b%2FMwVQRYJXoIO8%3D&reserved=0> > > v1-v2: > - fallback to mark/rss in case of any failure of action offload. > - dp_layer changed to "in_hw" in case the flow is offloaded. > - using netdev_ports_get instead of dp_netdev_lookup_port in stats reading. > - using flow_dump_next API instead of a new API. > - validity checks for last action of output and clone. > - count action should be done for drop as well. > - validity check for output action. > - added doc in Documentation/howto/dpdk.rst. > v2-v3: > - removed refactoring for netdev-offload-dpdk-flow.c file. > - dropped flush commits. > - dbg prints rate-limited, and outside lock. > - added validity check for output action - same flow_api handle for both netdevs. > - added a mutex to protect possible concurrent deletion/querying of a flow. > - statistics are collected using flow_get API. > - added more documentation in Documentation/howto/dpdk.rst. > v3-v4: > - debug prints moved to netdev-offload-dpdk.c. > - flow get returns full stats, not diff. > - removed additional fields from offload_info and dp_netdev_flow. > - read HW stats during dpif_netdev_flow_get as well as > dpif_netdev_flow_dump_next. > - moved actions offload framework just before support commit. > - fixed memory leak in rewrite code. > - dropped clone commit - will be posted in next series. > v4-v5: > - statistics collecting changed to populate dp_layer and offloaded > attributes. dp_layer changed to "dpdk" for fully offloaded by dpdk. > - display offloaded:partial for partially offloaded flows. > - support filter types "dpdk" and "partially-offloaded" in > dpctl/dump-flows type=X. > - simplify code readability of rewrite commits. > v5-v6: > - debug prints improvement for partial offload installations. dbg prints > for failed actions, warn for other failure. > - fixed memory leak in case retrieved dpdk port-id is invalid. > > Eli Britstein (17): > netdev-offload-dpdk: Refactor flow patterns > netdev-offload-dpdk: Refactor action items freeing scheme > netdev-offload-dpdk: Dynamically allocate pattern items > netdev-offload-dpdk: Improve HW offload flow debuggability > netdev-dpdk: Introduce rte flow query function > netdev-offload-dpdk: Return UFID-rte_flow entry in find method > netdev-offload-dpdk: Framework for actions offload > netdev-offload-dpdk: Implement flow get method > dpctl: Support dump-flows filters "dpdk" and "partially-offloaded" > dpif-netdev: Populate dpif class field in offload struct > netdev-dpdk: Getter function for dpdk port id API > netdev-offload: Introduce a function to validate same flow api handle > netdev-offload-dpdk: Support offload of output action > netdev-offload-dpdk: Support offload of drop action > netdev-offload-dpdk: Support offload of set MAC actions > netdev-offload-dpdk: Support offload of set IPv4 actions > netdev-offload-dpdk: Support offload of set TCP/UDP ports actions > > Ophir Munk (1): > dpif-netdev: Update offloaded flows statistics > > Documentation/howto/dpdk.rst | 21 +- > NEWS | 5 + > lib/dpctl.c | 97 +++-- > lib/dpctl.man | 2 + > lib/dpif-netdev.c | 81 +++- > lib/netdev-dpdk.c | 48 +++ > lib/netdev-dpdk.h | 8 + > lib/netdev-offload-dpdk.c | 964 ++++++++++++++++++++++++++++++++---------- > lib/netdev-offload-provider.h | 1 + > lib/netdev-offload.c | 12 + > 10 files changed, 972 insertions(+), 267 deletions(-) > Hi. I still didn't reach the actual code review, but I applied the series and tried to make some tests. Few things: 1. Unfortunately, with this patch-set applied, "make check-offloads" doesn't work at all. I didn't investigate yet, but it seems that flow dumping is not working as expected. 2. patch-set needs a rebase on current master. 3. Since commit a13a0209750c ("userspace: Improved packet drop statistics.") you need to add support for explicit drop action as I told previously (OVS_ACTION_ATTR_DROP). You may start fixing/implementing this. Meanwhile, I'll continue my review and investigation. Best regards, Ilya Maximets.