| Message ID | 20230829084753.209210-2-amusil@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2,1/2] ofctrl: Do not try to program long flows | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Thanks Ales, Acked-by: Mark Michelson <mmichels@redhat.com> Thankfully the extra overhead is in the much less frequently-hit case. On 8/29/23 04:47, Ales Musil wrote: > During ofctrl_add_or_append_flow we are able to combine > two flows with same match but different conjunctions. > However, the function didn't check if the conjunctions already > exist in the installed flow, which could result in conjunction > duplication and the flow would grow infinitely e.g. > actions=conjunction(1,1/2), conjunction(1,1/2) > > Make sure that we add only conjunctions that are not present > in the already existing flow. > > Reported-at: https://bugzilla.redhat.com/2175928 > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > controller/ofctrl.c | 56 ++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 55 insertions(+), 1 deletion(-) > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > index 9de8a145f..e39cef7aa 100644 > --- a/controller/ofctrl.c > +++ b/controller/ofctrl.c > @@ -1273,6 +1273,37 @@ ofctrl_add_flow_metered(struct ovn_desired_flow_table *desired_flows, > meter_id, as_info, true); > } > > +struct ofpact_ref { > + struct hmap_node hmap_node; > + struct ofpact *ofpact; > +}; > + > +static struct ofpact_ref * > +ofpact_ref_find(const struct hmap *refs, const struct ofpact *ofpact) > +{ > + uint32_t hash = hash_bytes(ofpact, ofpact->len, 0); > + > + struct ofpact_ref *ref; > + HMAP_FOR_EACH_WITH_HASH (ref, hmap_node, hash, refs) { > + if (ofpacts_equal(ref->ofpact, ref->ofpact->len, > + ofpact, ofpact->len)) { > + return ref; > + } > + } > + > + return NULL; > +} > + > +static void > +ofpact_refs_destroy(struct hmap *refs) > +{ > + struct ofpact_ref *ref; > + HMAP_FOR_EACH_POP (ref, hmap_node, refs) { > + free(ref); > + } > + hmap_destroy(refs); > +} > + > /* Either add a new flow, or append actions on an existing flow. If the > * flow existed, a new link will also be created between the new sb_uuid > * and the existing flow. */ > @@ -1292,6 +1323,21 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, > meter_id); > existing = desired_flow_lookup_conjunctive(desired_flows, &f->flow); > if (existing) { > + struct hmap existing_conj = HMAP_INITIALIZER(&existing_conj); > + > + struct ofpact *ofpact; > + OFPACT_FOR_EACH (ofpact, existing->flow.ofpacts, > + existing->flow.ofpacts_len) { > + if (ofpact->type != OFPACT_CONJUNCTION) { > + continue; > + } > + > + struct ofpact_ref *ref = xmalloc(sizeof *ref); > + ref->ofpact = ofpact; > + uint32_t hash = hash_bytes(ofpact, ofpact->len, 0); > + hmap_insert(&existing_conj, &ref->hmap_node, hash); > + } > + > /* There's already a flow with this particular match and action > * 'conjunction'. Append the action to that flow rather than > * adding a new flow. > @@ -1301,7 +1347,15 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, > ofpbuf_use_stub(&compound, compound_stub, sizeof(compound_stub)); > ofpbuf_put(&compound, existing->flow.ofpacts, > existing->flow.ofpacts_len); > - ofpbuf_put(&compound, f->flow.ofpacts, f->flow.ofpacts_len); > + > + OFPACT_FOR_EACH (ofpact, f->flow.ofpacts, f->flow.ofpacts_len) { > + if (ofpact->type != OFPACT_CONJUNCTION || > + !ofpact_ref_find(&existing_conj, ofpact)) { > + ofpbuf_put(&compound, ofpact, OFPACT_ALIGN(ofpact->len)); > + } > + } > + > + ofpact_refs_destroy(&existing_conj); > > mem_stats.desired_flow_usage -= desired_flow_size(existing); > free(existing->flow.ofpacts);
On 9/6/23 16:39, Mark Michelson wrote: > Thanks Ales, > Hi Ales, Mark, > Acked-by: Mark Michelson <mmichels@redhat.com> > > Thankfully the extra overhead is in the much less frequently-hit case. > We still do a O(N^2) processing, I wonder if it's a lot of work/complexity to add the map to all 'struct desired_flow'. But yes, in general, it's likely fine to do things as this patch proposes. However, to make sure we catch it in the future, should we log/count whenever we have a lot of entries in this specific hmap ('existing_conj')? Thanks, Dumitru > On 8/29/23 04:47, Ales Musil wrote: >> During ofctrl_add_or_append_flow we are able to combine >> two flows with same match but different conjunctions. >> However, the function didn't check if the conjunctions already >> exist in the installed flow, which could result in conjunction >> duplication and the flow would grow infinitely e.g. >> actions=conjunction(1,1/2), conjunction(1,1/2) >> >> Make sure that we add only conjunctions that are not present >> in the already existing flow. >> >> Reported-at: https://bugzilla.redhat.com/2175928 >> Signed-off-by: Ales Musil <amusil@redhat.com> >> --- >> controller/ofctrl.c | 56 ++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 55 insertions(+), 1 deletion(-) >> >> diff --git a/controller/ofctrl.c b/controller/ofctrl.c >> index 9de8a145f..e39cef7aa 100644 >> --- a/controller/ofctrl.c >> +++ b/controller/ofctrl.c >> @@ -1273,6 +1273,37 @@ ofctrl_add_flow_metered(struct >> ovn_desired_flow_table *desired_flows, >> meter_id, as_info, true); >> } >> +struct ofpact_ref { >> + struct hmap_node hmap_node; >> + struct ofpact *ofpact; >> +}; >> + >> +static struct ofpact_ref * >> +ofpact_ref_find(const struct hmap *refs, const struct ofpact *ofpact) >> +{ >> + uint32_t hash = hash_bytes(ofpact, ofpact->len, 0); >> + >> + struct ofpact_ref *ref; >> + HMAP_FOR_EACH_WITH_HASH (ref, hmap_node, hash, refs) { >> + if (ofpacts_equal(ref->ofpact, ref->ofpact->len, >> + ofpact, ofpact->len)) { >> + return ref; >> + } >> + } >> + >> + return NULL; >> +} >> + >> +static void >> +ofpact_refs_destroy(struct hmap *refs) >> +{ >> + struct ofpact_ref *ref; >> + HMAP_FOR_EACH_POP (ref, hmap_node, refs) { >> + free(ref); >> + } >> + hmap_destroy(refs); >> +} >> + >> /* Either add a new flow, or append actions on an existing flow. If the >> * flow existed, a new link will also be created between the new >> sb_uuid >> * and the existing flow. */ >> @@ -1292,6 +1323,21 @@ ofctrl_add_or_append_flow(struct >> ovn_desired_flow_table *desired_flows, >> meter_id); >> existing = desired_flow_lookup_conjunctive(desired_flows, >> &f->flow); >> if (existing) { >> + struct hmap existing_conj = HMAP_INITIALIZER(&existing_conj); >> + >> + struct ofpact *ofpact; >> + OFPACT_FOR_EACH (ofpact, existing->flow.ofpacts, >> + existing->flow.ofpacts_len) { >> + if (ofpact->type != OFPACT_CONJUNCTION) { >> + continue; >> + } >> + >> + struct ofpact_ref *ref = xmalloc(sizeof *ref); >> + ref->ofpact = ofpact; >> + uint32_t hash = hash_bytes(ofpact, ofpact->len, 0); >> + hmap_insert(&existing_conj, &ref->hmap_node, hash); >> + } >> + >> /* There's already a flow with this particular match and action >> * 'conjunction'. Append the action to that flow rather than >> * adding a new flow. >> @@ -1301,7 +1347,15 @@ ofctrl_add_or_append_flow(struct >> ovn_desired_flow_table *desired_flows, >> ofpbuf_use_stub(&compound, compound_stub, >> sizeof(compound_stub)); >> ofpbuf_put(&compound, existing->flow.ofpacts, >> existing->flow.ofpacts_len); >> - ofpbuf_put(&compound, f->flow.ofpacts, f->flow.ofpacts_len); >> + >> + OFPACT_FOR_EACH (ofpact, f->flow.ofpacts, f->flow.ofpacts_len) { >> + if (ofpact->type != OFPACT_CONJUNCTION || >> + !ofpact_ref_find(&existing_conj, ofpact)) { >> + ofpbuf_put(&compound, ofpact, >> OFPACT_ALIGN(ofpact->len)); >> + } >> + } >> + >> + ofpact_refs_destroy(&existing_conj); >> mem_stats.desired_flow_usage -= desired_flow_size(existing); >> free(existing->flow.ofpacts); > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 9de8a145f..e39cef7aa 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -1273,6 +1273,37 @@ ofctrl_add_flow_metered(struct ovn_desired_flow_table *desired_flows, meter_id, as_info, true); } +struct ofpact_ref { + struct hmap_node hmap_node; + struct ofpact *ofpact; +}; + +static struct ofpact_ref * +ofpact_ref_find(const struct hmap *refs, const struct ofpact *ofpact) +{ + uint32_t hash = hash_bytes(ofpact, ofpact->len, 0); + + struct ofpact_ref *ref; + HMAP_FOR_EACH_WITH_HASH (ref, hmap_node, hash, refs) { + if (ofpacts_equal(ref->ofpact, ref->ofpact->len, + ofpact, ofpact->len)) { + return ref; + } + } + + return NULL; +} + +static void +ofpact_refs_destroy(struct hmap *refs) +{ + struct ofpact_ref *ref; + HMAP_FOR_EACH_POP (ref, hmap_node, refs) { + free(ref); + } + hmap_destroy(refs); +} + /* Either add a new flow, or append actions on an existing flow. If the * flow existed, a new link will also be created between the new sb_uuid * and the existing flow. */ @@ -1292,6 +1323,21 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, meter_id); existing = desired_flow_lookup_conjunctive(desired_flows, &f->flow); if (existing) { + struct hmap existing_conj = HMAP_INITIALIZER(&existing_conj); + + struct ofpact *ofpact; + OFPACT_FOR_EACH (ofpact, existing->flow.ofpacts, + existing->flow.ofpacts_len) { + if (ofpact->type != OFPACT_CONJUNCTION) { + continue; + } + + struct ofpact_ref *ref = xmalloc(sizeof *ref); + ref->ofpact = ofpact; + uint32_t hash = hash_bytes(ofpact, ofpact->len, 0); + hmap_insert(&existing_conj, &ref->hmap_node, hash); + } + /* There's already a flow with this particular match and action * 'conjunction'. Append the action to that flow rather than * adding a new flow. @@ -1301,7 +1347,15 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, ofpbuf_use_stub(&compound, compound_stub, sizeof(compound_stub)); ofpbuf_put(&compound, existing->flow.ofpacts, existing->flow.ofpacts_len); - ofpbuf_put(&compound, f->flow.ofpacts, f->flow.ofpacts_len); + + OFPACT_FOR_EACH (ofpact, f->flow.ofpacts, f->flow.ofpacts_len) { + if (ofpact->type != OFPACT_CONJUNCTION || + !ofpact_ref_find(&existing_conj, ofpact)) { + ofpbuf_put(&compound, ofpact, OFPACT_ALIGN(ofpact->len)); + } + } + + ofpact_refs_destroy(&existing_conj); mem_stats.desired_flow_usage -= desired_flow_size(existing); free(existing->flow.ofpacts);
During ofctrl_add_or_append_flow we are able to combine two flows with same match but different conjunctions. However, the function didn't check if the conjunctions already exist in the installed flow, which could result in conjunction duplication and the flow would grow infinitely e.g. actions=conjunction(1,1/2), conjunction(1,1/2) Make sure that we add only conjunctions that are not present in the already existing flow. Reported-at: https://bugzilla.redhat.com/2175928 Signed-off-by: Ales Musil <amusil@redhat.com> --- controller/ofctrl.c | 56 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-)