[ovs-dev] [PATCH ovs V2 00/21] Introducing HW offload support for openvswitch

[Joe Stringer]

On 25 December 2016 at 03:39, Paul Blakey <paulb at mellanox.com> 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 ;)

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
* 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?
* 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.
* 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
* There's still some instances where entire functions have been copied
to other files instead of refactoring them into central locations for
reuse.
* 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? 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