On 10 January 2017 at 10:07, Joe Stringer <j...@ovn.org> wrote: > On 8 January 2017 at 07:18, Paul Blakey <pa...@mellanox.com> wrote: >> >> On 05/01/2017 02:11, Joe Stringer wrote: >> >>> On 25 December 2016 at 03:39, Paul Blakey <pa...@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 ;) >> >> 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. > > I discussed this with a few people offline and it seems that the most > reasonable way to handle it for dump is just to dump from netdevs > anyway. You can fetch the list of relevant ports from OVS datapath in > kernel, then only dump those ports. > > For adding and deleting flows, using ovs-dpctl is not really > supported, it's primarily for debug. ovs-vswitchd is going to override > that behaviour anyway - if you add a flow, it may delete it; if you > delete a flow, it may re-add it. I suggest that for debugging purposes > the default behaviour is to install into OVS, but if you want to put > it into TC sw/hw, provide an additional flag on the commandline. > > If you want ovs-vswitchd to decide how to install the flow, you can > always use "ovs-appctl dpctl/add-flow ..." and the command will go via > ovs-vswitchd, which will make the offloading decision based on the > same logic as it would when handling regular upcalls. > >>> * 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? > > At the time, I was thinking just stats. > > However I think it would also be useful to have some flag for > ovs-dpctl that chooses which flows to fetch. > > Something like: > ovs-dpctl dump-flows: Dump all flows from both OVS and TC (flower only). > ovs-dpctl dump-flows --only-flower > ovs-dpctl dump-flows --only-ovs > > Each flow when printed should also indicate in some way where it came from.
Could be something like ovs-dpctl dump-flows --type=flower instead, to be a bit more generic. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev