On Wed, 2022-04-27 at 01:58 +0200, Ilya Maximets wrote: > 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220399423&data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=WiPBTWZX1XsWskvmW9RPgH6VU3znLRcLq894G8cYGUw%3D&reserved=0 > > 2. tc: Add support parsing tc police action > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220409233&data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=jeMI5%2F9CN%2B2lXFEL%2Bu3L1YkENYoNB9iudLukRZHv1cU%3D&reserved=0 > > 3. netdev-linux: Refactor put police action netlink message > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220516986&data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vXnK87IOXkdQt%2BDYG90ksy0CWZHfH7lo%2FmdHCXTu6Bc%3D&reserved=0 > > 4. netdev-linux: Add functions to manipulate tc police action > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220566774&data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=iqFPTqegSUQX35jkLzjCDavPWrzTTX3Bg%2FgZEdDCHho%3D&reserved=0 > > 5. netdev-offload-tc: Implement and register meter offload API for > > tc > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220568491&data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=sZ5izlE%2FYu9wfy%2B1q7wwzyn5JDE2MeH8OzTKilE7Fqg%3D&reserved=0 > > 6. netdev-offload-tc: Cleanup police actions with reserved indexes > > on startup > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220569761&data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=ODy0cbf5MvUFZ%2F61%2F49wMNTzkXhOFIZinhcVQmG78I0%3D&reserved=0 > > 7. netdev-offload-tc: Offloading rules with police actions > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220571039&data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=pvaCfZZD3j0rBL7jw8olMJxRz4%2FNBbE81jkZqgOyCzk%3D&reserved=0 > > 8. dpif-netlink: Offloading meter to tc police action > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220572904&data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=k4r3DT2R%2FzcbrByDcj8LrIEcjPkgxh%2BtXLNPc8XT%2FLA%3D&reserved=0 > > > > 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?
Conceptually, meter is used globally, and it does not belong to any perticular netdev/flow. All the callbacks in 'struct netdev_flow_api' have a parameter related to netdev, but meter callbacks don't need. The meter_offload_set/get/del can be part of 'struct netdev_flow_api', but meter_offload_init/destory must be placed to elsewhere, to avoid being called more than once. So, why not add new API for meter? But I'd like to change if you insist. What's your decision? Thanks! Jianbo > > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
