controller: Don't artificially limit group and meter IDs to 16bit.

OVS actually supports way more. Detect the real number of groups and meters instead. To avoid preallocating huge bitmaps for the IDs, switch to id-pool instead (as suggested by Ilya). Reported-at: https://issues.redhat.com/browse/FDP-70 Suggested-by: Ilya Maximets <[email protected]> Signed-off-by: Dumitru Ceara <[email protected]> --- V3: - Addressed Ilya's comments: - removed references to 'bitmap' in comments - fixed indentation & typos - moved "id-pool.h" include to C file - extended the OVS feature detection component to also check for the max number of groups. V2: - Addressed Ilya's comment: fixed the way id_pool_create() is called. - renamed max_size to max_id, it's more accurate.
dceara · Oct 26, 2023 · b083cca · b083cca
1 parent 4db5ab2
commit b083cca
Show file tree

Hide file tree

Showing 7 changed files with 140 additions and 57 deletions.
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
@@ -677,11 +677,13 @@ run_S_CLEAR_FLOWS(void)
     /* Clear existing groups, to match the state of the switch. */
     if (groups) {
         ovn_extend_table_clear(groups, true);
+        ovn_extend_table_reinit(groups, ovs_feature_max_select_groups_get());
     }
 
     /* Clear existing meters, to match the state of the switch. */
     if (meters) {
         ovn_extend_table_clear(meters, true);
+        ovn_extend_table_reinit(meters, ovs_feature_max_meters_get());
         ofctrl_meter_bands_clear();
     }
 

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
@@ -3948,8 +3948,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, "group-table", 0);
+    ovn_extend_table_init(&data->meter_table, "meter-table", 0);
     objdep_mgr_init(&data->lflow_deps_mgr);
     lflow_conj_ids_init(&data->conj_ids);
     uuidset_init(&data->objs_processed);
@@ -5656,7 +5656,7 @@ main(int argc, char *argv[])
                 engine_set_force_recompute(true);
             }
 
-            if (br_int) {
+            if (br_int && ovs_feature_set_discovered()) {
                 ct_zones_data = engine_get_data(&en_ct_zones);
                 if (ct_zones_data && ofctrl_run(br_int, ovs_table,
                                                 &ct_zones_data->pending)) {

diff --git a/include/ovn/features.h b/include/ovn/features.h
@@ -49,5 +49,8 @@ void ovs_feature_support_destroy(void);
 bool ovs_feature_is_supported(enum ovs_feature_value feature);
 bool ovs_feature_support_run(const struct smap *ovs_capabilities,
                              const char *br_name);
+bool ovs_feature_set_discovered(void);
+uint32_t ovs_feature_max_meters_get(void);
+uint32_t ovs_feature_max_select_groups_get(void);
 
 #endif
diff --git a/lib/extend-table.c b/lib/extend-table.c
@@ -17,9 +17,9 @@
 #include <config.h>
 #include <string.h>
 
-#include "bitmap.h"
 #include "extend-table.h"
 #include "hash.h"
+#include "id-pool.h"
 #include "lib/uuid.h"
 #include "openvswitch/vlog.h"
 
@@ -30,13 +30,29 @@ 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, const char *table_name,
+                      uint32_t max_id)
 {
-    table->table_ids = bitmap_allocate(MAX_EXT_TABLE_ID);
-    bitmap_set1(table->table_ids, 0); /* table id 0 is invalid. */
-    hmap_init(&table->desired);
-    hmap_init(&table->lflow_to_desired);
-    hmap_init(&table->existing);
+    *table = (struct ovn_extend_table) {
+        .name = table_name,
+        .max_id = max_id,
+        /* Table id 0 is invalid, set id-pool base to 1. */
+        .table_ids = id_pool_create(1, max_id),
+        .desired = HMAP_INITIALIZER(&table->desired),
+        .lflow_to_desired = HMAP_INITIALIZER(&table->lflow_to_desired),
+        .existing = HMAP_INITIALIZER(&table->existing),
+    };
+}
+
+void
+ovn_extend_table_reinit(struct ovn_extend_table *table, uint32_t max_id)
+{
+    ovn_extend_table_clear(table, true);
+    if (max_id != table->max_id) {
+        table->max_id = max_id;
+        id_pool_destroy(table->table_ids);
+        table->table_ids = id_pool_create(1, max_id);
+    }
 }
 
 static struct ovn_extend_table_info *
@@ -117,13 +133,13 @@ ovn_extend_table_add_desired_to_lflow(struct ovn_extend_table *table,
         ovs_list_init(&l->desired);
         hmap_insert(&table->lflow_to_desired, &l->hmap_node,
                     uuid_hash(lflow_uuid));
-        VLOG_DBG("%s: add new lflow_to_desired entry "UUID_FMT,
-                 __func__, UUID_ARGS(lflow_uuid));
+        VLOG_DBG("%s: table %s: add new lflow_to_desired entry "UUID_FMT,
+                 __func__, table->name, UUID_ARGS(lflow_uuid));
     }
 
     ovs_list_insert(&l->desired, &r->list_node);
-    VLOG_DBG("%s: lflow "UUID_FMT" use new item %s, id %"PRIu32,
-             __func__, UUID_ARGS(lflow_uuid), r->desired->name,
+    VLOG_DBG("%s: table %s: lflow "UUID_FMT" use new item %s, id %"PRIu32,
+             __func__, table->name, UUID_ARGS(lflow_uuid), r->desired->name,
              r->desired->table_id);
 }
 
@@ -160,10 +176,11 @@ ovn_extend_info_add_lflow_ref(struct ovn_extend_table *table,
 }
 
 static void
-ovn_extend_info_del_lflow_ref(struct ovn_extend_table_lflow_ref *r)
+ovn_extend_info_del_lflow_ref(struct ovn_extend_table *table,
+                              struct ovn_extend_table_lflow_ref *r)
 {
-    VLOG_DBG("%s: name %s, lflow "UUID_FMT" n %"PRIuSIZE, __func__,
-             r->desired->name, UUID_ARGS(&r->lflow_uuid),
+    VLOG_DBG("%s: table %s: name %s, lflow "UUID_FMT" n %"PRIuSIZE, __func__,
+             table->name, r->desired->name, UUID_ARGS(&r->lflow_uuid),
              hmap_count(&r->desired->references));
     hmap_remove(&r->desired->references, &r->hmap_node);
     ovs_list_remove(&r->list_node);
@@ -191,8 +208,8 @@ ovn_extend_table_clear(struct ovn_extend_table *table, bool existing)
         if (g->peer) {
             g->peer->peer = NULL;
         } else {
-            /* Unset the bitmap because the peer is deleted already. */
-            bitmap_set0(table->table_ids, g->table_id);
+            /* Unset the id because the peer is deleted already. */
+            id_pool_free_id(table->table_ids, g->table_id);
         }
         ovn_extend_table_info_destroy(g);
     }
@@ -206,7 +223,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 +238,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);
 }
@@ -234,15 +251,15 @@ ovn_extend_table_delete_desired(struct ovn_extend_table *table,
     struct ovn_extend_table_lflow_ref *r;
     LIST_FOR_EACH_SAFE (r, list_node, &l->desired) {
         struct ovn_extend_table_info *e = r->desired;
-        ovn_extend_info_del_lflow_ref(r);
+        ovn_extend_info_del_lflow_ref(table, r);
         if (hmap_is_empty(&e->references)) {
-            VLOG_DBG("%s: %s, "UUID_FMT, __func__,
-                     e->name, UUID_ARGS(&l->lflow_uuid));
+            VLOG_DBG("%s: table %s: %s, "UUID_FMT, __func__,
+                     table->name, e->name, UUID_ARGS(&l->lflow_uuid));
             hmap_remove(&table->desired, &e->hmap_node);
             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);
         }
@@ -284,7 +301,7 @@ ovn_extend_table_sync(struct ovn_extend_table *table)
     }
 }
 
