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
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
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
turning the allocation complexity of that function fron O(N), where
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
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);
+ 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;
- }
-
- 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,
--
2.51.0
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
