On Mon, Oct 28, 2019, 9:54 PM Numan Siddique <nusid...@redhat.com> wrote:
>
>
> On Mon, Oct 28, 2019, 9:29 PM Mark Michelson <mmich...@redhat.com> wrote:
>
>> On 10/28/19 11:33 AM, Numan Siddique wrote:
>> > On Sat, Oct 26, 2019 at 2:37 AM Mark Michelson <mmich...@redhat.com>
>> 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 <mmich...@redhat.com>
>>
>
Acked-by: Numan Siddique <num...@ovn.org>
>> ---
>> >> 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() ?
>>
>> You can't use ofpbuf_put() directly since existing->ofpacts is of type
>> ofpact, not ofpbuf.
>
>
> Oops. Please ignore my comment. I thought existing->ofpacts is of type
> ofpbuf.
>
> Sorry for the noise.
>
> Numan
>
> And I don't think you can cast existing->ofpacts to
>> an ofpbuf since existing->ofpacts was created from the 'data' member of
>> an ofpbuf; we don't have the metadata, such as the method by which the
>> data was allocated.
>>
>> So maybe it's possible to create a new ofpbuf but have it use the
>> existing->ofpacts buffer? I was looking and here are the issues:
>>
>> 1) ofpbuf_clone_data() creates a copy of the data passed in rather than
>> using it directly. So it still requires freeing existing->ofpacts and
>> reassigning it.
>> 2) ofpbuf_use_stub() would result in overwriting the data passed into it
>> unless we adjust the ofpbuf's size so that it points past the end of the
>> data we passed in. I can't find any example of this being done in OVS.
>>
>> If you have a good idea on how to re-use existing->ofpacts, then I'll
>> happily do it.
>>
>> >
>> > 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
>> >> d...@openvswitch.org
>> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
