Skip to content

Commit

Permalink
ovn-controller: Avoid reprocessing same lflows in the same I-P run.
Browse files Browse the repository at this point in the history 
For I-P node lflow_output, different change handlers can trigger
reprocessing lflows. However, it is a waste to reprocess the same lflow
multiple times in the same run, because each processing of the lflow is
against the same input data.

For example, if a lflow references an addresset A and a port-group P, it
is possible that both A and P are changed in the same run, the same
lflow reprocess would be triggered by both
lflow_output_addr_sets_handler() and lflow_output_port_groups_handler().
Another example may incur a even bigger waste is that when a NB port-group
include ports across a big number of datapaths, so the lflow is
referencing a big number of SB port-groups with DP group including all
those DPs. When there are multiple port changes in the NB port-group,
there can be multiple small SB port-group changes, and so the lflows can
be reprocessed multiple times.

This patch avoid reprocessing the same lflow in the same I-P run, which
should improve performance in above scenarios.

There is only one exception when a lflow may still be reprocessed more
than once: if a lflow A is processed, which results in a compound
conjunction added to an existed flow generated by an exited lflow B, and
then lflow B needs to be reprocessed in the same run, which would
cause flood-remove and reprocess lflow A. In this case lflow A is
processed twice.

Apart from the performance gains, there is also a potential problem of
DP group fixed by this patch. If there is addrset/pg change and at the
same run there is also a new local datapath monitored, then the existed
code would firstly handle the addrset/pg changes causing a lflow being
processed against the DP group including the new DP, which could have
conjunction flows, but later the same lflow is reprocessed by
lflow_output_runtime_data_handler()->lflow_add_flows_for_datapath() for
the new DP only. Because lflows adding conjunction flows will not be
checked against redundancy but only tries to combine the conjunction
actions, it would result in redundanct conjunction actions added to the
same flow, which is also linked to the same SB lflow twice. The
mechanism of this patch will avoid this problem.

Signed-off-by: Han Zhou <hzhou@ovn.org>
Acked-by: Mark Michelson <mmichels@redhat.com>
  • Loading branch information
@hzhou8
hzhou8 committed Feb 5, 2022
1 parent 889037e commit 8994d4a
Show file tree
Hide file tree
Showing 3 changed files with 147 additions and 21 deletions.
143 changes: 123 additions & 20 deletions controller/lflow.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,16 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
struct hmap *nd_ra_opts,
struct controller_event_options *controller_event_opts,
bool is_recompute,
struct lflow_ctx_in *l_ctx_in,
struct lflow_ctx_out *l_ctx_out);
static struct lflow_processed_node *
lflows_processed_find(struct hmap *lflows_processed,
const struct uuid *lflow_uuid);
static void lflows_processed_add(struct hmap *lflows_processed,
const struct uuid *lflow_uuid);
static void lflows_processed_remove(struct hmap *lflows_processed,
struct lflow_processed_node *node);
static void lflow_resource_add(struct lflow_resource_ref *, enum ref_type,
const char *ref_name, const struct uuid *);
static struct ref_lflow_node *ref_lflow_lookup(struct hmap *ref_lflow_table,
Expand Down Expand Up @@ -366,7 +374,7 @@ add_logical_flows(struct lflow_ctx_in *l_ctx_in,

SBREC_LOGICAL_FLOW_TABLE_FOR_EACH (lflow, l_ctx_in->logical_flow_table) {
consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
&nd_ra_opts, &controller_event_opts,
&nd_ra_opts, &controller_event_opts, true,
l_ctx_in, l_ctx_out);
}

Expand Down Expand Up @@ -413,6 +421,12 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in,
struct ofctrl_flood_remove_node *ofrn, *next;
SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow,
l_ctx_in->logical_flow_table) {
if (lflows_processed_find(l_ctx_out->lflows_processed,
&lflow->header_.uuid)) {
VLOG_DBG("lflow "UUID_FMT"has been processed, skip.",
UUID_ARGS(&lflow->header_.uuid));
continue;
}
VLOG_DBG("delete lflow "UUID_FMT, UUID_ARGS(&lflow->header_.uuid));
ofrn = xmalloc(sizeof *ofrn);
ofrn->sb_uuid = lflow->header_.uuid;
Expand All @@ -437,8 +451,20 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in,
if (lflow) {
VLOG_DBG("re-add lflow "UUID_FMT,
UUID_ARGS(&lflow->header_.uuid));

/* For the extra lflows that need to be reprocessed because of the
* flood remove, remove it from lflows_processed. */
struct lflow_processed_node *lfp_node =
lflows_processed_find(l_ctx_out->lflows_processed,
&lflow->header_.uuid);
if (lfp_node) {
VLOG_DBG("lflow "UUID_FMT"has been processed, now reprocess.",
UUID_ARGS(&lflow->header_.uuid));
lflows_processed_remove(l_ctx_out->lflows_processed, lfp_node);
}

consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
&nd_ra_opts, &controller_event_opts,
&nd_ra_opts, &controller_event_opts, false,
l_ctx_in, l_ctx_out);
}
}
Expand Down Expand Up @@ -473,15 +499,22 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
*changed = false;
bool ret = true;

