On Fri, Dec 6, 2019 at 11:29 PM Han Zhou <[email protected]> wrote: > > A meter/group can be used by multiple logical flows. However, current > code didn't handle this properly. Each table_info item has a lflow_uuid > field, which can keep track of only a single lflow. > > In most cases this doesn't create problems because multiple table_info > entries are created even for same "name". > > However, when multiple lflows are added in the same main loop iteration > using the same "name" (i.e. when the new_table_id == true), the function > ovn_extend_table_assign_id() will return the old id without creating a > new entry, and the reference by the second lflow is untracked. Later > with incremental processing, if the old lflow is deleted, the table_info > will be deleted, which results in the deletion of group/meter in OVS, > even when it is still used by the second lflow. > > This patch fixes the problem by adding a hmap in each desired table_info > item to keep track of multiple lflow references. A test case is added. > The test case would fail without this fix. > > At the same time, this patch adds an index that maps from lflow_uuid to > a list of desired table_info items used by the lflow, so that the > ovn_extend_table_remove_desired() is more efficient, without having > to do a O(N) iteration every time. > > Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine - > quiet mode.") > Signed-off-by: Han Zhou <[email protected]>
Nice catch! Looks good to me. Acked-by: Dumitru Ceara <[email protected]> > --- > lib/extend-table.c | 180 > ++++++++++++++++++++++++++++++++++++++++++++++------- > lib/extend-table.h | 31 ++++++++- > tests/ovn.at | 55 ++++++++++++++++ > 3 files changed, 241 insertions(+), 25 deletions(-) > > diff --git a/lib/extend-table.c b/lib/extend-table.c > index 82dfcfa..b102e44 100644 > --- a/lib/extend-table.c > +++ b/lib/extend-table.c > @@ -31,9 +31,37 @@ ovn_extend_table_init(struct ovn_extend_table *table) > 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); > } > > +static struct ovn_extend_table_info * > +ovn_extend_table_info_alloc(const char *name, uint32_t id, bool is_new_id, > + uint32_t hash) > +{ > + struct ovn_extend_table_info *e = xmalloc(sizeof *e); > + e->name = xstrdup(name); > + e->table_id = id; > + e->new_table_id = is_new_id; > + e->hmap_node.hash = hash; > + hmap_init(&e->references); > + return e; > +} > + > +static void > +ovn_extend_table_info_destroy(struct ovn_extend_table_info *e) > +{ > + free(e->name); > + struct ovn_extend_table_lflow_ref *r, *r_next; > + HMAP_FOR_EACH_SAFE (r, r_next, hmap_node, &e->references) { > + hmap_remove(&e->references, &r->hmap_node); > + ovs_list_remove(&r->list_node); > + free(r); > + } > + hmap_destroy(&e->references); > + free(e); > +} > + > /* Finds and returns a group_info in 'existing' whose key is identical > * to 'target''s key, or NULL if there is none. */ > struct ovn_extend_table_info * > @@ -51,6 +79,89 @@ ovn_extend_table_lookup(struct hmap *exisiting, > return NULL; > } > > +static struct ovn_extend_table_lflow_to_desired * > +ovn_extend_table_find_desired_by_lflow(struct ovn_extend_table *table, > + const struct uuid *lflow_uuid) > +{ > + struct ovn_extend_table_lflow_to_desired *l; > + HMAP_FOR_EACH_WITH_HASH (l, hmap_node, uuid_hash(lflow_uuid), > + &table->lflow_to_desired) { > + if (uuid_equals(&l->lflow_uuid, lflow_uuid)) { > + return l; > + } > + } > + return NULL; > +} > + > +/* Add a reference to the list of items that <lflow_uuid> uses. > + * If the <lflow_uuid> entry doesn't exist in lflow_to_desired mapping, add > + * the <lflow_uuid> entry first. */ > +static void > +ovn_extend_table_add_desired_to_lflow(struct ovn_extend_table *table, > + const struct uuid *lflow_uuid, > + struct ovn_extend_table_lflow_ref *r) > +{ > + struct ovn_extend_table_lflow_to_desired *l = > + ovn_extend_table_find_desired_by_lflow(table, lflow_uuid); > + if (!l) { > + l = xmalloc(sizeof *l); > + l->lflow_uuid = *lflow_uuid; > + 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)); > + } > + > + 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, > + r->desired->table_id); > +} > + > +static struct ovn_extend_table_lflow_ref * > +ovn_extend_info_find_lflow_ref(struct ovn_extend_table_info *e, > + const struct uuid *lflow_uuid) > +{ > + struct ovn_extend_table_lflow_ref *r; > + HMAP_FOR_EACH_WITH_HASH (r, hmap_node, uuid_hash(lflow_uuid), > + &e->references) { > + if (uuid_equals(&r->lflow_uuid, lflow_uuid)) { > + return r; > + } > + } > + return NULL; > +} > + > +/* Create the cross reference between <e> and <lflow_uuid> */ > +static void > +ovn_extend_info_add_lflow_ref(struct ovn_extend_table *table, > + struct ovn_extend_table_info *e, > + const struct uuid *lflow_uuid) > +{ > + struct ovn_extend_table_lflow_ref *r = > + ovn_extend_info_find_lflow_ref(e, lflow_uuid); > + if (!r) { > + r = xmalloc(sizeof *r); > + r->lflow_uuid = *lflow_uuid; > + r->desired = e; > + hmap_insert(&e->references, &r->hmap_node, uuid_hash(lflow_uuid)); > + > + ovn_extend_table_add_desired_to_lflow(table, lflow_uuid, r); > + } > +} > + > +static void > +ovn_extend_info_del_lflow_ref(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), > + hmap_count(&r->desired->references)); > + hmap_remove(&r->desired->references, &r->hmap_node); > + ovs_list_remove(&r->list_node); > + free(r); > +} > + > /* Clear either desired or existing in ovn_extend_table. */ > void > ovn_extend_table_clear(struct ovn_extend_table *table, bool existing) > @@ -58,6 +169,16 @@ ovn_extend_table_clear(struct ovn_extend_table *table, > bool existing) > struct ovn_extend_table_info *g, *next; > struct hmap *target = existing ? &table->existing : &table->desired; > > + /* Clear lflow_to_desired index, if the target is desired table. */ > + if (!existing) { > + struct ovn_extend_table_lflow_to_desired *l, *l_next; > + HMAP_FOR_EACH_SAFE (l, l_next, hmap_node, &table->lflow_to_desired) { > + hmap_remove(&table->lflow_to_desired, &l->hmap_node); > + free(l); > + } > + } > + > + /* Clear the target table. */ > HMAP_FOR_EACH_SAFE (g, next, hmap_node, target) { > hmap_remove(target, &g->hmap_node); > /* Don't unset bitmap for desired group_info if the group_id > @@ -65,8 +186,7 @@ ovn_extend_table_clear(struct ovn_extend_table *table, > bool existing) > if (existing || g->new_table_id) { > bitmap_set0(table->table_ids, g->table_id); > } > - free(g->name); > - free(g); > + ovn_extend_table_info_destroy(g); > } > } > > @@ -75,6 +195,7 @@ ovn_extend_table_destroy(struct ovn_extend_table *table) > { > ovn_extend_table_clear(table, false); > hmap_destroy(&table->desired); > + hmap_destroy(&table->lflow_to_desired); > ovn_extend_table_clear(table, true); > hmap_destroy(&table->existing); > bitmap_free(table->table_ids); > @@ -87,11 +208,10 @@ ovn_extend_table_remove_existing(struct ovn_extend_table > *table, > { > /* Remove 'existing' from 'groups->existing' */ > hmap_remove(&table->existing, &existing->hmap_node); > - free(existing->name); > > /* Dealloc group_id. */ > bitmap_set0(table->table_ids, existing->table_id); > - free(existing); > + ovn_extend_table_info_destroy(existing); > } > > /* Remove entries in desired table that are created by the lflow_uuid */ > @@ -99,29 +219,39 @@ void > ovn_extend_table_remove_desired(struct ovn_extend_table *table, > const struct uuid *lflow_uuid) > { > - struct ovn_extend_table_info *e, *next_e; > - HMAP_FOR_EACH_SAFE (e, next_e, hmap_node, &table->desired) { > - if (uuid_equals(&e->lflow_uuid, lflow_uuid)) { > + struct ovn_extend_table_lflow_to_desired *l = > + ovn_extend_table_find_desired_by_lflow(table, lflow_uuid); > + > + if (!l) { > + return; > + } > + > + hmap_remove(&table->lflow_to_desired, &l->hmap_node); > + struct ovn_extend_table_lflow_ref *r, *next_r; > + LIST_FOR_EACH_SAFE (r, next_r, list_node, &l->desired) { > + struct ovn_extend_table_info *e = r->desired; > + ovn_extend_info_del_lflow_ref(r); > + if (hmap_is_empty(&e->references)) { > + VLOG_DBG("%s: %s, "UUID_FMT, __func__, > + e->name, UUID_ARGS(lflow_uuid)); > hmap_remove(&table->desired, &e->hmap_node); > - free(e->name); > if (e->new_table_id) { > bitmap_set0(table->table_ids, e->table_id); > } > - free(e); > + ovn_extend_table_info_destroy(e); > } > } > - > + free(l); > } > > static struct ovn_extend_table_info* > ovn_extend_info_clone(struct ovn_extend_table_info *source) > { > - struct ovn_extend_table_info *clone = xmalloc(sizeof *clone); > - clone->name = xstrdup(source->name); > - clone->table_id = source->table_id; > - clone->new_table_id = source->new_table_id; > - clone->hmap_node.hash = source->hmap_node.hash; > - clone->lflow_uuid = source->lflow_uuid; > + struct ovn_extend_table_info *clone = > + ovn_extend_table_info_alloc(source->name, > + source->table_id, > + source->new_table_id, > + source->hmap_node.hash); > return clone; > } > > @@ -155,8 +285,12 @@ 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) && > - table_info->new_table_id) { > + 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, > + UUID_ARGS(&lflow_uuid)); > + ovn_extend_info_add_lflow_ref(table, table_info, &lflow_uuid); > return table_info->table_id; > } > } > @@ -183,15 +317,13 @@ ovn_extend_table_assign_id(struct ovn_extend_table > *table, const char *name, > } > bitmap_set1(table->table_ids, table_id); > > - table_info = xmalloc(sizeof *table_info); > - table_info->name = xstrdup(name); > - table_info->table_id = table_id; > - table_info->hmap_node.hash = hash; > - table_info->new_table_id = new_table_id; > - table_info->lflow_uuid = lflow_uuid; > + 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); > > + ovn_extend_info_add_lflow_ref(table, table_info, &lflow_uuid); > + > return table_id; > } > diff --git a/lib/extend-table.h b/lib/extend-table.h > index 833dced..4d80cfd 100644 > --- a/lib/extend-table.h > +++ b/lib/extend-table.h > @@ -31,16 +31,45 @@ struct ovn_extend_table { > * for allocated group ids in either > * desired or existing. */ > struct hmap desired; > + struct hmap lflow_to_desired; /* Index for looking up desired table > + * items from given lflow uuid, with > + * ovn_extend_table_lflow_to_desired nodes. > + */ > struct hmap existing; > }; > > +struct ovn_extend_table_lflow_to_desired { > + struct hmap_node hmap_node; /* In ovn_extend_table.lflow_to_desired. */ > + struct uuid lflow_uuid; > + struct ovs_list desired; /* List of desired items used by the lflow. */ > +}; > + > struct ovn_extend_table_info { > struct hmap_node hmap_node; > char *name; /* Name for the table entity. */ > - struct uuid lflow_uuid; > uint32_t table_id; > bool new_table_id; /* 'True' if 'table_id' was reserved from > * ovn_extend_table's 'table_ids' bitmap. */ > + struct hmap references; /* The lflows that are using this item, with > + * ovn_extend_table_lflow_ref nodes. Only useful > + * for items in ovn_extend_table.desired. */ > +}; > + > +/* Maintains the link between a lflow and an ovn_extend_table_info item in > + * ovn_extend_table.desired, indexed by both > + * ovn_extend_table_lflow_to_desired.desired and > + * ovn_extend_table_info.references. > + * > + * The struct is allocated whenever a new reference happens. > + * It destroyed when a lflow is deleted (for all the desired table_info > + * used by it), or when the lflow_to_desired table is being cleared. > + * */ > +struct ovn_extend_table_lflow_ref { > + struct hmap_node hmap_node; /* In ovn_extend_table_info.references. */ > + struct ovs_list list_node; /* In > ovn_extend_table_lflow_to_desired.desired. > + */ > + struct uuid lflow_uuid; > + struct ovn_extend_table_info *desired; > }; > > void ovn_extend_table_init(struct ovn_extend_table *); > diff --git a/tests/ovn.at b/tests/ovn.at > index 5dc7af2..7bb97ed 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -7380,6 +7380,61 @@ OVN_CLEANUP([hv]) > AT_CLEANUP > > > +AT_SETUP([ovn -- same meter used by multiple logical flows]) > +AT_KEYWORDS([ovn]) > +ovn_start > + > +net_add n1 > + > +sim_add hv > +as hv > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > +for i in lp1 lp2; do > + ovs-vsctl -- add-port br-int $i -- \ > + set interface $i external-ids:iface-id=$i \ > + options:tx_pcap=hv/$i-tx.pcap \ > + options:rxq_pcap=hv/$i-rx.pcap > +done > + > +lp1_mac="f0:00:00:00:00:01" > +lp1_ip="192.168.1.2" > + > +lp2_mac="f0:00:00:00:00:02" > +lp2_ip="192.168.1.3" > + > +ovn-nbctl ls-add lsw0 > +ovn-nbctl --wait=sb lsp-add lsw0 lp1 > +ovn-nbctl --wait=sb lsp-add lsw0 lp2 > +ovn-nbctl lsp-set-addresses lp1 $lp1_mac > +ovn-nbctl lsp-set-addresses lp2 $lp2_mac > +ovn-nbctl --wait=sb sync > + > +ovn-appctl -t ovn-controller vlog/set file:dbg > + > +# Add acl1 and acl2 using same meter. > +ovn-nbctl meter-add http-rl1 drop 10 pktps > +ovn-nbctl --log --meter=http-rl1 acl-add lsw0 to-lport 1000 'tcp.dst==80' > drop \ > + -- --log --meter=http-rl1 acl-add lsw0 to-lport 1000 'tcp.dst==81' > allow > + > +ovn-nbctl --wait=hv sync > + > +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br-int | grep meter], [0], > [ignore], [ignore]) > + > +# Delete acl1, meter should be kept in OVS > +ovn-nbctl acl-del lsw0 to-lport 1000 'tcp.dst==80' > +ovn-nbctl --wait=hv sync > +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br-int | grep meter], [0], > [ignore], [ignore]) > + > +# Delete acl2, meter should be deleted in OVS > +ovn-nbctl acl-del lsw0 to-lport 1000 'tcp.dst==81' > +ovn-nbctl --wait=hv sync > +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br-int | grep meter], [1]) > + > +OVN_CLEANUP([hv]) > +AT_CLEANUP > + > + > AT_SETUP([ovn -- DSCP marking and meter check]) > AT_KEYWORDS([ovn]) > ovn_start > -- > 2.1.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
