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. 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. 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 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
