On Sat, Oct 26, 2019 at 2:37 AM Mark Michelson <[email protected]> wrote:
>
> As stated in previous commits, conjunctive matches have an issue where
> it is possible to install multiple flows that have identical matches.
> This results in ambiguity, and can lead to features (such as ACLs) not
> functioning properly.
>
> This change fixes the problem by combining conjunctions with identical
> matches into a single flow. As an example, in the past we may have had
> something like:
>
> nw_dst=10.0.0.1 actions=conjunction(2,1/2)
> nw_dst=10.0.0.1 actions=conjunction(3,1/2)
>
> This commit changes this into
>
> nw_dst=10.0.0.1 actions=conjunction(2,1/2),conjunction(3,1/2)
>
> This way, there is only a single flow with the proscribed match, and
> there is no ambiguity.
>
> Signed-off-by: Mark Michelson <[email protected]>
> ---
> controller/lflow.c | 5 ++--
> controller/ofctrl.c | 73
> +++++++++++++++++++++++++++++++++++++++++++++--------
> controller/ofctrl.h | 6 +++++
> tests/ovn.at | 17 +++++--------
> 4 files changed, 79 insertions(+), 22 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index e3ed20cd4..34b7c36a6 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -736,8 +736,9 @@ consider_logical_flow(
> dst->clause = src->clause;
> dst->n_clauses = src->n_clauses;
> }
> - ofctrl_add_flow(flow_table, ptable, lflow->priority, 0,
> &m->match,
> - &conj, &lflow->header_.uuid);
> +
> + ofctrl_add_or_append_flow(flow_table, ptable, lflow->priority, 0,
> + &m->match, &conj,
> &lflow->header_.uuid);
> ofpbuf_uninit(&conj);
> }
> }
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 3131baff0..afb0036f1 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -69,6 +69,11 @@ struct ovn_flow {
> uint64_t cookie;
> };
>
> +static struct ovn_flow *ovn_flow_alloc(uint8_t table_id, uint16_t priority,
> + uint64_t cookie,
> + const struct match *match,
> + const struct ofpbuf *actions,
> + const struct uuid *sb_uuid);
> static uint32_t ovn_flow_match_hash(const struct ovn_flow *);
> static struct ovn_flow *ovn_flow_lookup(struct hmap *flow_table,
> const struct ovn_flow *target,
> @@ -657,16 +662,8 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table
> *flow_table,
> const struct uuid *sb_uuid,
> bool log_duplicate_flow)
> {
> - struct ovn_flow *f = xmalloc(sizeof *f);
> - f->table_id = table_id;
> - f->priority = priority;
> - minimatch_init(&f->match, match);
> - f->ofpacts = xmemdup(actions->data, actions->size);
> - f->ofpacts_len = actions->size;
> - f->sb_uuid = *sb_uuid;
> - f->match_hmap_node.hash = ovn_flow_match_hash(f);
> - f->uuid_hindex_node.hash = uuid_hash(&f->sb_uuid);
> - f->cookie = cookie;
> + struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie, match,
> + actions, sb_uuid);
>
> ovn_flow_log(f, "ofctrl_add_flow");
>
> @@ -721,9 +718,65 @@ ofctrl_add_flow(struct ovn_desired_flow_table
> *desired_flows,
> ofctrl_check_and_add_flow(desired_flows, table_id, priority, cookie,
> match, actions, sb_uuid, true);
> }
> +
> +void
> +ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows,
> + uint8_t table_id, uint16_t priority, uint64_t
> cookie,
> + const struct match *match,
> + const struct ofpbuf *actions,
> + const struct uuid *sb_uuid)
> +{
> + struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie, match,
> + actions, sb_uuid);
> +
> + ovn_flow_log(f, "ofctrl_add_or_append_flow");
> +
> + struct ovn_flow *existing;
> + existing = ovn_flow_lookup(&desired_flows->match_flow_table, f, false);
> + if (existing) {
> + /* There's already a flow with this particular match. Append the
> + * action to that flow rather than adding a new flow
> + */
> + uint64_t compound_stub[64 / 8];
> + struct ofpbuf compound;
> + ofpbuf_use_stub(&compound, compound_stub, sizeof(compound_stub));
> + ofpbuf_put(&compound, existing->ofpacts, existing->ofpacts_len);
> + ofpbuf_put(&compound, f->ofpacts, f->ofpacts_len);
Instead of making use of a stub and copying the existing and new
actions, can't we just
copy the new actions to "existing->ofpacts" using ofpbuf_put() ?
ofpbuf_put() will take care of reallocating the memory if required.
Other than that, this and the 1st patch of this series, looks good to me.
Thanks
Numan
> +
> + free(existing->ofpacts);
> + existing->ofpacts = xmemdup(compound.data, compound.size);
> + existing->ofpacts_len = compound.size;
> +
> + ovn_flow_destroy(f);
> + } else {
> + hmap_insert(&desired_flows->match_flow_table, &f->match_hmap_node,
> + f->match_hmap_node.hash);
> + hindex_insert(&desired_flows->uuid_flow_table, &f->uuid_hindex_node,
> + f->uuid_hindex_node.hash);
> + }
> +}
>
> /* ovn_flow. */
>
> +static struct ovn_flow *
> +ovn_flow_alloc(uint8_t table_id, uint16_t priority, uint64_t cookie,
> + const struct match *match, const struct ofpbuf *actions,
> + const struct uuid *sb_uuid)
> +{
> + struct ovn_flow *f = xmalloc(sizeof *f);
> + f->table_id = table_id;
> + f->priority = priority;
> + minimatch_init(&f->match, match);
> + f->ofpacts = xmemdup(actions->data, actions->size);
> + f->ofpacts_len = actions->size;
> + f->sb_uuid = *sb_uuid;
> + f->match_hmap_node.hash = ovn_flow_match_hash(f);
> + f->uuid_hindex_node.hash = uuid_hash(&f->sb_uuid);
> + f->cookie = cookie;
> +
> + return f;
> +}
> +
> /* Returns a hash of the match key in 'f'. */
> static uint32_t
> ovn_flow_match_hash(const struct ovn_flow *f)
> diff --git a/controller/ofctrl.h b/controller/ofctrl.h
> index 1e9ac16b9..21d2ce648 100644
> --- a/controller/ofctrl.h
> +++ b/controller/ofctrl.h
> @@ -70,6 +70,12 @@ void ofctrl_add_flow(struct ovn_desired_flow_table *,
> uint8_t table_id,
> const struct match *, const struct ofpbuf *ofpacts,
> const struct uuid *);
>
> +void ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows,
> + uint8_t table_id, uint16_t priority,
> + uint64_t cookie, const struct match *match,
> + const struct ofpbuf *actions,
> + const struct uuid *sb_uuid);
> +
> void ofctrl_remove_flows(struct ovn_desired_flow_table *, const struct uuid
> *);
>
> void ovn_desired_flow_table_init(struct ovn_desired_flow_table *);
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 641a646fc..50d8efeec 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -12247,7 +12247,7 @@ ovn-nbctl create Address_Set name=set1 \
> addresses=\"10.0.0.4\",\"10.0.0.5\",\"10.0.0.6\"
> ovn-nbctl create Address_Set name=set2 \
> addresses=\"10.0.0.7\",\"10.0.0.8\",\"10.0.0.9\"
> -ovn-nbctl acl-add ls1 to-lport 1002 \
> +ovn-nbctl acl-add ls1 to-lport 1001 \
> 'ip4 && ip4.src == $set1 && ip4.dst == $set1' allow
> ovn-nbctl acl-add ls1 to-lport 1001 \
> 'ip4 && ip4.src == $set1 && ip4.dst == $set2' drop
> @@ -12296,7 +12296,7 @@ cat 2.expected > expout
> $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets
> AT_CHECK([cat 2.packets], [0], [expout])
>
> -# There should be total of 12 flows present with conjunction action and 2
> flows
> +# There should be total of 9 flows present with conjunction action and 2
> flows
> # with conj match. Eg.
> # table=44, priority=2002,conj_id=2,metadata=0x1 actions=resubmit(,45)
> # table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop
> @@ -12306,14 +12306,11 @@ AT_CHECK([cat 2.packets], [0], [expout])
> # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(3,2/2)
> # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(3,2/2)
> # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(3,2/2)
> -# priority=2002,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(2,1/2)
> -# priority=2002,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(2,1/2)
> -# priority=2002,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(2,1/2)
> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(3,1/2)
> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(3,1/2)
> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(3,1/2)
> -
> -OVS_WAIT_UNTIL([test 12 = `as hv1 ovs-ofctl dump-flows br-int | \
> +# priority=2002,ip,metadata=0x1,nw_src=10.0.0.6
> actions=conjunction(2,1/2),conjunction(3,1/2)
> +# priority=2002,ip,metadata=0x1,nw_src=10.0.0.4
> actions=conjunction(2,1/2),conjunction(3,1/2)
> +# priority=2002,ip,metadata=0x1,nw_src=10.0.0.5
> actions=conjunction(2,1/2),conjunction(3,1/2)
> +
> +OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows br-int | \
> grep conjunction | wc -l`])
> OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \
> grep conj_id | wc -l`])
> --
> 2.14.5
>
> _______________________________________________
> 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
