On 4/27/22 04:41, Jianbo Liu wrote: > 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?
I agree that meters do not belong to any particular netdev, that's true. However, netdev_flow_api is also a global entity. All netdevs are using the same instance of the netdev_fow_api. And most of the structures in netdev-offload-tc are global (hash maps, configurations). 'netdev' argument can just be viewed as a guide for these callbacks, like meter_id for the meter offload API, not something necessary. (I guess, we could rename netdev_flow_api to just offload_api to make this more clear, but let's not think about that too much for now to not overcomplicate things.) meter_offload_init/destory APIs might not be even needed. meter_tc_init_policer can be called directly from the netdev_tc_init_flow_api, if made re-enter-able (if (meter_police_ids) { return; }), or can be called from under the existing ovsthread_once_start(&once). meter_offload_destory in current implementation only frees the id pool. This might be not necessary for the following reasons: - If datapath is destroyed, but the process is still running, it's likely that we're waiting for the creation of a new datapath that will re-use the id-pool. - If we're going to shut down the OVS, it's nice but not necessary to free all the memory before doing so. Since the id-pool is a global variable, that will not be treated as a memory leak by sanitizers. What do you think? Forgot to mention in a previous reply that the patch 8/8 of the patch set should add a NEWS entry for the new feature (the NEWS file is a bit mangled at the moment, but Alin is going to fix it soon). Some test in the tests/system-offloads-traffic.at would be also nice to have, if possible. > > 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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev