On 9/6/23 16:39, Mark Michelson wrote: > Thanks Ales, > Hi Ales, Mark,
> Acked-by: Mark Michelson <[email protected]> > > 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 <[email protected]> >> --- >> 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