hmap_remove(&l_ctx_out->lfrr->ref_lflow_table, &rlfn->node);
struct ovs_list lflows_todo = OVS_LIST_INITIALIZER(&lflows_todo);

struct lflow_ref_list_node *lrln, *next;
/* Detach the rlfn->lflow_uuids nodes from the lfrr table and clean
* up all other nodes related to the lflows that uses the resource,
* so that the old nodes won't interfere with updating the lfrr table
* when reparsing the lflows. */
struct lflow_ref_list_node *lrln, *lrln_uuid, *lrln_uuid_next;
HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) {
ovs_list_remove(&lrln->list_node);
if (lflows_processed_find(l_ctx_out->lflows_processed,
&lrln->lflow_uuid)) {
continue;
}
/* Use lflow_ref_list_node as list node to store the uuid.
* Other fields are not used here. */
lrln_uuid = xmalloc(sizeof *lrln_uuid);
lrln_uuid->lflow_uuid = lrln->lflow_uuid;
ovs_list_push_back(&lflows_todo, &lrln_uuid->list_node);
}
if (ovs_list_is_empty(&lflows_todo)) {
return true;
}

struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
Expand Down Expand Up @@ -509,17 +542,19 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
/* Re-parse the related lflows. */
/* Firstly, flood remove the flows from desired flow table. */
struct hmap flood_remove_nodes = HMAP_INITIALIZER(&flood_remove_nodes);
struct ofctrl_flood_remove_node *ofrn, *ofrn_next;
HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) {
LIST_FOR_EACH_SAFE (lrln_uuid, lrln_uuid_next, list_node, &lflows_todo) {
VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d,"
" name: %s.",
UUID_ARGS(&lrln->lflow_uuid),
UUID_ARGS(&lrln_uuid->lflow_uuid),
ref_type, ref_name);
ofctrl_flood_remove_add_node(&flood_remove_nodes, &lrln->lflow_uuid);
ofctrl_flood_remove_add_node(&flood_remove_nodes,
&lrln_uuid->lflow_uuid);
free(lrln_uuid);
}
ofctrl_flood_remove_flows(l_ctx_out->flow_table, &flood_remove_nodes);

/* Secondly, for each lflow that is actually removed, reprocessing it. */
struct ofctrl_flood_remove_node *ofrn, *ofrn_next;
HMAP_FOR_EACH (ofrn, hmap_node, &flood_remove_nodes) {
lflow_resource_destroy_lflow(l_ctx_out->lfrr, &ofrn->sb_uuid);
lflow_conj_ids_free(l_ctx_out->conj_ids, &ofrn->sb_uuid);
Expand All @@ -535,8 +570,19 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
continue;
}

/* For the extra lflows that need to be reprocessed because of the
* flood remove, remove it from lflows_processed. */
struct lflow_processed_node *lfp_node =
lflows_processed_find(l_ctx_out->lflows_processed,
&lflow->header_.uuid);
if (lfp_node) {
VLOG_DBG("lflow "UUID_FMT"has been processed, now reprocess.",
UUID_ARGS(&lflow->header_.uuid));
lflows_processed_remove(l_ctx_out->lflows_processed, lfp_node);
}

consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
&nd_ra_opts, &controller_event_opts,
&nd_ra_opts, &controller_event_opts, false,
l_ctx_in, l_ctx_out);
*changed = true;
}
Expand All @@ -546,12 +592,6 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
}
hmap_destroy(&flood_remove_nodes);

HMAP_FOR_EACH_SAFE (lrln, next, hmap_node, &rlfn->lflow_uuids) {
hmap_remove(&rlfn->lflow_uuids, &lrln->hmap_node);
free(lrln);
}
ref_lflow_node_destroy(rlfn);

dhcp_opts_destroy(&dhcp_opts);
dhcp_opts_destroy(&dhcpv6_opts);
nd_ra_opts_destroy(&nd_ra_opts);
Expand Down Expand Up @@ -969,11 +1009,54 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
free(matches);
}

static struct lflow_processed_node *
lflows_processed_find(struct hmap *lflows_processed,
const struct uuid *lflow_uuid)
{
struct lflow_processed_node *node;
HMAP_FOR_EACH_WITH_HASH (node, hmap_node, uuid_hash(lflow_uuid),
lflows_processed) {
if (uuid_equals(&node->lflow_uuid, lflow_uuid)) {
return node;
}
}
return NULL;
}

static void
lflows_processed_add(struct hmap *lflows_processed,
const struct uuid *lflow_uuid)
{
struct lflow_processed_node *node = xmalloc(sizeof *node);
node->lflow_uuid = *lflow_uuid;
hmap_insert(lflows_processed, &node->hmap_node, uuid_hash(lflow_uuid));
}

static void
lflows_processed_remove(struct hmap *lflows_processed,
struct lflow_processed_node *node)
{
hmap_remove(lflows_processed, &node->hmap_node);
free(node);
}

void
lflows_processed_destroy(struct hmap *lflows_processed)
{
struct lflow_processed_node *node, *next;
HMAP_FOR_EACH_SAFE (node, next, hmap_node, lflows_processed) {
hmap_remove(lflows_processed, &node->hmap_node);
free(node);
}
hmap_destroy(lflows_processed);
}

static void
consider_logical_flow(const struct sbrec_logical_flow *lflow,
struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
struct hmap *nd_ra_opts,
struct controller_event_options *controller_event_opts,
bool is_recompute,
struct lflow_ctx_in *l_ctx_in,
struct lflow_ctx_out *l_ctx_out)
{
Expand All @@ -987,6 +1070,13 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
}
ovs_assert(!dp_group || !dp);

if (!is_recompute) {
ovs_assert(!lflows_processed_find(l_ctx_out->lflows_processed,
&lflow->header_.uuid));
lflows_processed_add(l_ctx_out->lflows_processed,
&lflow->header_.uuid);
}

