[ovs-dev] [OVN Patch v2] northd: Optimize dp/lflow postprocessing
Mark Gray
mark.d.gray at redhat.com
Mon Oct 11 10:16:31 UTC 2021
On 08/10/2021 17:03, anton.ivanov at cambridgegreys.com wrote:
> From: Anton Ivanov <anton.ivanov at cambridgegreys.com>
>
> 1. Compute dp group hash only if there will be dp group processing.
> 2. Remove hmapx interim storage and related hmapx computation for
> single dp flows and replace it with a pre-sized hmap.
>
> Signed-off-by: Anton Ivanov <anton.ivanov at cambridgegreys.com>
> ---
I tested the performance of this using the following on a large database:
`ovn-appctl -t ovn-northd stopwatch/reset; ovn-appctl -t ovn-northd
stopwatch/show lflows_dp_groups `
With patch:
Run 1: 1572ms
Run 2: 1536ms
Run 3: 1562ms
Without patch:
Run 1: 1640ms
Run 2: 1622ms
Run 3: 1646ms
So looks like there is a ~5% improvement there.
It passed UTs and system tests.
One small comment below otherwise
Acked-by: Mark D. Gray <mark.d.gray at redhat.com>
> northd/northd.c | 48 ++++++++++++++++++++++++++++--------------------
> ovs | 2 +-
> 2 files changed, 29 insertions(+), 21 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.> index e42795ca0..a3c3dbcf9 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -13331,10 +13331,20 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths,
> stopwatch_start(LFLOWS_DP_GROUPS_STOPWATCH_NAME, time_msec());
> /* Collecting all unique datapath groups. */
> struct hmap dp_groups = HMAP_INITIALIZER(&dp_groups);
> - struct hmapx single_dp_lflows = HMAPX_INITIALIZER(&single_dp_lflows);
> - struct ovn_lflow *lflow;
> - HMAP_FOR_EACH (lflow, hmap_node, &lflows) {
> - uint32_t hash = hash_int(hmapx_count(&lflow->od_group), 0);
> + struct hmap single_dp_lflows;
> +
> + /* Single dp_flows will never grow bigger than lflows,
> + * thus the two hmaps will remain the same size regardless
> + * of how many elements we remove from lflows and add to
> + * single_dp_lflows.
> + * Note - lflows is always sized for at least 128 flows.
> + */
> + fast_hmap_size_for(&single_dp_lflows, max_seen_lflow_size);
> +
> + struct ovn_lflow *lflow, *next_lflow;
> + struct hmapx_node *node;
> + HMAP_FOR_EACH_SAFE (lflow, next_lflow, hmap_node, &lflows) {
> + uint32_t hash;
> struct ovn_dp_group *dpg;
>
> ovs_assert(hmapx_count(&lflow->od_group));
> @@ -13342,17 +13352,24 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths,
> if (hmapx_count(&lflow->od_group) == 1) {
> /* There is only one datapath, so it should be moved out of the
> * group to a single 'od'. */
> - const struct hmapx_node *node;
> HMAPX_FOR_EACH (node, &lflow->od_group) {
> lflow->od = node->data;
> break;
> }
> hmapx_clear(&lflow->od_group);
> +
> /* Logical flow should be re-hashed later to allow lookups. */
This comment should be changed. Maybe just remove "later"
> - hmapx_add(&single_dp_lflows, lflow);
> + hash = hmap_node_hash(&lflow->hmap_node);
> + /* Remove from lflows. */
> + hmap_remove(&lflows, &lflow->hmap_node);
> + hash = ovn_logical_flow_hash_datapath(&lflow->od->sb->header_.uuid,
> + hash);
> + /* Add to single_dp_lflows. */
> + hmap_insert_fast(&single_dp_lflows, &lflow->hmap_node, hash);
> continue;
> }
>
> + hash = hash_int(hmapx_count(&lflow->od_group), 0);
> dpg = ovn_dp_group_find(&dp_groups, &lflow->od_group, hash);
> if (!dpg) {
> dpg = xzalloc(sizeof *dpg);
> @@ -13362,19 +13379,11 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths,
> lflow->dpg = dpg;
> }
>
> - /* Adding datapath to the flow hash for logical flows that have only one,
> - * so they could be found by the southbound db record. */
> - const struct hmapx_node *node;
> - uint32_t hash;
> - HMAPX_FOR_EACH (node, &single_dp_lflows) {
> - lflow = node->data;
> - hash = hmap_node_hash(&lflow->hmap_node);
> - hmap_remove(&lflows, &lflow->hmap_node);
> - hash = ovn_logical_flow_hash_datapath(&lflow->od->sb->header_.uuid,
> - hash);
> - hmap_insert(&lflows, &lflow->hmap_node, hash);
> - }
> - hmapx_destroy(&single_dp_lflows);
> + /* Merge multiple and single dp hashes. */
> +
> + fast_hmap_merge(&lflows, &single_dp_lflows);
> +
> + hmap_destroy(&single_dp_lflows);
>
> /* Push changes to the Logical_Flow table to database. */
> const struct sbrec_logical_flow *sbflow, *next_sbflow;
> @@ -13507,7 +13516,6 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths,
> }
>
> stopwatch_stop(LFLOWS_DP_GROUPS_STOPWATCH_NAME, time_msec());
> - struct ovn_lflow *next_lflow;
> HMAP_FOR_EACH_SAFE (lflow, next_lflow, hmap_node, &lflows) {
> const char *pipeline = ovn_stage_get_pipeline_name(lflow->stage);
> uint8_t table = ovn_stage_get_table(lflow->stage);
>
More information about the dev
mailing list