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