On 11/20/23 13:03, Ales Musil wrote:
> On Tue, Oct 31, 2023 at 6:01 PM Dumitru Ceara <[email protected]> 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 <[email protected]>
>> Signed-off-by: Dumitru Ceara <[email protected]>
>> Acked-by: Numan Siddique <[email protected]>
>> ---
>>
>
> Hi Dumitru,
> I have two small comments below that could be addressed during merge.
>
Thanks for your review!
>
>> 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(-)
>>
>> 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) {
>>
>
> nit: That's the only place where we use the n_ids, could we use the pool's
> n_ids instead? It seems redundant to store twice the same thing.
>
Unfortunately struct id_pool is opaque, that's why I had to use a new
n_ids field.
>
>> + 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;
>>
>
> nit: n_features_reply_expected? The current name implies more bool and not
> uint IMO.
>
You're right, it looks better, I changed it to n_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. */
>> + 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
>> 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;
>> --
>> 2.39.3
>>
>> _______________________________________________
>> dev mailing list
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
> Other than that it looks good.
>
> Acked-by: Ales Musil <[email protected]>
>
Thanks, applied to main and backported to all stable branches down to 22.03.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
