Hi Ales,
Thanks for the patch!
On 9/11/25 1:44 PM, Ales Musil wrote:
> The function has only single call site and was used for modifying
> flows where the only actions in the list were conjunctions. By
> specializing we can make durther assumptions that allows us to
Typo: durther
> optimize as much as possible. The main issue with the function was
> very high number of allocations, 90% of them were actually to perform
> a duplication check when we try to append new conjnctions into
Typo: conjnctions
> already existing flow.
>
> The strategy to avoid those allocations is to allocate the array
> of references only once instead of going one by name effectively
"one by name"? Did you mean to say "one by one"?
> turning the allocation complexity of that function fron O(N), where
Typo: fron
> N is the number of already existing conjunctions into O(1) where we
> allocate constant number of times depending on the code path.
>
> This also improves the performance of operations directly related
> to conjunctions, mainly port group and address set modifications
> within ACLs.
>
> Below are numbers from a test run with 2000 LSPs, 1000 PGs, 1000 ACLs
> that resulted with flows that had 1000 conjunctions as an action.
>
> Without the change:
> ovn-controller(s) completion: 910ms
> node: lflow_output, handler for input port_groups took 427ms
>
> With the change:
> ovn-controller(s) completion: 407ms
> node: lflow_output, handler for input port_groups took 157ms
>
> So the speed up for the sync call is on avarage aroung 2.2x faster> and the
> speedup of precessing those lflows due to port group change
> is on avarage 2.7x faster. This is also not limited only to port
Typos: avarage (x2), aroung, precessing
> groups, but the address set changes resulting in conjunctions will
> benefit too.
>
> Fixes: 5ad4e5395b9e ("ofctrl: Prevent conjunction duplication")
> Fixes: e659bab31a91 ("Combine conjunctions with identical matches into one
> flow.")
> Signed-off-by: Ales Musil <amu...@redhat.com>
> ---
> controller/lflow.c | 23 ++------
> controller/ofctrl.c | 136 +++++++++++++++++++++++++-------------------
> controller/ofctrl.h | 15 ++---
> 3 files changed, 92 insertions(+), 82 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index b75ae5c0d..11ec33faf 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -938,24 +938,11 @@ add_matches_to_flow_table(const struct
> sbrec_logical_flow *lflow,
> if (vector_len(&m->conjunctions) > 1) {
> ovs_assert(!as_info.name);
> }
> - uint64_t conj_stubs[64 / 8];
> - struct ofpbuf conj;
> -
> - ofpbuf_use_stub(&conj, conj_stubs, sizeof conj_stubs);
> - const struct cls_conjunction *src;
> - VECTOR_FOR_EACH_PTR (&m->conjunctions, src) {
> - struct ofpact_conjunction *dst =
> ofpact_put_CONJUNCTION(&conj);
> - dst->id = src->id;
> - dst->clause = src->clause;
> - dst->n_clauses = src->n_clauses;
> - }
> -
> - ofctrl_add_or_append_flow(l_ctx_out->flow_table, ptable,
> - lflow->priority, 0,
> - &m->match, &conj, &lflow->header_.uuid,
> - ctrl_meter_id,
> - as_info.name ? &as_info : NULL);
> - ofpbuf_uninit(&conj);
> + ofctrl_add_or_append_conj_flow(l_ctx_out->flow_table, ptable,
> + lflow->priority, &m->match,
> + &m->conjunctions,
> + &lflow->header_.uuid,
> ctrl_meter_id,
> + as_info.name ? &as_info : NULL);
> }
> }
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 2a518161f..20d4bed01 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -290,6 +290,11 @@ static void remove_flows_from_sb_to_flow(struct
> ovn_desired_flow_table *,
> struct sb_to_flow *,
> const char *log_msg,
> struct uuidset *flood_remove_nodes);
> +static void ovn_flow_uninit(struct ovn_flow *f);
> +static void ovn_flow_init(struct ovn_flow *f, uint8_t table_id,
> + uint16_t priority, uint64_t cookie,
> + const struct match *match, void *actions,
> + size_t action_len, uint32_t meter_id);
>
> /* OpenFlow connection to the switch. */
> static struct rconn *swconn;
> @@ -1300,77 +1305,91 @@ ofpact_ref_find(const struct hmap *refs, const struct
> ofpact *ofpact)
> return NULL;
> }
>
> -static void
> -ofpact_refs_destroy(struct hmap *refs)
> +static inline void
> +ofpacts_append(struct ovn_flow *flow, const struct ofpbuf *append)
> {
> - struct ofpact_ref *ref;
> - HMAP_FOR_EACH_POP (ref, hmap_node, refs) {
> - free(ref);
> + size_t new_len = flow->ofpacts_len + append->size;
> +
> + flow->ofpacts = xrealloc(flow->ofpacts, new_len);
> + memcpy((uint8_t *) flow->ofpacts + flow->ofpacts_len,
> + append->data, append->size);
> + flow->ofpacts_len = new_len;
> +}
> +
> +static inline struct ofpbuf *
> +create_conjuction_actions(const struct vector *conjunctions,
> + const struct hmap *existing)
> +{
> + size_t len = vector_len(conjunctions);
> + struct ofpbuf *ofpbuf =
> + ofpbuf_new(sizeof(struct ofpact_conjunction) * len);
> +
> + const struct cls_conjunction *cls;
> + VECTOR_FOR_EACH_PTR (conjunctions, cls) {
> + struct ofpact_conjunction *conj = ofpact_put_CONJUNCTION(ofpbuf);
> + conj->id = cls->id;
> + conj->clause = cls->clause;
> + conj->n_clauses = cls->n_clauses;
> +
> + if (existing && ofpact_ref_find(existing, &conj->ofpact)) {
> + ofpbuf_pull(ofpbuf, sizeof *conj);
> + }
> }
> - hmap_destroy(refs);
> +
> + return ofpbuf;
> }
>
> /* 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. */
> 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,
> - uint32_t meter_id,
> - const struct addrset_info *as_info)
> +ofctrl_add_or_append_conj_flow(struct ovn_desired_flow_table *desired_flows,
> + uint8_t table_id, uint16_t priority,
> + const struct match *match,
> + const struct vector *conjunctions,
> + const struct uuid *sb_uuid,
> + uint32_t meter_id,
> + const struct addrset_info *as_info)
> {
> struct desired_flow *existing;
> struct desired_flow *f;
> -
> - f = desired_flow_alloc(table_id, priority, cookie, match, actions,
> - meter_id);
> - existing = desired_flow_lookup_conjunctive(desired_flows, &f->flow);
> + struct ofpbuf *actions;
> +
> + /* Create flow just to search for existing one, this avoids the
> allocation
> + * of ofpacts buffer if it's not needed yet. */
> + struct ovn_flow search_flow;
> + ovn_flow_init(&search_flow, table_id, priority, 0,
> + match, NULL, 0, meter_id);
> + existing = desired_flow_lookup_conjunctive(desired_flows, &search_flow);
> if (existing) {
> struct hmap existing_conj = HMAP_INITIALIZER(&existing_conj);
> -
> + /* We can calculate the length directly because the flow actions
> + * consist only of conjunctions. Make a rough assert to ensure
> + * that is that case in the future. */
> + size_t conj_size = sizeof (struct ofpact_conjunction);
Nit: I think it should be "sizeof(...)".
> + ovs_assert(!(existing->flow.ofpacts_len % conj_size));
> + struct ofpact_ref *refs =
> + xmalloc(sizeof *refs * existing->flow.ofpacts_len / conj_size);
> +
> + size_t index = 0;
> struct ofpact *ofpact;
> OFPACT_FOR_EACH (ofpact, existing->flow.ofpacts,
> existing->flow.ofpacts_len) {
> - if (ofpact->type != OFPACT_CONJUNCTION) {
> - continue;
> - }
Just to be extra sure we don't miss a case could we also add an
assertion here? E.g.:
ovs_assert(ofpact->type == OFPACT_CONJUNCTION);
Or a VLOG_WARN_RL(), I'm on the fence whether we should crash or not if
we determine there's a bug here.
I see that all flows here must be "pure" conjunctive flows but in theory
flow_action_has_conj() could return true also if the flow has a mix of
conj and non-conjunction actions.
> -
> - struct ofpact_ref *ref = xmalloc(sizeof *ref);
> + struct ofpact_ref *ref = &refs[index++];
> 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.
> - */
> - uint64_t compound_stub[64 / 8];
> - struct ofpbuf compound;
> - ofpbuf_use_stub(&compound, compound_stub, sizeof(compound_stub));
> - ofpbuf_put(&compound, existing->flow.ofpacts,
> - existing->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);
> -
> + actions = create_conjuction_actions(conjunctions, &existing_conj);
> mem_stats.desired_flow_usage -= desired_flow_size(existing);
> - free(existing->flow.ofpacts);
> - existing->flow.ofpacts = xmemdup(compound.data, compound.size);
> - existing->flow.ofpacts_len = compound.size;
> + ofpacts_append(&existing->flow, actions);
> mem_stats.desired_flow_usage += desired_flow_size(existing);
>
> - ofpbuf_uninit(&compound);
> - desired_flow_destroy(f);
> +
> + free(refs);
> + hmap_destroy(&existing_conj);
> +
> f = existing;
>
> /* Since the flow now shared by more than one SB lflows, don't track
> @@ -1388,17 +1407,20 @@ ofctrl_add_or_append_flow(struct
> ovn_desired_flow_table *desired_flows,
> }
> link_flow_to_sb(desired_flows, f, sb_uuid, NULL);
> } else {
> + actions = create_conjuction_actions(conjunctions, NULL);
> + f = desired_flow_alloc(table_id, priority, 0, match, actions,
> + meter_id);
> hmap_insert(&desired_flows->match_flow_table, &f->match_hmap_node,
> f->flow.hash);
> link_flow_to_sb(desired_flows, f, sb_uuid, as_info);
> }
> track_flow_add_or_modify(desired_flows, f);
>
> - if (existing) {
> - ovn_flow_log(&f->flow, "ofctrl_add_or_append_flow (append)");
> - } else {
> - ovn_flow_log(&f->flow, "ofctrl_add_or_append_flow (add)");
> - }
> + ovn_flow_log(&f->flow, existing
> + ? "ofctrl_add_or_append_conj_flow (append)"
> + : "ofctrl_add_or_append_conj_flow (add)");
> + ovn_flow_uninit(&search_flow);
> + ofpbuf_delete(actions);
> }
>
> void
> @@ -1621,13 +1643,13 @@ remove_flows_from_sb_to_flow(struct
> ovn_desired_flow_table *flow_table,
> static void
> ovn_flow_init(struct ovn_flow *f, uint8_t table_id, uint16_t priority,
> uint64_t cookie, const struct match *match,
> - const struct ofpbuf *actions, uint32_t meter_id)
> + void *actions, size_t action_len, uint32_t meter_id)
> {
> 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->ofpacts = actions ? xmemdup(actions, action_len) : NULL;
> + f->ofpacts_len = action_len;
> f->hash = ovn_flow_match_hash(f);
> f->cookie = cookie;
> f->ctrl_meter_id = meter_id;
> @@ -1651,8 +1673,8 @@ desired_flow_alloc(uint8_t table_id, uint16_t priority,
> uint64_t cookie,
> ovs_list_init(&f->track_list_node);
> f->installed_flow = NULL;
> f->is_deleted = false;
> - ovn_flow_init(&f->flow, table_id, priority, cookie, match, actions,
> - meter_id);
> + ovn_flow_init(&f->flow, table_id, priority, cookie, match, actions->data,
> + actions->size, meter_id);
>
> mem_stats.desired_flow_usage += desired_flow_size(f);
> return f;
> diff --git a/controller/ofctrl.h b/controller/ofctrl.h
> index 76e2fbece..abd2ff1c9 100644
> --- a/controller/ofctrl.h
> +++ b/controller/ofctrl.h
> @@ -34,6 +34,7 @@ struct sbrec_meter_table;
> struct sbrec_ecmp_nexthop_table;
> struct shash;
> struct tracked_acl_ids;
> +struct vector;
>
> struct ovn_desired_flow_table {
> /* Hash map flow table using flow match conditions as hash key.*/
> @@ -96,13 +97,13 @@ 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 *,
> - uint8_t table_id, uint16_t priority,
> - uint64_t cookie, const struct match *,
> - const struct ofpbuf *actions,
> - const struct uuid *sb_uuid,
> - uint32_t meter_id,
> - const struct addrset_info *);
> +void ofctrl_add_or_append_conj_flow(struct ovn_desired_flow_table *,
> + uint8_t table_id, uint16_t priority,
> + const struct match *,
> + const struct vector *conjunctions,
> + const struct uuid *sb_uuid,
> + uint32_t meter_id,
> + const struct addrset_info *);
>
> void ofctrl_add_flow_metered(struct ovn_desired_flow_table *desired_flows,
> uint8_t table_id, uint16_t priority,
The rest looks good to me. With the small typos/nits addressed and
optionally with the assert/log added:
Acked-by: Dumitru Ceara <dce...@redhat.com>
Thanks,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
