From be513494ebafffb9729afc319065868cc394df7b Mon Sep 17 00:00:00 2001 From: Dumitru Ceara Date: Tue, 10 Oct 2023 12:50:59 +0200 Subject: [PATCH] controller: Don't artificially limit group and meter IDs to 16bit. OVS actually supports way more. Use the real numbers instead. To avoid preallocating huge bitmaps for the group/meter IDs, switch to id-pool instead (as suggested by Ilya). Reported-at: https://issues.redhat.com/browse/FDP-70 Suggested-by: Ilya Maximets Signed-off-by: Dumitru Ceara --- V2: - Addressed Ilya's comment: fixed the way id_pool_create() is called. - renamed max_size to max_id, it's more accurate. --- controller/ovn-controller.c | 4 ++-- lib/extend-table.c | 28 ++++++++++++---------------- lib/extend-table.h | 7 +++---- tests/test-ovn.c | 4 ++-- 4 files changed, 19 insertions(+), 24 deletions(-) diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index fbb501cab4..6ff4548c9f 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -3949,8 +3949,8 @@ en_lflow_output_init(struct engine_node *node OVS_UNUSED, { struct ed_type_lflow_output *data = xzalloc(sizeof *data); ovn_desired_flow_table_init(&data->flow_table); - ovn_extend_table_init(&data->group_table); - ovn_extend_table_init(&data->meter_table); + ovn_extend_table_init(&data->group_table, OFPG_MAX); + ovn_extend_table_init(&data->meter_table, OFPM13_MAX); objdep_mgr_init(&data->lflow_deps_mgr); lflow_conj_ids_init(&data->conj_ids); uuidset_init(&data->objs_processed); diff --git a/lib/extend-table.c b/lib/extend-table.c index ebb1a054cb..90a1cf92b2 100644 --- a/lib/extend-table.c +++ b/lib/extend-table.c @@ -17,7 +17,6 @@ #include #include -#include "bitmap.h" #include "extend-table.h" #include "hash.h" #include "lib/uuid.h" @@ -30,10 +29,10 @@ ovn_extend_table_delete_desired(struct ovn_extend_table *table, struct ovn_extend_table_lflow_to_desired *l); void -ovn_extend_table_init(struct ovn_extend_table *table) +ovn_extend_table_init(struct ovn_extend_table *table, uint32_t max_id) { - table->table_ids = bitmap_allocate(MAX_EXT_TABLE_ID); - bitmap_set1(table->table_ids, 0); /* table id 0 is invalid. */ + /* Table id 0 is invalid, set id-pool base to 1. */ + table->table_ids = id_pool_create(1, max_id); hmap_init(&table->desired); hmap_init(&table->lflow_to_desired); hmap_init(&table->existing); @@ -192,7 +191,7 @@ ovn_extend_table_clear(struct ovn_extend_table *table, bool existing) g->peer->peer = NULL; } else { /* Unset the bitmap because the peer is deleted already. */ - bitmap_set0(table->table_ids, g->table_id); + id_pool_free_id(table->table_ids, g->table_id); } ovn_extend_table_info_destroy(g); } @@ -206,7 +205,7 @@ ovn_extend_table_destroy(struct ovn_extend_table *table) hmap_destroy(&table->lflow_to_desired); ovn_extend_table_clear(table, true); hmap_destroy(&table->existing); - bitmap_free(table->table_ids); + id_pool_destroy(table->table_ids); } /* Remove an entry from existing table */ @@ -221,7 +220,7 @@ ovn_extend_table_remove_existing(struct ovn_extend_table *table, existing->peer->peer = NULL; } else { /* Dealloc the ID. */ - bitmap_set0(table->table_ids, existing->table_id); + id_pool_free_id(table->table_ids, existing->table_id); } ovn_extend_table_info_destroy(existing); } @@ -242,7 +241,7 @@ ovn_extend_table_delete_desired(struct ovn_extend_table *table, if (e->peer) { e->peer->peer = NULL; } else { - bitmap_set0(table->table_ids, e->table_id); + id_pool_free_id(table->table_ids, e->table_id); } ovn_extend_table_info_destroy(e); } @@ -320,15 +319,12 @@ ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name, if (!existing_info) { /* Reserve a new id. */ - table_id = bitmap_scan(table->table_ids, 0, 1, MAX_EXT_TABLE_ID + 1); - } - - if (table_id == MAX_EXT_TABLE_ID + 1) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); - VLOG_ERR_RL(&rl, "%"PRIu32" out of table ids.", table_id); - return EXT_TABLE_ID_INVALID; + if (!id_pool_alloc_id(table->table_ids, &table_id)) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); + VLOG_ERR_RL(&rl, "%"PRIu32" out of table ids.", table_id); + return EXT_TABLE_ID_INVALID; + } } - bitmap_set1(table->table_ids, table_id); table_info = ovn_extend_table_info_alloc(name, table_id, existing_info, hash); diff --git a/lib/extend-table.h b/lib/extend-table.h index b43a146b4f..56d54afe93 100644 --- a/lib/extend-table.h +++ b/lib/extend-table.h @@ -17,18 +17,17 @@ #ifndef EXTEND_TABLE_H #define EXTEND_TABLE_H 1 -#define MAX_EXT_TABLE_ID 65535 #define EXT_TABLE_ID_INVALID 0 #include "openvswitch/hmap.h" #include "openvswitch/list.h" #include "openvswitch/uuid.h" +#include "id-pool.h" /* Used to manage expansion tables associated with Flow table, * such as the Group Table or Meter Table. */ struct ovn_extend_table { - unsigned long *table_ids; /* Used as a bitmap with value set - * for allocated ids in either desired or + struct id_pool *table_ids; /* Used to allocated ids in either desired or * existing (or both). If the same "name" * exists in both desired and existing tables, * they must share the same ID. The "peer" @@ -81,7 +80,7 @@ struct ovn_extend_table_lflow_ref { struct ovn_extend_table_info *desired; }; -void ovn_extend_table_init(struct ovn_extend_table *); +void ovn_extend_table_init(struct ovn_extend_table *, uint32_t max_id); void ovn_extend_table_destroy(struct ovn_extend_table *); diff --git a/tests/test-ovn.c b/tests/test-ovn.c index 1f1e27b51f..e4f03806a3 100644 --- a/tests/test-ovn.c +++ b/tests/test-ovn.c @@ -1300,11 +1300,11 @@ test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED) /* Initialize group ids. */ struct ovn_extend_table group_table; - ovn_extend_table_init(&group_table); + ovn_extend_table_init(&group_table, OFPG_MAX); /* Initialize meter ids for QoS. */ struct ovn_extend_table meter_table; - ovn_extend_table_init(&meter_table); + ovn_extend_table_init(&meter_table, OFPM13_MAX); /* Initialize collector sets. */ struct flow_collector_ids collector_ids;