-/* Assign a new table ID for the table information from the bitmap.
+/* Assign a new table ID for the table information from the ID pool.
  * If it already exists, return the old ID. */
 uint32_t
 ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name,
@@ -298,9 +315,9 @@ ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name,
     /* Check whether we have non installed but allocated group_id. */
     HMAP_FOR_EACH_WITH_HASH (table_info, hmap_node, hash, &table->desired) {
         if (!strcmp(table_info->name, name)) {
-            VLOG_DBG("ovn_externd_table_assign_id: reuse old id %"PRIu32
-                     " for %s, used by lflow "UUID_FMT,
-                     table_info->table_id, table_info->name,
+            VLOG_DBG("ovn_extend_table_assign_id: table %s: "
+                     "reuse old id %"PRIu32" for %s, used by lflow "UUID_FMT,
+                     table->name, table_info->table_id, table_info->name,
                      UUID_ARGS(&lflow_uuid));
             ovn_extend_info_add_lflow_ref(table, table_info, &lflow_uuid);
             return table_info->table_id;
@@ -320,15 +337,13 @@ 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 (!id_pool_alloc_id(table->table_ids, &table_id)) {
+            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 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;
+            VLOG_ERR_RL(&rl, "table %s: out of table ids.", table->name);
+            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
@@ -17,18 +17,21 @@
 #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"
 
+struct id_pool;
+
 /* 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
+    const char *name; /* Used to identify this table in a user friendly way,
+                       * e.g., for logging. */
+    uint32_t max_id; /* Max assignable ID. */
+    struct id_pool *table_ids; /* Used to allocate 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 +84,9 @@ 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 *, const char *table_name,
+                           uint32_t max_id);
+void ovn_extend_table_reinit(struct ovn_extend_table *table, uint32_t max_id);
 
 void ovn_extend_table_destroy(struct ovn_extend_table *);
 

diff --git a/lib/features.c b/lib/features.c
@@ -27,6 +27,7 @@
 #include "openvswitch/rconn.h"
 #include "openvswitch/ofp-msgs.h"
 #include "openvswitch/ofp-meter.h"
+#include "openvswitch/ofp-group.h"
 #include "openvswitch/ofp-util.h"
 #include "ovn/features.h"
 
@@ -81,6 +82,18 @@ static struct ovs_feature all_ovs_features[] = {
 /* A bitmap of OVS features that have been detected as 'supported'. */
 static uint32_t supported_ovs_features;
 
+/* Last set of received feature replies. */
+static struct ofputil_meter_features ovs_meter_features_reply;
+static struct ofputil_group_features ovs_group_features_reply;
+
+/* Currently discovered set of features. */
+static struct ofputil_meter_features ovs_meter_features;
+static struct ofputil_group_features ovs_group_features;
+
+/* Number of features replies still expected to receive for the requests
+ * we sent already. */
+static uint32_t features_reply_expected;
+
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
 
 /* ovs-vswitchd connection. */
@@ -145,11 +158,19 @@ ovs_feature_get_openflow_cap(const char *br_name)
 
     /* send new requests just after reconnect. */
     if (conn_seq_no != rconn_get_connection_seqno(swconn)) {
+        features_reply_expected = 0;
+
         /* dump datapath meter capabilities. */
         msg = ofpraw_alloc(OFPRAW_OFPST13_METER_FEATURES_REQUEST,
                            rconn_get_version(swconn), 0);
         rconn_send(swconn, msg, NULL);
+        features_reply_expected++;
+        /* dump datapath group capabilities. */
+        msg = ofputil_encode_group_features_request(rconn_get_version(swconn));
+        rconn_send(swconn, msg, NULL);
+        features_reply_expected++;
     }
+    conn_seq_no = rconn_get_connection_seqno(swconn);
 
     bool ret = false;
     for (int i = 0; i < 50; i++) {
@@ -163,21 +184,13 @@ ovs_feature_get_openflow_cap(const char *br_name)
         ofptype_decode(&type, oh);
 
         if (type == OFPTYPE_METER_FEATURES_STATS_REPLY) {
-            struct ofputil_meter_features mf;
-            ofputil_decode_meter_features(oh, &mf);
-
-            bool old_state = supported_ovs_features & OVS_DP_METER_SUPPORT;
-            bool new_state = mf.max_meters > 0;
-
-            if (old_state != new_state) {
-                ret = true;
-                if (new_state) {
-                    supported_ovs_features |= OVS_DP_METER_SUPPORT;
-                } else {
-                    supported_ovs_features &= ~OVS_DP_METER_SUPPORT;
-                }
-            }
-            conn_seq_no = rconn_get_connection_seqno(swconn);
+            ofputil_decode_meter_features(oh, &ovs_meter_features_reply);
+            ovs_assert(features_reply_expected);
+            features_reply_expected--;
+        } else if (type == OFPTYPE_GROUP_FEATURES_STATS_REPLY) {
+            ofputil_decode_group_features_reply(oh, &ovs_group_features_reply);
+            ovs_assert(features_reply_expected);
+            features_reply_expected--;
         } else if (type == OFPTYPE_ECHO_REQUEST) {
             rconn_send(swconn, ofputil_encode_echo_reply(oh), NULL);
         }
@@ -186,6 +199,28 @@ ovs_feature_get_openflow_cap(const char *br_name)
     rconn_run_wait(swconn);
     rconn_recv_wait(swconn);
 
+    /* If all feature replies were received, update the set of supported
+     * features. */
+    if (!features_reply_expected) {
+        if (memcmp(&ovs_meter_features,
+                   &ovs_meter_features_reply,
+                   sizeof ovs_meter_features_reply)) {
+            ovs_meter_features = ovs_meter_features_reply;
+            if (ovs_meter_features.max_meters) {
+                supported_ovs_features |= OVS_DP_METER_SUPPORT;
+            } else {
+                supported_ovs_features &= ~OVS_DP_METER_SUPPORT;
+            }
+            ret = true;
+        }
+        if (memcmp(&ovs_group_features,
+                    &ovs_group_features_reply,
+                    sizeof ovs_group_features_reply)) {
+            ovs_group_features = ovs_group_features_reply;
+            ret = true;
+        }
+    }
+
     return ret;
 }
 
@@ -229,3 +264,26 @@ ovs_feature_support_run(const struct smap *ovs_capabilities,
     }
     return updated;
 }
+
+bool
+ovs_feature_set_discovered(void)
+{
+    /* The supported feature set has been discovered if we're connected
+     * to OvS and OVS replied to all our feature request messages. */
+    return swconn && rconn_is_connected(swconn) &&
+        features_reply_expected == 0;
+}
+
+/* Returns the number of meters the OVS datapath supports. */
+uint32_t
+ovs_feature_max_meters_get(void)
+{
+    return ovs_meter_features.max_meters;
+}
+
+/* Returns the number of select groups the OVS datapath supports. */
+uint32_t
+ovs_feature_max_select_groups_get(void)
+{
+    return ovs_group_features.max_groups[OFPGT11_SELECT];
+}
diff --git 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, "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, "meter-table", OFPM13_MAX);
 
     /* Initialize collector sets. */
     struct flow_collector_ids collector_ids;