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
> 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'.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
