On 05/01/2017 02:11, Joe Stringer wrote:
On 25 December 2016 at 03:39, Paul Blakey <[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 natrual 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.
changes from V1
- Added generic netdev flow offloads API.
- Implemented relevant flow API in netdev-linux (and netdev-vport).
- Added a other_config hw-offload option to enable offloading (defaults to
false).
- Fixed coding style to conform with OVS.
- Policy removed for now. (Will be discussed how best implemented later).
Hi Paul,
Happy holidays to you too ;)
Hi :)
Thanks for your thorough review, We are working on resubmitting the next
patch set and I'll
try and reply as much as I can right now, but this is a big review, so
I'll split this to several replies.
A few high level comments:
* Overall the patchset is looking in better state than previously.
* That said, on platforms other than your specific test environment
OVS fails to build.
* netdev-vport seems to have acquired platform-specific code
Thanks, We've seen your links for travis-ci and app veyor, we'll create an
account and fix it accordingly.
* I don't think that ovs-dpctl should read the database. One of the
nice things about ovs-dpctl is that it is currently standalone, ie
doesn't require anything else to be running to be able to query the
state of the kernel datapath. What are you trying to achieve with
this? Perhaps you can extend the ovs-appctl dpctl/... commands instead
to provide the functionality you need?
We have two config options that isn't saved with the kernel datapath
(and dumped back on dpif open):
if offload is enabled to know if we even need to dump from netdevs or
not for dump-flows cmd (I guess we could
just dump the netdev anyway) and for add-flow, and tc flower flag
skip_hw/skip_sw that is also needed for add-flow cmd,
so we can know what flag we be used while inserting to flower if
offloading is enabled and possible.
* It would be good to see at least some basic system tests ala
tests/system-traffic.at or tests/system-ovn.at. Maybe there could be
tests/system-tc.at that uses SKIP_HW to show that flows can be
installed and that they work.
Sure.
* I think it would be great to get some ability to see what's
happening with flow offloads; eg "ovs-appctl dpctl/show-hw-offloads"
that could give you some idea of how many flows are being offloaded
You mean a number/stats or actually dump only offloaded flows?
* There's still some instances where entire functions have been copied
to other files instead of refactoring them into central locations for
reuse.
You're right this will be taken care of.
* There's still a few remaining style issues, some which
utilities/checkpatch.py picks up, and more which I just noticed at a
glance (extra memsets after zalloc, extra unnecessary variables,
unused functions, style like "for(;;) { if { ... } break; }",
functions that return an int but only ever return 0)
I applied your patches and pushed to my github, and launched these
builds, you can see some failures here:
https://travis-ci.org/joestringer/openvswitch/builds/188626142
https://ci.appveyor.com/project/joestringer/openvswitch
Please use these CI systems to validate that the code builds on at
least the basic platforms. All it takes is a github account with copy
of OVS to push to, and then you can login and run builds on travis and
appveyor with your github account.
For the Travis issues, it looks like you've introduced a dependency on
Kernel headers v4.9. However, userspace should be able to compile on
much earlier versions of OVS. There's a couple of different ways to
handle this, but I think that the appropriate method is to make a copy
of the header available in include/linux/foo/bar.h, with #ifdefs that
check the existence of the file, and #include_next to include the
system version if available, or if it's not available, define the
symbols we need. There's a bunch of examples in
datapath/linux/compat/include/, eg
datapath/linux/compat/include/linux/u64_stats_sync.h that show how
this is done. I think we should avoid checks against kernel version
though in the userspace portions.
For the Appveyor (windows) builds, it looks like platform-specific
code has been added to lib/netdev-vport.c that breaks it. I figure
that the reason you are looking for offload support in this netdev
type is for the ability to set tc flows on tunnel devices?
Right, and yes, we will fix these issues.
There is
another ongoing effort to add support for the newer rtnetlink tunnel
KAPI which may influence this piece. I'm not sure whether the idea
behind that support is to add the rtnetlink-based tunnel devices to
netdev-linux or netdev-vport; I think that Eric Garver might be able
to shed some light on that. Based on that discussion we can figure out
how to allow such offloads.
This isn't comprehensive but should be a good start point for further
discussion.
Cheers,
Joe
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
