On 10/31/23 18:00, Dumitru Ceara wrote:
> 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 <i.maxim...@ovn.org>
> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
> Acked-by: Numan Siddique <num...@ovn.org>
> ---
> V4:
> - Addressed Numan's comment and added his ack:
> - copy the table name argument
> - kept Numan's ack as the other changes since v3 were not
> functionality related.
> - Addressed Ilya's comments:
> - Removed _clear() from ovn_extend_table_reinit().
> - Fixed indentation.
> - Use better name for extend_table number of IDs.
> - Omit name in prototype when possible.
> - Changed "datapath capabilities" comment to "OpenFlow switch
> capabilities".
> 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.
> ---
> controller/ofctrl.c | 2 +
> controller/ovn-controller.c | 6 +--
> include/ovn/features.h | 3 ++
> lib/extend-table.c | 81 ++++++++++++++++++++--------------
> lib/extend-table.h | 13 ++++--
> lib/features.c | 88 ++++++++++++++++++++++++++++++-------
> tests/test-ovn.c | 4 +-
> 7 files changed, 139 insertions(+), 58 deletions(-)
Thanks for v4! I didn't test it, so I'm not going to Ack.
But this version visually looks good to me.
Couple of tiny nitpicks below.
Best regards, Ilya Maximets.
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 63b0aa975b..06f8b33232 100644
> --- 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
> index da7d145ed3..7a8ca38286 100644
> --- 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
> index 7c3004b271..2c47ab766e 100644
> --- 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
> index ebb1a054cb..7f23587784 100644
> --- 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,28 @@ 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 n_ids)
> {
> - 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 = xstrdup(table_name),
> + .n_ids = n_ids,
> + /* Table id 0 is invalid, set id-pool base to 1. */
> + .table_ids = id_pool_create(1, n_ids),
> + .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 n_ids)
> +{
> + if (n_ids != table->n_ids) {
> + id_pool_destroy(table->table_ids);
> + table->table_ids = id_pool_create(1, n_ids);
> + table->n_ids = n_ids;
> + }
> }
>
> static struct ovn_extend_table_info *
> @@ -117,13 +132,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 +175,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 +207,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 +222,8 @@ 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);
> + free(table->name);
> }
>
> /* 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
> index b43a146b4f..90e6e470d0 100644
> --- 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
> + char *name; /* Used to identify this table in a user friendly way,
> + * e.g., for logging. */
> + uint32_t n_ids;
> + 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 n_ids);
> +void ovn_extend_table_reinit(struct ovn_extend_table *, uint32_t n_ids);
>
> void ovn_extend_table_destroy(struct ovn_extend_table *);
>
> diff --git a/lib/features.c b/lib/features.c
> index d24e8f6c5c..914f52d39c 100644
> --- 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)) {
> - /* dump datapath meter capabilities. */
> + features_reply_expected = 0;
> +
> + /* Dump OpenFlow switch meter capabilities. */
> msg = ofpraw_alloc(OFPRAW_OFPST13_METER_FEATURES_REQUEST,
> rconn_get_version(swconn), 0);
> rconn_send(swconn, msg, NULL);
> + features_reply_expected++;
> + /* Dump OpenFlow switch 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,26 @@ 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 +262,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. */
OvS and OVS ?
> + return swconn && rconn_is_connected(swconn) &&
> + features_reply_expected == 0;
Maybe indent this line to the 'swconn' ?
> +}
> +
> +/* 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
> index 1f1e27b51f..aaf2825edc 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, "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;
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
