Hi Lorenzo,
I think this and patch 3 should be combined. See notes below for why.
On 1/14/22 13:01, Lorenzo Bianconi wrote:
Introduce pending_id map used to store meter_id allocated but not yet
inserted in the desired_table. This is a preliminary patch to add
metering IP engine.
Signed-off-by: Lorenzo Bianconi <[email protected]>
---
controller/ofctrl.c | 9 +++++
controller/ofctrl.h | 1 +
lib/extend-table.c | 82 +++++++++++++++++++++++++++++++++------------
lib/extend-table.h | 7 ++++
4 files changed, 78 insertions(+), 21 deletions(-)
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 08fcfed8b..bf715787e 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -1801,6 +1801,15 @@ add_meter_string(struct ovn_extend_table_info *m_desired,
free(meter_string);
}
+uint32_t ofctrl_get_meter_id(const char *name, bool new_id)
ofctrl_get_meter_id() is not used anywhere in this patch, so trying to
analyze its purpose on its own is difficult (e.g. what are the
circumstances for setting new_id to true or false?). To properly review
this, I had to look at patch 3 as well.
+{
+ uint32_t id;
+ bool val;
+
+ ovn_extend_table_get_id(meters, name, &id, NULL, &val, new_id);
+ return id;
+}
+
static void
add_meter(struct ovn_extend_table_info *m_desired,
const struct sbrec_meter_table *meter_table,
diff --git a/controller/ofctrl.h b/controller/ofctrl.h
index 014de210d..0efcb2ef5 100644
--- a/controller/ofctrl.h
+++ b/controller/ofctrl.h
@@ -129,5 +129,6 @@ void ofctrl_check_and_add_flow_metered(struct
ovn_desired_flow_table *,
bool ofctrl_is_connected(void);
void ofctrl_set_probe_interval(int probe_interval);
void ofctrl_get_memory_usage(struct simap *usage);
+uint32_t ofctrl_get_meter_id(const char *name, bool new_id);
#endif /* controller/ofctrl.h */
diff --git a/lib/extend-table.c b/lib/extend-table.c
index c708e24b9..52c8cd62e 100644
--- a/lib/extend-table.c
+++ b/lib/extend-table.c
@@ -37,6 +37,7 @@ ovn_extend_table_init(struct ovn_extend_table *table)
hmap_init(&table->desired);
hmap_init(&table->lflow_to_desired);
hmap_init(&table->existing);
+ shash_init(&table->pending_ids);
}
static struct ovn_extend_table_info *
@@ -191,6 +192,14 @@ ovn_extend_table_clear(struct ovn_extend_table *table,
bool existing)
}
ovn_extend_table_info_destroy(g);
}
+
+ struct shash_node *node, *tmp;
+ SHASH_FOR_EACH_SAFE (node, tmp, &table->pending_ids) {
+ uint32_t *id = node->data;
+ shash_delete(&table->pending_ids, node);
+ free(id);
+ }
+ shash_destroy(&table->pending_ids);
You can replace all of these lines with
shash_destroy_free_data(&table->pending_ids)
}
void
@@ -282,54 +291,85 @@ ovn_extend_table_sync(struct ovn_extend_table *table)
}
}
-/* Assign a new table ID for the table information from the bitmap.
- * If it already exists, return the old ID. */
-uint32_t
-ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name,
- struct uuid lflow_uuid)
+bool
+ovn_extend_table_get_id(struct ovn_extend_table *table, const char *name,
+ uint32_t *meter_id, struct uuid *lflow_uuid,
+ bool *new_id, bool pending)
The return value of this function needs to be documented. It's not clear
at all what a true or false return means. I *think* that a true return
means that a new ID was allocated for
I think the meter_id parameter should have its name changed. Extend
tables are used for groups, meters, and other purposes. "id" is a better
choice.
{
- uint32_t table_id = 0, hash;
struct ovn_extend_table_info *table_info;
-
- hash = hash_string(name, 0);
+ uint32_t hash = hash_string(name, 0);
/* 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
Nit: This is in a different function now, so it should be
"ovn_extend_table_get_id" in this debug message.
- " for %s, used by lflow "UUID_FMT,
- 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;
+ " for %s", table_info->table_id, table_info->name);
+ if (lflow_uuid) {
+ ovn_extend_info_add_lflow_ref(table, table_info, lflow_uuid);
+ }
+ *meter_id = table_info->table_id;
+ return false;
}
}
+ *new_id = false;
/* Check whether we already have an installed entry for this
* combination. */
HMAP_FOR_EACH_WITH_HASH (table_info, hmap_node, hash, &table->existing) {
if (!strcmp(table_info->name, name)) {
- table_id = table_info->table_id;
+ *meter_id = table_info->table_id;
+ return true;
}
}
- bool new_table_id = false;
- if (!table_id) {
- /* Reserve a new group_id. */
- table_id = bitmap_scan(table->table_ids, 0, 1, MAX_EXT_TABLE_ID + 1);
- new_table_id = true;
+ /* Check if a ids has been already allocated for this flow. */
+ uint32_t *id = shash_find_and_delete(&table->pending_ids, name);
It took me quite a while to understand what the pending_ids were being
used for. I *think* the idea is that the incremental engine is
generating IDs but storing them in table->pending_ids instead of
creating extend_table_info structures. Then later, when
ovn_extend_table_assign_id() is called by the flow_engine, we can pull
the ID from here instead of using another bit in table->table_ids. This
way, the table info is generated by lflow processing like it always has,
and the lflow_uuid can be assigned at that point.
The problem is, this is an unorthodox use of the incremental engine, and
it's the reason why it took me so long to understand. The IDs created by
the meter engine node are directly required by the lflow processing
engine node. So presumably, the meter engine node should be an input to
the lflow processing engine node. This way, the lflow change handler can
be called in the case the meter information has changed. As it stands,
the two separate engines are using a third-party structure for their
data, and it just happens to work out because the meter_engine is run
before the flow_engine.
I also think this was harder to analyze since it required me to look at
patch 3 to understand the concept fully. This is another reason I think
patch 2 and patch 3 should be combined.
+ if (id) {
+ *meter_id = *id;
+ free(id);
+ return true;
}
+ /* Reserve a new group_id. */
+ uint32_t 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;
+ *meter_id = EXT_TABLE_ID_INVALID;
+ return false;
}
bitmap_set1(table->table_ids, table_id);
+ *meter_id = table_id;
+
+ *new_id = true;
+ if (pending) {
+ /* Add the id to pedning map. */
+ id = xmalloc(sizeof *id);
+ *id = table_id;
+ shash_add(&table->pending_ids, name, id);
+ }
+
+ return true;
+}
+
+/* Assign a new table ID for the table information from the bitmap.
+ * If it already exists, return the old ID. */
+uint32_t
+ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name,
+ struct uuid lflow_uuid)
+{
+ uint32_t table_id, hash = hash_string(name, 0);
+ struct ovn_extend_table_info *table_info;
+ bool new_table_id;
+
+ if (!ovn_extend_table_get_id(table, name, &table_id,
+ &lflow_uuid, &new_table_id, false)) {
+ return table_id;
+ }
table_info = ovn_extend_table_info_alloc(name, table_id, new_table_id,
hash);
-
hmap_insert(&table->desired,
&table_info->hmap_node, table_info->hmap_node.hash);
diff --git a/lib/extend-table.h b/lib/extend-table.h
index 4d80cfd80..5c5f65695 100644
--- a/lib/extend-table.h
+++ b/lib/extend-table.h
@@ -21,6 +21,7 @@
#define EXT_TABLE_ID_INVALID 0
#include "openvswitch/hmap.h"
+#include "openvswitch/shash.h"
#include "openvswitch/list.h"
#include "openvswitch/uuid.h"
@@ -30,6 +31,8 @@ struct ovn_extend_table {
unsigned long *table_ids; /* Used as a bitmap with value set
* for allocated group ids in either
* desired or existing. */
+ struct shash pending_ids;
+
struct hmap desired;
struct hmap lflow_to_desired; /* Index for looking up desired table
* items from given lflow uuid, with
@@ -93,6 +96,10 @@ void ovn_extend_table_sync(struct ovn_extend_table *);
uint32_t ovn_extend_table_assign_id(struct ovn_extend_table *,
const char *name,
struct uuid lflow_uuid);
+bool
+ovn_extend_table_get_id(struct ovn_extend_table *table, const char *name,
+ uint32_t *meter_id, struct uuid *lflow_uuid,
+ bool *new_id, bool pending);
/* Iterates 'DESIRED' through all of the 'ovn_extend_table_info's in
* 'TABLE'->desired that are not in 'TABLE'->existing. (The loop body
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
