On 9/6/23 16:39, Mark Michelson wrote:
> Thanks Ales,
>
Hi Ales, Mark,
> Acked-by: Mark Michelson <mmich...@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 <amu...@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
> 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
