On Mon 2/6/2023 7:35 PM, Eelco Chaudron wrote: > > On 6 Feb 2023, at 8:48, Tianyu Yuan wrote: > > > On Fri 2/3/2023 8:35 PM, Eelco Chaudron wrote: > >> > >> On 30 Jan 2023, at 17:42, Simon Horman wrote: > >> > >>> From: Tianyu Yuan <[email protected]> > >>> > >>> Allow revalidator to continuously delete police in kernel tc > >>> datapath until it is deleted. > >>> > >>> In current implementation, polices in tc datapath will not deleted > >>> when they are being used and these remaining polices will be cleared > >>> when the openvswitch restarts. > >>> > >>> This patch supports revalidator to delete remaining polices in tc > >>> datapath by storing meter_id deleted unsuccessfully in a hmap and > >>> continuously deleting them in revalidator until success. > >> > >> Hi Tianyu, > >> > >> Thanks for the new revision of the patch... > >> > >> Were you able to figure out if we really need to do this during > >> revalidating flows? > >> > >> I was wondering if there would be a different way, i.e. on flow > >> deletion see if it was the last flow using this meter and if so > >> remove it then. I guess this can be done with the meter having a flag that > it has been deleted. > >> > >> This would be way more elegant than doing this in the revalidator thread. > >> > >> See some inline comments below. This was a quick review/tests, as the > >> above might require a different approach. > >> > >> Cheers, > >> > >> Eelco > >> > > > > Hi Eelco, > > > > Thanks for your review and comments. I've got your point of view: > > 1. Another approach that trigger tc police deletion only when the last > > flow using this meter. > > Not sure if this is possible, but if, it will be more clean and hidden in the > tc > layers. > > > 2. depend the deletion on fail deletion of police_index rather than > > that of meter_id, keep the operation in dpif layer and this should be > invisible from ofproto layer. > > Yes, if the above is not possible, we should keep all this handling in the > offload layer for TC. > > > Please point me out if I misunderstood. > > > > The AT test of system-offload is attached at the end of this mail. > > The error "9: offloads - check_pkt_len action - offloads disabled" > > also happens on Upstream master branch so it is unrelated to this > > patch > > I’m asking for a new test case that shows this problem. i.e. it will fail > without > your patch applied but will pass with it.
Ok, I will consider your suggestions and attach the conresponding AT test script in V4 patch. Thanks, Tianyu > > Thanks, > > Eelco > > > Cheers > > Tianyu > > > >>> Signed-off-by: Tianyu Yuan <[email protected]> > >>> Signed-off-by: Simon Horman <[email protected]> > >>> > >>> --- > >>> lib/dpif-netdev.c | 1 + > >>> lib/dpif-netlink.c | 26 +++++++++++++++++++++++++- > >>> lib/dpif-provider.h | 4 ++++ > >>> lib/dpif.c | 7 +++++++ > >>> lib/dpif.h | 2 ++ > >>> lib/netdev-offload-tc.c | 5 ++++- > >>> lib/netdev-offload.c | 8 +++++--- > >>> ofproto/ofproto-dpif-upcall.c | 6 ++++++ > >>> ofproto/ofproto-dpif.c | 10 +++++++++- > >>> ofproto/ofproto-dpif.h | 3 +++ > >>> 10 files changed, 66 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index > >>> c9f7179c3b4c..878fe5933d1a 100644 > >>> --- a/lib/dpif-netdev.c > >>> +++ b/lib/dpif-netdev.c > >>> @@ -9689,6 +9689,7 @@ const struct dpif_class dpif_netdev_class = { > >>> dpif_netdev_meter_set, > >>> dpif_netdev_meter_get, > >>> dpif_netdev_meter_del, > >>> + NULL, > >>> dpif_netdev_bond_add, > >>> dpif_netdev_bond_del, > >>> dpif_netdev_bond_stats_get, > >>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index > >>> 026b0daa8d83..8b95912aea97 100644 > >>> --- a/lib/dpif-netlink.c > >>> +++ b/lib/dpif-netlink.c > >>> @@ -25,6 +25,7 @@ > >>> #include <net/if.h> > >>> #include <linux/types.h> > >>> #include <linux/pkt_sched.h> > >>> +#include <linux/rtnetlink.h> > >>> #include <poll.h> > >>> #include <stdlib.h> > >>> #include <strings.h> > >>> @@ -37,6 +38,7 @@ > >>> #include "dpif-provider.h" > >>> #include "fat-rwlock.h" > >>> #include "flow.h" > >>> +#include "id-pool.h" > >>> #include "netdev-linux.h" > >>> #include "netdev-offload.h" > >>> #include "netdev-provider.h" > >>> @@ -61,6 +63,7 @@ > >>> #include "packets.h" > >>> #include "random.h" > >>> #include "sset.h" > >>> +#include "tc.h" > >>> #include "timeval.h" > >>> #include "unaligned.h" > >>> #include "util.h" > >>> @@ -4363,12 +4366,32 @@ dpif_netlink_meter_del(struct dpif *dpif, > >> ofproto_meter_id meter_id, > >>> err = dpif_netlink_meter_get_stats(dpif, meter_id, stats, > >>> max_bands, OVS_METER_CMD_DEL); > >>> if (!err && netdev_is_flow_api_enabled()) { > >>> - meter_offload_del(meter_id, stats); > >>> + return meter_offload_del(meter_id, stats); > >>> } > >>> > >>> return err; > >>> } > >>> > >>> +static void > >>> +dpif_netlink_meter_revalidate(struct dpif *dpif_ OVS_UNUSED, > >>> + struct hmap *meter_map) { > >>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > >>> + ofproto_meter_id meter_id; > >>> + struct id_node *id_node; > >>> + > >>> + HMAP_FOR_EACH_POP (id_node, node, meter_map) { > >>> + meter_id.uint32 = id_node->id; > >>> + > >>> + if (meter_offload_del(meter_id, NULL) == EPERM) { > >>> + id_hmap_add(meter_map, meter_id.uint32); > >>> + } else { > >>> + VLOG_DBG_RL(&rl, "Delete meter %u in datapath success", > >>> + meter_id.uint32); > >>> + } > >>> + } > >>> +} > >>> + > >>> static bool > >>> probe_broken_meters__(struct dpif *dpif) { @@ -4588,6 +4611,7 @@ > >>> const struct dpif_class dpif_netlink_class = { > >>> dpif_netlink_meter_set, > >>> dpif_netlink_meter_get, > >>> dpif_netlink_meter_del, > >>> + dpif_netlink_meter_revalidate, > >>> NULL, /* bond_add */ > >>> NULL, /* bond_del */ > >>> NULL, /* bond_stats_get */ > >>> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h index > >>> 12477a24feee..794fbc21686a 100644 > >>> --- a/lib/dpif-provider.h > >>> +++ b/lib/dpif-provider.h > >>> @@ -631,6 +631,10 @@ struct dpif_class { > >>> int (*meter_del)(struct dpif *, ofproto_meter_id meter_id, > >>> struct ofputil_meter_stats *, uint16_t > >>> n_bands); > >>> > >>> + /* Checks unneeded meters from 'dpif' and removes them. They > may > >>> + * be caused by deleting in-use meters. */ > >>> + void (*meter_revalidate)(struct dpif *, struct hmap > >>> + *meter_map); > >>> + > >> > >> I guess this is not really a revalidate action, it's more of a cleanup. > >> So maybe also call it something like meter_cleanup(), or > >> meter_garbage_collection. > >> > >> Or maybe even better, why not use the existing periodic 'bool > >> (*run)(struct dpif *dpif)' callback and handle it there? > >> > >>> /* Adds a bond with 'bond_id' and the member-map to 'dpif'. */ > >>> int (*bond_add)(struct dpif *dpif, uint32_t bond_id, > >>> odp_port_t *member_map); diff --git > >>> a/lib/dpif.c b/lib/dpif.c index fe4db83fbfee..c8718239117d 100644 > >>> --- a/lib/dpif.c > >>> +++ b/lib/dpif.c > >>> @@ -2028,6 +2028,13 @@ dpif_meter_del(struct dpif *dpif, > >> ofproto_meter_id meter_id, > >>> return error; > >>> } > >>> > >>> +void > >>> +dpif_meter_revalidate(struct dpif *dpif, struct hmap *meter_map){ > >>> + if (dpif->dpif_class->meter_revalidate) { > >>> + dpif->dpif_class->meter_revalidate(dpif, meter_map); > >>> + } > >>> +} > >>> + > >>> int > >>> dpif_bond_add(struct dpif *dpif, uint32_t bond_id, odp_port_t > >>> *member_map) { diff --git a/lib/dpif.h b/lib/dpif.h index > >>> 6cb4dae6d8d7..e181e4ee1d6c 100644 > >>> --- a/lib/dpif.h > >>> +++ b/lib/dpif.h > >>> @@ -378,6 +378,7 @@ > >>> > >>> #include "dpdk.h" > >>> #include "dp-packet.h" > >>> +#include "id-pool.h" > >>> #include "netdev.h" > >>> #include "openflow/openflow.h" > >>> #include "openvswitch/ofp-meter.h" > >>> @@ -905,6 +906,7 @@ int dpif_meter_get(const struct dpif *, > >> ofproto_meter_id meter_id, > >>> struct ofputil_meter_stats *, uint16_t n_bands); > >>> int dpif_meter_del(struct dpif *, ofproto_meter_id meter_id, > >>> struct ofputil_meter_stats *, uint16_t n_bands); > >>> +void dpif_meter_revalidate(struct dpif *dpif, struct hmap > >>> +*meter_map); > >>> > >>> /* Bonding. */ > >>> > >>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index > >>> 15d1c36aa04e..f87a5e05e202 100644 > >>> --- a/lib/netdev-offload-tc.c > >>> +++ b/lib/netdev-offload-tc.c > >>> @@ -2980,8 +2980,11 @@ meter_tc_del_policer(ofproto_meter_id > >> meter_id, > >>> police_index, meter_id.uint32, > >>> ovs_strerror(err)); > >>> } else { > >>> meter_free_police_index(police_index); > >>> + /* Do not remove the mapping between meter_id and > police_index > >>> + * until this meter is deleted successfully in datapath. This > >>> + * mapping will be used in meter deletion in revalidator. */ > >>> + meter_id_remove(meter_id.uint32); > >> > >> This approach might have problem, what if the policy gets deleted, > >> and re- added with a new configuration? > >> The datapath flows are not cleared, and the add will fail as the > >> meter_id is still in use. > >> > >> Guess we need to keep the police index as this is tc specific, the > >> meter_id is global. > >> > >>> } > >>> - meter_id_remove(meter_id.uint32); > >>> } > >>> > >>> return err; > >>> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c index > >>> 4592262bd34e..0769f796e5b7 100644 > >>> --- a/lib/netdev-offload.c > >>> +++ b/lib/netdev-offload.c > >>> @@ -241,19 +241,21 @@ int > >>> meter_offload_del(ofproto_meter_id meter_id, struct > >>> ofputil_meter_stats *stats) { > >>> struct netdev_registered_flow_api *rfa; > >>> + int ret = 0; > >>> > >>> CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { > >>> if (rfa->flow_api->meter_del) { > >>> - int ret = rfa->flow_api->meter_del(meter_id, stats); > >>> - if (ret) { > >>> + int err = rfa->flow_api->meter_del(meter_id, stats); > >>> + if (err) { > >>> VLOG_DBG_RL(&rl, "Failed deleting meter %u for flow api > >>> %s, " > >>> "error %d", meter_id.uint32, > >>> rfa->flow_api->type, > >>> ret); > >>> + ret = err; > >>> } > >>> } > >>> } > >>> > >>> - return 0; > >>> + return ret; > >>> } > >>> > >>> int > >>> diff --git a/ofproto/ofproto-dpif-upcall.c > >>> b/ofproto/ofproto-dpif-upcall.c index 31ac02d116fc..b802c9c7f13a > >>> 100644 > >>> --- a/ofproto/ofproto-dpif-upcall.c > >>> +++ b/ofproto/ofproto-dpif-upcall.c > >>> @@ -2828,6 +2828,12 @@ revalidate(struct revalidator *revalidator) > >>> } > >>> ovsrcu_quiesce(); > >>> } > >>> + /* When fail_meter_hmap is not empty, revalidate to delete > >>> + meters in > >> this > >>> + * hashmap. */ > >>> + if (hmap_count(&udpif->backer->fail_meter_hmap)) { > >>> + dpif_meter_revalidate(udpif->dpif, > >>> + &udpif->backer->fail_meter_hmap); > >> > >> This could be removed with the general run() implementation. But in > >> general we should not have this hmap, just call the callback. See below. > >> > >>> + } > >>> + > >>> dpif_flow_dump_thread_destroy(dump_thread); > >>> ofpbuf_uninit(&odp_actions); > >>> } > >>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index > >>> f87e27a8cd7f..591af5503765 100644 > >>> --- a/ofproto/ofproto-dpif.c > >>> +++ b/ofproto/ofproto-dpif.c > >>> @@ -835,6 +835,7 @@ open_dpif_backer(const char *type, struct > >> dpif_backer **backerp) > >>> dpif_meter_get_features(backer->dpif, &features); > >>> if (features.max_meters) { > >>> backer->meter_ids = id_pool_create(0, features.max_meters); > >>> + hmap_init(&backer->fail_meter_hmap); > >>> } else { > >>> backer->meter_ids = NULL; > >>> } > >>> @@ -6790,8 +6791,15 @@ static void > >>> free_meter_id(struct free_meter_id_args *args) { > >>> struct ofproto_dpif *ofproto = args->ofproto; > >>> + int err; > >>> + > >>> + err = dpif_meter_del(ofproto->backer->dpif, args->meter_id, NULL, > 0); > >>> + /* Add fail deleted meter to fail_meter_hmap and waiting to be > >> deleted in > >>> + * revalidator. */ > >>> + if (err == EPERM) { > >>> + id_hmap_add(&ofproto->backer->fail_meter_hmap, args- > >>> meter_id.uint32); > >>> + } > >> > >> I think we should not track any of this in the ofproto layer. As this > >> is a dpif problem, it should be completely hidden in the dpif layer. > >> This should be easy if you remove the dependency on meter_id as > suggested above. > >> > >>> > >>> - dpif_meter_del(ofproto->backer->dpif, args->meter_id, NULL, 0); > >>> id_pool_free_id(ofproto->backer->meter_ids, args- > >meter_id.uint32); > >>> free(args); > >>> } > >>> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h index > >>> d8e0cd37ac5b..befc68d13baa 100644 > >>> --- a/ofproto/ofproto-dpif.h > >>> +++ b/ofproto/ofproto-dpif.h > >>> @@ -61,6 +61,7 @@ struct ofproto_dpif; struct uuid; struct > >>> xlate_cache; struct xlate_ctx; > >>> +struct id_pool; > >>> > >>> /* Number of implemented OpenFlow tables. */ enum { N_TABLES = > 255 > >>> }; @@ -260,6 +261,8 @@ struct dpif_backer { > >>> > >>> /* Meter. */ > >>> struct id_pool *meter_ids; /* Datapath meter allocation. */ > >>> + struct hmap fail_meter_hmap; /* Hashmap for meter with failed > >> deletion > >>> + * in datapath */ > >>> > >>> /* Connection tracking. */ > >>> struct id_pool *tp_ids; /* Datapath timeout policy id > >>> -- > >>> 2.30.2 > >> > >> Please add a test to 'make check-offloads'. > > > > `````` > > ## ------------------------------ ## > > ## openvswitch 3.1.90 test suite. ## > > ## ------------------------------ ## > > > > datapath offloads > > > > 1: offloads - ping between two ports - offloads disabled ok > > 2: offloads - ping between two ports - offloads enabled ok > > 3: offloads - set ingress_policing_rate and ingress_policing_burst - > offloads disabled ok > > 4: offloads - set ingress_policing_rate and ingress_policing_burst - > offloads enabled ok > > 5: offloads - set ingress_policing_kpkts_rate and > ingress_policing_kpkts_burst - offloads disabled ok > > 6: offloads - set ingress_policing_kpkts_rate and > ingress_policing_kpkts_burst - offloads enabled ok > > 7: offloads - check interface meter offloading - offloads disabled ok > > 8: offloads - check interface meter offloading - offloads enabled ok > > 9: offloads - check_pkt_len action - offloads disabled FAILED > > (system-offloads-traffic.at:328) > > 10: offloads - check_pkt_len action - offloads enabled ok > > > > ## ------------- ## > > ## Test results. ## > > ## ------------- ## > > > > ERROR: All 10 tests were run, > > 1 failed unexpectedly. > > ## ------------------------------------------ ## ## > > system-offloads-testsuite.log was created. ## ## > > ------------------------------------------ ## > > > > Please send `tests/system-offloads-testsuite.log' and all information you > think might help: > > > > To: <[email protected]> > > Subject: [openvswitch 3.1.90] system-offloads-testsuite: 9 failed > > > > You may investigate any problem if you feel able to do so, in which > > case the test suite provides a good starting point. Its output may be > > found below `tests/system-offloads-testsuite.dir'. > > > > make: *** [Makefile:7034: check-offloads] Error 1 `````` _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
