On 23/03/2017 09:01, Joe Stringer wrote:
On 22 March 2017 at 04:10, Roi Dayan <[email protected]
<mailto:[email protected]>> wrote:
This patch series introduces rule offload functionality to dpif-netlink
via netdev ports new flow offloading API. The user can specify whether to
enable rule offloading or not via OVS configuration. Netdev providers
are able to implement netdev flow offload API in order to offload rules.
This patch series also implements one offload scheme for netdev-linux,
using TC flower classifier, which was chosen because its sort of natural
to state OVS DP rules for this classifier. However, the code can be
extended to support other classifiers such as U32, eBPF, etc which support
offload as well.
The use-case we are currently addressing is the newly sriov switchdev mode
in the Linux kernel which was introduced in version 4.8 [1][2].
This series was tested against sriov vfs vports representors of the
Mellanox 100G ConnectX-4 series exposed by the mlx5 kernel driver.
V4->V5:
- Fix compat
- Fix VXLAN IPv6 tunnel matching
- Fix order of actions in dump flows
- Update ovs-dpctl man page about the addtion of type to dump-flows
Travis
https://travis-ci.org/roidayan/ovs/builds/213735371
AppVeyor
https://ci.appveyor.com/project/roidayan/ovs/build/1.0.18
Hi Roi,
This series does not compile on the OVS master:
https://travis-ci.org/joestringer/openvswitch/builds/214039943
Hi Joe,
right. didn't rebase over latest master. We'll check it out.
I ran the make check-offloads tests on a recent net-next kernel and it
failed, output was not as expected:
../../tests/system-offloaded-traffic.at:54
<http://system-offloaded-traffic.at:54>: ovs-appctl dpctl/dump-flows |
grep "eth_type(0x0800)" | sed -e
's/used:[0-9].[0-9]*s/used:0.001s/;s/eth(src=[a-z0-9:]*,dst=[a-z0-9:]*)/eth(mac
s)/;s/actions:[0-9,]*/actions:output/;s/recirc_id(0),//' | sort
--- - 2017-03-22 16:43:37.598689692 -0700
+++
/home/vagrant/ovs/_build-clang/tests/system-offloads-testsuite.dir/at-groups/2/stdout
2017-03-22 16:43:37.595628000 -0700
@@ -1,3 +1,3 @@
-in_port(2),eth(macs),eth_type(0x0800), packets:9, bytes:756,
used:0.001s, actions:output
-in_port(3),eth(macs),eth_type(0x0800), packets:9, bytes:756,
used:0.001s, actions:output
+in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:9,
bytes:882, used:0.001s, actions:output
+in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:9,
bytes:882, used:0.001s, actions:output
we'll check it.
Did you end up allowing other_config:hw-offload to be configured at
runtime? The tests seem to be assuming this, but the documentation still
says that OVS must be restarted to enable offloads.
It's possible to change to true at runtime but not back to false because
of the possible mixup that could be between offloaded rules and
non-offloaded rules that we aren't handling currently. e.g. should
probably flush all offloaded rules so they will be added back non-offloaded.
Because of this we left the comment in the doc.
The testsuite is an encouraging sign. I can only imagine that it's not
attempting to comprehensively cover OVS flow translation to the tc
flower API, because of its simplicity. Presumably you intend to improve
this over time though?
of course. this is a very simple example for a test. this example test
should probably be improved as well.
At a high level, the functionality isn't particularly compelling at this
stage. OVS has a huge level of programmability, and the limits applied
in this patch restrict the offloads to quite a small subset of use
cases. That said, I believe I've mentioned off-list a couple of times
that if there is broad interest in the OVS community regarding this
series, and that interest is shown (for instance, by people trying out
the patches, providing review, etc) then that's an encouraging sign that
this feature is providing useful functionality---and therefore should be
merged.
For reference, the kinds of use cases that I think could make this
series more compelling include stateful connection tracking and tunnel
termination, in particular with TLV support (for Geneve, NSH).
The biggest concern from a maintainership point of view is the potential
burden on reviews and support that this may introduce. Your approach of
making this disabled by default and configurable in the database makes
this easier, as this restricts the potential impact on regular users.
I've heard arguments on both sides of this, but effectively if over time
the usefulness is low and maintenance burden is high, we'd be inclined
to remove this again; rather, if it becomes more useful over time and we
get active involvement from the community for ongoing patch review and
assisting users that run into trouble, then that would make the
maintainers job easier.
thanks for the feedback,
Roi
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
