[ovs-dev] [PATCH ovn 4/4] extend-table: Fix reusing group/meter by multiple logical flows.
Dumitru Ceara
dceara at redhat.com
Fri Dec 20 21:09:38 UTC 2019
On Fri, Dec 6, 2019 at 11:29 PM Han Zhou <hzhou at ovn.org> 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 <hzhou at ovn.org>
Nice catch! Looks good to me.
Acked-by: Dumitru Ceara <dceara at redhat.com>
> ---
> 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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list