> 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.
ack, I am fine with it.
>
> > +{
> > + 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)
ack, I will fix it
>
> > }
> > 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.
ack, I will fix it
>
> > {
> > - 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.
ack, correct.
>
> 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 think for meter-band table is not 1:1 with all other SB tables since changing
a meter band will not change the related lflow since in logical_flow table we
just reference the UUID not the band value.
I guess we can try to hook it into the lflow engine, but I guess we need to
understand how to manage the full-recompute and how to send configuration
changes to OVS.
>
> 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.
ack, fine with it.
Regards,
Lorenzo
>
> > + 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