if (dp) {
consider_logical_flow__(lflow, dp,
dhcp_opts, dhcpv6_opts, nd_ra_opts,
Expand Down Expand Up @@ -1923,6 +2013,12 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp,
const struct sbrec_logical_flow *lflow;
SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL (
lflow, lf_row, l_ctx_in->sbrec_logical_flow_by_logical_datapath) {
if (lflows_processed_find(l_ctx_out->lflows_processed,
&lflow->header_.uuid)) {
continue;
}
lflows_processed_add(l_ctx_out->lflows_processed,
&lflow->header_.uuid);
consider_logical_flow__(lflow, dp, &dhcp_opts, &dhcpv6_opts,
&nd_ra_opts, &controller_event_opts,
l_ctx_in, l_ctx_out);
Expand All @@ -1949,6 +2045,13 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp,
sbrec_logical_flow_index_set_logical_dp_group(lf_row, ldpg);
SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL (
lflow, lf_row, l_ctx_in->sbrec_logical_flow_by_logical_dp_group) {
if (lflows_processed_find(l_ctx_out->lflows_processed,
&lflow->header_.uuid)) {
continue;
}
/* Don't call lflows_processed_add() because here we process the
* lflow only for one of the DPs in the DP group, which may be
* incomplete. */
consider_logical_flow__(lflow, dp, &dhcp_opts, &dhcpv6_opts,
&nd_ra_opts, &controller_event_opts,
l_ctx_in, l_ctx_out);
Expand Down
7 changes: 7 additions & 0 deletions controller/lflow.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,17 @@ struct lflow_ctx_out {
struct lflow_resource_ref *lfrr;
struct lflow_cache *lflow_cache;
struct conj_ids *conj_ids;
struct hmap *lflows_processed;
struct simap *hairpin_lb_ids;
struct id_pool *hairpin_id_pool;
};

struct lflow_processed_node {
struct hmap_node hmap_node; /* In ed_type_lflow_output.lflows_processed. */
struct uuid lflow_uuid;
};
void lflows_processed_destroy(struct hmap *lflows_processed);

void lflow_init(void);
void lflow_run(struct lflow_ctx_in *, struct lflow_ctx_out *);
void lflow_handle_cached_flows(struct lflow_cache *,
Expand Down
18 changes: 17 additions & 1 deletion controller/ovn-controller.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2176,6 +2176,11 @@ struct ed_type_lflow_output {
/* conjunciton ID usage information of lflows */
struct conj_ids conj_ids;

/* lflows processed in the current engine execution.
* Cleared by en_lflow_output_clear_tracked_data before each engine
* execution. */
struct hmap lflows_processed;

/* Data which is persistent and not cleared during
* full recompute. */
struct lflow_output_persistent_data pd;
Expand Down Expand Up @@ -2307,6 +2312,7 @@ init_lflow_ctx(struct engine_node *node,
l_ctx_out->meter_table = &fo->meter_table;
l_ctx_out->lfrr = &fo->lflow_resource_ref;
l_ctx_out->conj_ids = &fo->conj_ids;
l_ctx_out->lflows_processed = &fo->lflows_processed;
l_ctx_out->lflow_cache = fo->pd.lflow_cache;
l_ctx_out->hairpin_id_pool = fo->hd.pool;
l_ctx_out->hairpin_lb_ids = &fo->hd.ids;
Expand All @@ -2322,11 +2328,20 @@ en_lflow_output_init(struct engine_node *node OVS_UNUSED,
ovn_extend_table_init(&data->meter_table);
lflow_resource_init(&data->lflow_resource_ref);
lflow_conj_ids_init(&data->conj_ids);
hmap_init(&data->lflows_processed);
simap_init(&data->hd.ids);
data->hd.pool = id_pool_create(1, UINT32_MAX - 1);
return data;
}

static void
en_lflow_output_clear_tracked_data(void *data)
{
struct ed_type_lflow_output *flow_output_data = data;
lflows_processed_destroy(&flow_output_data->lflows_processed);
hmap_init(&flow_output_data->lflows_processed);
}

static void
en_lflow_output_cleanup(void *data)
{
Expand All @@ -2336,6 +2351,7 @@ en_lflow_output_cleanup(void *data)
ovn_extend_table_destroy(&flow_output_data->meter_table);
lflow_resource_destroy(&flow_output_data->lflow_resource_ref);
lflow_conj_ids_destroy(&flow_output_data->conj_ids);
lflows_processed_destroy(&flow_output_data->lflows_processed);
lflow_cache_destroy(flow_output_data->pd.lflow_cache);
simap_destroy(&flow_output_data->hd.ids);
id_pool_destroy(flow_output_data->hd.pool);
Expand Down Expand Up @@ -3213,7 +3229,7 @@ main(int argc, char *argv[])
ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve");
ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected");
ENGINE_NODE(pflow_output, "physical_flow_output");
ENGINE_NODE(lflow_output, "logical_flow_output");
ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lflow_output, "logical_flow_output");
ENGINE_NODE(flow_output, "flow_output");
ENGINE_NODE(addr_sets, "addr_sets");
ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups");
Expand Down

0 comments on commit 8994d4a

Please sign in to comment.