On Thu, 2022-04-28 at 14:00 +0200, Ilya Maximets wrote: > On 4/28/22 13:49, Ilya Maximets wrote: > > On 4/28/22 11:06, Jianbo Liu wrote: > > > 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%7Cd62f3e1865b2471053e608da290eb982%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637867440481236224%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=UmOUgdVlvQuuCPEqlfgRlei5tZtmAv7liSbfVVaWXgE%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%7Cd62f3e1865b2471053e608da290eb982%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637867440481236224%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=3CA7zQl8evhu9RIF6BC5pwiLpASiSLN3jJ7%2B4lANKCo%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%7Cd62f3e1865b2471053e608da290eb982%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637867440481236224%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Fv0GEkVRMvT5TQo2wxwB%2FB4FMmUu3d2F5mBRONhen1U%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%7Cd62f3e1865b2471053e608da290eb982%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637867440481236224%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Nm0T35Z4q5RlkAAAXsiwOqbVo7Utm4nVZYfEtYul4gk%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%7Cd62f3e1865b2471053e608da290eb982%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637867440481236224%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=f67ByBBmS7sJC8D4kOcgSOTQSAl5NueDo4NhJL2aozU%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%7Cd62f3e1865b2471053e608da290eb982%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637867440481236224%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=1iLtBH6tsyoOSH8HoPzg3o1zcqEkBQUVNjcs%2BHg3%2Fvc%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%7Cd62f3e1865b2471053e608da290eb982%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637867440481236224%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=i1%2FUfISCkzpZJmSygf1eT2T%2FlMiDFosrcBrIinYJ5x8%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%7Cd62f3e1865b2471053e608da290eb982%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637867440481236224%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vGH1Em4ddDfnHiayWJXabdyqT7TXNua8awyJoPTD4Fk%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? > > > > Hmm. Good point, I missed that. > > In fact, a single dpif can have ports with different offload API > > initialized. For example, linux ports may use TC, while DPDK ports > > can use DPDK's rte_flow while being in the same dpif-netdev > > instance. > > > > So, I think, the correct solution is to add a meter to all flow > > APIs > > that are initialized for ports within that datapath. Maybe > > something > > like this (not tested, pseudo-code): > > > > diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c > > index fb108c0d5..91504ee15 100644 > > --- a/lib/netdev-offload.c > > +++ b/lib/netdev-offload.c > > @@ -79,6 +79,9 @@ struct netdev_registered_flow_api { > > /* Number of references: one for the flow_api itself and one > > for every > > * instance of the netdev that uses it. */ > > struct ovs_refcount refcnt; > > + > > + /* Reference counter for dpif types which has a port with this > > flow API. */ > > + struct simap used_by_dpif_types; > > }; > > > > static struct netdev_registered_flow_api * > > @@ -120,6 +123,7 @@ netdev_register_flow_api_provider(const struct > > netdev_flow_api *new_flow_api) > > hash_string(new_flow_api->type, 0)); > > rfa->flow_api = new_flow_api; > > ovs_refcount_init(&rfa->refcnt); > > + simap_init(&rfa->used_by_dpif_types); > > VLOG_DBG("netdev: flow API '%s' registered.", > > new_flow_api->type); > > } > > ovs_mutex_unlock(&netdev_flow_api_provider_mutex); > > @@ -154,6 +158,7 @@ netdev_unregister_flow_api_provider(const char > > *type) > > } else { > > cmap_remove(&netdev_flow_apis, &rfa->cmap_node, > > hash_string(rfa->flow_api->type, 0)); > > + ovsrcu_postpone(simap_destroy, &rfa->used_by_dpif_types); > > ovsrcu_postpone(free, rfa); > > error = 0; > > } > > @@ -182,6 +187,8 @@ netdev_assign_flow_api(struct netdev *netdev) > > CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { > > if (!rfa->flow_api->init_flow_api(netdev)) { > > ovs_refcount_ref(&rfa->refcnt); > > + simap_increase(&rfa->used_by_dpif_types, > > + netdev_get_dpif_type(netdev), 1); > > ovsrcu_set(&netdev->flow_api, rfa->flow_api); > > VLOG_INFO("%s: Assigned flow API '%s'.", > > netdev_get_name(netdev), rfa->flow_api- > > >type); > > @@ -344,6 +351,8 @@ netdev_uninit_flow_api(struct netdev *netdev) > > > > ovsrcu_set(&netdev->flow_api, NULL); > > rfa = netdev_lookup_flow_api(flow_api->type); > > + ovs_assert(simap_increase(&rfa->used_by_dpif_types, > > + netdev_get_dpif_type(netdev), -1) >= > > 0); > > ovs_refcount_unref(&rfa->refcnt); > > } > > > > @@ -739,6 +748,20 @@ netdev_ports_get_n_flows(const char > > *dpif_type, odp_port_t port_no, > > return ret; > > } > > > > + > > +void > > +netdev_offload_meter_set(const char *dpif_type, ...) > > +{ > > + struct netdev_registered_flow_api *rfa; > > + > > + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { > > + if (simap_get(&rfa->used_by_dpif_types, dpif_type) > 0) { > > + rfa->flow_api->meter_set(...); > > + } > > + } > > +} > > + > > + > > odp_port_t > > netdev_ifindex_to_odp_port(int ifindex) > > { > > --- > > > > ? > > Thinking more, there is no point in counting netdevs, since they > can be added after meters already created. So, I guess, we just > have to always add meters to all registered flow API providers, > i.e. just: > > void > netdev_offload_meter_set(...) > { > struct netdev_registered_flow_api *rfa; > > CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { > rfa->flow_api->meter_set(...); > } > }
You know I will put this netdev_offload_meter_set in dpif_netlink_meter_set. But, in the future, someone may implement meter_set for netdev_offload_dpdk, so it will be called as well. It looks unreasonable as we want to call linux_tc's meter_set only, dpdk_flow_api's meter_set should be called in dpif_netdev. > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
