On 4/26/22 09:14, Simon Horman wrote: > On Tue, Apr 19, 2022 at 01:38:33AM +0000, Jianbo Liu via dev wrote: >> On Thu, 2022-04-07 at 09:15 +0000, Jianbo Liu wrote: >>> This series is to add support for tc offloading of ovs metering, and >>> enhance OVS to use new kernel feature which offload tc police action to >>> hardware. >>> To do the offloading, new APIs for meter are added in netdev-offload, >>> and OVS meters are mapped to tc police actions with one-to-one >>> relationship for dpif-netlink. >>> >>> Notes: >>> v3 >>> - Add netdev-offload APIs for meter. >>> - Move the implementation to lower netdev-offload-tc layer. >>> >>> v2 >>> - Move tc police parse call from last patch to 2nd patch. >>> 2nd patch is adding the parse lib func and add the call to it in >>> that patch. >>> Last patch is the put of tc police act. >>> In 2nd patch also add empty switch case for tc police act put as >>> the impl. >>> is done in the last patch when support for put is being added. > > ... > >> Friendly ping for review. > > Hi Jianbo, > > sorry for the delayed response. > > My colleagues at Corigine have exercised this patchset and have verified > it for our test-cases. > > Also, I have run the upstream test workflows over them, and they look good. > > 1. netdev-offload: Add meter offload provider > https://github.com/horms2/ovs/actions/runs/2220399423 > 2. tc: Add support parsing tc police action > https://github.com/horms2/ovs/actions/runs/2220409233 > 3. netdev-linux: Refactor put police action netlink message > https://github.com/horms2/ovs/actions/runs/2220516986 > 4. netdev-linux: Add functions to manipulate tc police action > https://github.com/horms2/ovs/actions/runs/2220566774 > 5. netdev-offload-tc: Implement and register meter offload API for tc > https://github.com/horms2/ovs/actions/runs/2220568491 > 6. netdev-offload-tc: Cleanup police actions with reserved indexes on startup > https://github.com/horms2/ovs/actions/runs/2220569761 > 7. netdev-offload-tc: Offloading rules with police actions > https://github.com/horms2/ovs/actions/runs/2220571039 > 8. dpif-netlink: Offloading meter to tc police action > https://github.com/horms2/ovs/actions/runs/2220572904 > > I have also reviewed the patches, and with my OVS maintainer hat on, I'm > happy to apply them, and plan to do so in the coming days unless there are > objections.
Thanks, Simon, for taking care of this patch set! And sorry for me not being very responsive. I had a quick look over the patch set just now and I think that in this version most of my main concerns were addressed and it looks good in general. One thing I didn't understand is why we need to have 'struct meter_offload_api' as a separate entity? I mean, it never used if a normal offload API is not initialized and it's tied with the particular offload API. Can these callbacks be just part of the normal 'struct netdev_flow_api' ? Should not be hard to merge meter_offload_api and netdev_flow_api into one. That should also reduce the amount of code. Or am I missing something? Another small nit is that, I guess, lib/tc.c is a better home for most of the tc_police functions from lib/netdev-linux.c. But I agree that they are already mixed up and it's kind of a separate task to split them. So, it's not really a concern for this patch set. > > One area that we, at Corigine, would like to follow-up on is using a > revalidation process rather than evicting rules on start-up. If TC police deletion failures can happen, that can become a problem for long-running setups indeed. Not sure what the best solution should be, but I agree that this case has to be handled in some way. Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
