| Message ID | 20191025210631.7230-3-mmichels@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Reintroduce conjunctive matches | expand |
On Sat, Oct 26, 2019 at 2:37 AM Mark Michelson <mmichels@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 <mmichels@redhat.com> > --- > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 10/28/19 11:33 AM, Numan Siddique wrote: > On Sat, Oct 26, 2019 at 2:37 AM Mark Michelson <mmichels@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 <mmichels@redhat.com> >> --- >> 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. 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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Mon, Oct 28, 2019, 9:29 PM Mark Michelson <mmichels@redhat.com> wrote: > On 10/28/19 11:33 AM, Numan Siddique wrote: > > On Sat, Oct 26, 2019 at 2:37 AM Mark Michelson <mmichels@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 <mmichels@redhat.com> > >> --- > >> 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 > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Mon, Oct 28, 2019, 9:54 PM Numan Siddique <nusiddiq@redhat.com> wrote: > > > On Mon, Oct 28, 2019, 9:29 PM Mark Michelson <mmichels@redhat.com> wrote: > >> On 10/28/19 11:33 AM, Numan Siddique wrote: >> > On Sat, Oct 26, 2019 at 2:37 AM Mark Michelson <mmichels@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 <mmichels@redhat.com> >> > Acked-by: Numan Siddique <numans@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 >> >> dev@openvswitch.org >> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >>
I pushed the first two patches of the series to master. I'll resubmit patch three as a separate submission. On 10/28/19 1:45 PM, Numan Siddique wrote: > > > On Mon, Oct 28, 2019, 9:54 PM Numan Siddique <nusiddiq@redhat.com > <mailto:nusiddiq@redhat.com>> wrote: > > > > On Mon, Oct 28, 2019, 9:29 PM Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> wrote: > > On 10/28/19 11:33 AM, Numan Siddique wrote: > > On Sat, Oct 26, 2019 at 2:37 AM Mark Michelson > <mmichels@redhat.com <mailto:mmichels@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 <mmichels@redhat.com > <mailto:mmichels@redhat.com>> > > > Acked-by: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > > >> --- > >> controller/lflow.c | 5 ++-- > >> controller/ofctrl.c | 73 > +++++++++++++++++++++++++++++++++++++++++++++-------- > >> controller/ofctrl.h | 6 +++++ > >> tests/ovn.at <http://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 <http://ovn.at> b/tests/ovn.at > <http://ovn.at> > >> index 641a646fc..50d8efeec 100644 > >> --- a/tests/ovn.at <http://ovn.at> > >> +++ b/tests/ovn.at <http://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 > <http://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 > >> dev@openvswitch.org <mailto:dev@openvswitch.org> > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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); + + 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`])
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 <mmichels@redhat.com> --- controller/lflow.c | 5 ++-- controller/ofctrl.c | 73 +++++++++++++++++++++++++++++++++++++++++++++-------- controller/ofctrl.h | 6 +++++ tests/ovn.at | 17 +++++-------- 4 files changed, 79 insertions(+), 22 deletions(-)