On Wed, 2022-04-27 at 13:37 +0200, Ilya Maximets wrote: > 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%7Ccb6b0d3e0c3c4d3d295c08da284260d3%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866562838020340%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=8lRjZK35bENpZ3vRbMFBfGWJLXX4p%2BfjR57JEhADucw%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%7Ccb6b0d3e0c3c4d3d295c08da284260d3%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866562838020340%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=iorfBm3gxzS3fILCBF8h18%2FWLSUQXEhc2g7jWMn%2F4ug%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%7Ccb6b0d3e0c3c4d3d295c08da284260d3%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866562838020340%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=9%2BKMNZqAe6CM7ysWXwbeFpIyf%2FF44NdSJj6WVN46IdM%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%7Ccb6b0d3e0c3c4d3d295c08da284260d3%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866562838020340%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=wMPlN5cYZJfZloAZ48bKLpS3SKSMTyZAovWlSPICXxY%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%7Ccb6b0d3e0c3c4d3d295c08da284260d3%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866562838020340%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=MFKxDa7ZDScho536VVERyLS00uF76BgcbCS1q%2BRQoTI%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%7Ccb6b0d3e0c3c4d3d295c08da284260d3%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866562838020340%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=ef%2BFz7twkOqc9PQC%2FQPkgaqHwreKHpnw4iuPyp%2BdFDQ%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%7Ccb6b0d3e0c3c4d3d295c08da284260d3%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866562838020340%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=VjzfzOLeU%2FRgHhFBfGwTAUXvcRjIuYsY4%2BNV4l3OEb8%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%7Ccb6b0d3e0c3c4d3d295c08da284260d3%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866562838020340%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=HWC5u8pDX3vPRfeoJnXXBUiMlJDNYeApPFNauX2zVbc%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.
Usually, we get netdev_flow_api from netdev->flow_api, but dpif_class- >meter_get/set/del don't have the param of netdev, we must add a helper to get any netdev from dpif. The helper is more like: struct netdev * netdev_get_any(const char *dpif_type) { struct port_to_netdev_data *data; struct netdev *dev = NULL; ovs_rwlock_rdlock(&netdev_hmap_rwlock); HMAP_FOR_EACH (data, portno_node, &port_to_netdev) { if (netdev_get_dpif_type(data->netdev) == dpif_type) { dev = netdev_ref(data->netdev); break; } } ovs_rwlock_unlock(&netdev_hmap_rwlock); return dev; } Do you accept it, or have better solution? > (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? > It makes sense. > > 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=05%7C01%7Cjianbol%40nvidia.com%7Ccb6b0d3e0c3c4d3d295c08da284260d3%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866562838020340%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=dazl4Ccgfrkb7tkne2e6c%2BZTHIjZ2RJqjchon%2BA9l%2BY%3D&reserved=0 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev