On Fri, Dec 11, 2020 at 11:31 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
>
> Commit that introduced support for logical datapath groups removed
> the 'logical_datapath' column from the lflow hash. If datapath groups
> disabled, there will be many flows that are same except for the
> 'logical_datapath' column, so they will have exactly same hash.
> Since 'logical_datapath' is not involved into comparison inside
> ovn_lflow_equal(), all these flows will be considered equal.
>
> While iterating over Southbound DB records, ovn_lflow_found() will
> return first of many "equal" flows and, likely, not the right one.
> Comparison of logical datapaths will say that we need to update
> the logical datapath, so it will be updated with the value from
> the flow that we just found. Flow that we found will be removed from
> the hash map. Similar procedure for the next DB record will give
> us different "equal" flow leading to the update of the
> 'logical_datapath' column for the next record. And so on. This
> way we will swap 'logical_datapath' column for almost all logical
> flows. Since we're not using same lflow more than once, resulted
> database will still be correct, but all these unnecessary steps will
> generate huge database transaction if we'll have at least one new
> or modified logical flow, because that will cause different order
> in which lflows will be found in a hash map.
>
> Fix that by re-adding 'logical_datapath' column back to hash and to
> the check for equality. This way correct lflows could be found, so
> there will be no unnecessary updates.
>
> Some functions and variables renamed to better describe their meaning.
> Also size_t replaced with uint32_t to avoid confusion. lib/hash.c
> always returns uint32_t as a hash value and that is important because
> we will want to update current hash with the datapath value and not to
> re-calculate it for all lflow columns.
>
> Fixes: bfed22400675 ("northd: Add support for Logical Datapath Groups.")
> Reported-by: Dumitru Ceara <dce...@redhat.com>
> Acked-by: Dumitru Ceara <dce...@redhat.com>
> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
Thanks. I applied this patch to master and branch-20.12.
Numan
> ---
>
> Version 3:
> * Fixed grammar and a smal style nits from Dumitru.
> * Added ACK from Dumitru.
>
> Version 2:
> * Removed unused incorrect condition from the ovn_logical_flow_hash_datapath
> function. It should not return 0, but should return 'hash' instead.
> Anyway, there is no code that uses this branch, so I just removed it to
> make the code cleaner:
> - return logical_datapath ? hash_add(hash,
> uuid_hash(logical_datapath)) : 0;
> + return hash_add(hash, uuid_hash(logical_datapath));
>
> lib/ovn-util.c | 15 ++++-
> lib/ovn-util.h | 2 +
> northd/ovn-northd.c | 134 ++++++++++++++++++++++++++++----------------
> 3 files changed, 101 insertions(+), 50 deletions(-)
>
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index f62c97e96..2136f90fe 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -505,8 +505,12 @@ ovn_is_known_nb_lsp_type(const char *type)
> uint32_t
> sbrec_logical_flow_hash(const struct sbrec_logical_flow *lf)
> {
> - return ovn_logical_flow_hash(lf->table_id, lf->pipeline,
> - lf->priority, lf->match, lf->actions);
> + const struct sbrec_datapath_binding *ld = lf->logical_datapath;
> + uint32_t hash = ovn_logical_flow_hash(lf->table_id, lf->pipeline,
> + lf->priority, lf->match,
> + lf->actions);
> +
> + return ld ? ovn_logical_flow_hash_datapath(&ld->header_.uuid, hash) :
> hash;
> }
>
> uint32_t
> @@ -520,6 +524,13 @@ ovn_logical_flow_hash(uint8_t table_id, const char
> *pipeline,
> return hash_string(actions, hash);
> }
>
> +uint32_t
> +ovn_logical_flow_hash_datapath(const struct uuid *logical_datapath,
> + uint32_t hash)
> +{
> + return hash_add(hash, uuid_hash(logical_datapath));
> +}
> +
> bool
> datapath_is_switch(const struct sbrec_datapath_binding *ldp)
> {
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index 1d2f7a9c5..679f47a97 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -105,6 +105,8 @@ uint32_t sbrec_logical_flow_hash(const struct
> sbrec_logical_flow *);
> uint32_t ovn_logical_flow_hash(uint8_t table_id, const char *pipeline,
> uint16_t priority,
> const char *match, const char *actions);
> +uint32_t ovn_logical_flow_hash_datapath(const struct uuid *logical_datapath,
> + uint32_t hash);
> bool datapath_is_switch(const struct sbrec_datapath_binding *);
> int datapath_snat_ct_zone(const struct sbrec_datapath_binding *ldp);
> void ovn_conn_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 957242367..d94c735ee 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -4097,7 +4097,8 @@ ovn_igmp_group_destroy(struct hmap *igmp_groups,
> struct ovn_lflow {
> struct hmap_node hmap_node;
>
> - struct hmapx od; /* Hash map of 'struct ovn_datapath *'. */
> + struct ovn_datapath *od; /* 'logical_datapath' in SB schema. */
> + struct hmapx od_group; /* Hash map of 'struct ovn_datapath *'. */
> enum ovn_stage stage;
> uint16_t priority;
> char *match;
> @@ -4109,9 +4110,9 @@ struct ovn_lflow {
> static void ovn_lflow_destroy(struct hmap *lflows, struct ovn_lflow *lflow);
> static struct ovn_lflow * ovn_lflow_find_by_lflow(const struct hmap *,
> const struct ovn_lflow *,
> - size_t hash);
> + uint32_t hash);
>
> -static size_t
> +static uint32_t
> ovn_lflow_hash(const struct ovn_lflow *lflow)
> {
> return ovn_logical_flow_hash(ovn_stage_get_table(lflow->stage),
> @@ -4132,19 +4133,21 @@ ovn_lflow_hint(const struct ovsdb_idl_row *row)
> static bool
> ovn_lflow_equal(const struct ovn_lflow *a, const struct ovn_lflow *b)
> {
> - return (a->stage == b->stage
> + return (a->od == b->od
> + && a->stage == b->stage
> && a->priority == b->priority
> && !strcmp(a->match, b->match)
> && !strcmp(a->actions, b->actions));
> }
>
> static void
> -ovn_lflow_init(struct ovn_lflow *lflow,
> +ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od,
> enum ovn_stage stage, uint16_t priority,
> char *match, char *actions, char *stage_hint,
> const char *where)
> {
> - hmapx_init(&lflow->od);
> + hmapx_init(&lflow->od_group);
> + lflow->od = od;
> lflow->stage = stage;
> lflow->priority = priority;
> lflow->match = match;
> @@ -4167,10 +4170,13 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct
> ovn_datapath *od,
> ovs_assert(ovn_stage_to_datapath_type(stage) ==
> ovn_datapath_get_type(od));
>
> struct ovn_lflow *old_lflow, *lflow;
> - size_t hash;
> + uint32_t hash;
>
> lflow = xmalloc(sizeof *lflow);
> - ovn_lflow_init(lflow, stage, priority,
> + /* While adding new logical flows we're not setting single datapath, but
> + * collecting a group. 'od' will be updated later for all flows with
> only
> + * one datapath in a group, so it could be hashed correctly. */
> + ovn_lflow_init(lflow, NULL, stage, priority,
> xstrdup(match), xstrdup(actions),
> ovn_lflow_hint(stage_hint), where);
>
> @@ -4179,12 +4185,12 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct
> ovn_datapath *od,
> old_lflow = ovn_lflow_find_by_lflow(lflow_map, lflow, hash);
> if (old_lflow) {
> ovn_lflow_destroy(NULL, lflow);
> - hmapx_add(&old_lflow->od, od);
> + hmapx_add(&old_lflow->od_group, od);
> return;
> }
> }
>
> - hmapx_add(&lflow->od, od);
> + hmapx_add(&lflow->od_group, od);
> hmap_insert(lflow_map, &lflow->hmap_node, hash);
> }
>
> @@ -4218,12 +4224,12 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct
> ovn_datapath *od,
> NULL, OVS_SOURCE_LOCATOR)
>
> static struct ovn_lflow *
> -ovn_lflow_find(struct hmap *lflows,
> +ovn_lflow_find(struct hmap *lflows, struct ovn_datapath *od,
> enum ovn_stage stage, uint16_t priority,
> const char *match, const char *actions, uint32_t hash)
> {
> struct ovn_lflow target;
> - ovn_lflow_init(&target, stage, priority,
> + ovn_lflow_init(&target, od, stage, priority,
> CONST_CAST(char *, match), CONST_CAST(char *, actions),
> NULL, NULL);
>
> @@ -4237,7 +4243,7 @@ ovn_lflow_destroy(struct hmap *lflows, struct ovn_lflow
> *lflow)
> if (lflows) {
> hmap_remove(lflows, &lflow->hmap_node);
> }
> - hmapx_destroy(&lflow->od);
> + hmapx_destroy(&lflow->od_group);
> free(lflow->match);
> free(lflow->actions);
> free(lflow->stage_hint);
> @@ -4247,7 +4253,7 @@ ovn_lflow_destroy(struct hmap *lflows, struct ovn_lflow
> *lflow)
>
> static struct ovn_lflow *
> ovn_lflow_find_by_lflow(const struct hmap *lflows,
> - const struct ovn_lflow *target, size_t hash)
> + const struct ovn_lflow *target, uint32_t hash)
> {
> struct ovn_lflow *lflow;
> HMAP_FOR_EACH_WITH_HASH (lflow, hmap_node, hash, lflows) {
> @@ -11318,33 +11324,28 @@ ovn_sb_insert_logical_dp_group(struct
> northd_context *ctx,
> }
>
> static void
> -ovn_sb_set_lflow_logical_datapaths(
> +ovn_sb_set_lflow_logical_dp_group(
> struct northd_context *ctx,
> struct hmap *dp_groups,
> const struct sbrec_logical_flow *sbflow,
> - const struct hmapx *od)
> + const struct hmapx *od_group)
> {
> - const struct hmapx_node *node;
> struct ovn_dp_group *dpg;
>
> - if (hmapx_count(od) == 1) {
> - HMAPX_FOR_EACH (node, od) {
> - struct ovn_datapath *dp = node->data;
> -
> - sbrec_logical_flow_set_logical_datapath(sbflow, dp->sb);
> - sbrec_logical_flow_set_logical_dp_group(sbflow, NULL);
> - break;
> - }
> + if (!hmapx_count(od_group)) {
> + sbrec_logical_flow_set_logical_dp_group(sbflow, NULL);
> return;
> }
>
> - dpg = ovn_dp_group_find(dp_groups, od, hash_int(hmapx_count(od), 0));
> + ovs_assert(hmapx_count(od_group) != 1);
> +
> + dpg = ovn_dp_group_find(dp_groups, od_group,
> + hash_int(hmapx_count(od_group), 0));
> ovs_assert(dpg != NULL);
>
> if (!dpg->dp_group) {
> dpg->dp_group = ovn_sb_insert_logical_dp_group(ctx, &dpg->map);
> }
> - sbrec_logical_flow_set_logical_datapath(sbflow, NULL);
> sbrec_logical_flow_set_logical_dp_group(sbflow, dpg->dp_group);
> }
>
> @@ -11365,28 +11366,55 @@ build_lflows(struct northd_context *ctx, struct
> hmap *datapaths,
>
> /* 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), 0);
> + uint32_t hash = hash_int(hmapx_count(&lflow->od_group), 0);
> struct ovn_dp_group *dpg;
>
> - if (hmapx_count(&lflow->od) == 1) {
> + ovs_assert(hmapx_count(&lflow->od_group));
> +
> + 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. */
> + hmapx_add(&single_dp_lflows, lflow);
> continue;
> }
>
> - dpg = ovn_dp_group_find(&dp_groups, &lflow->od, hash);
> + dpg = ovn_dp_group_find(&dp_groups, &lflow->od_group, hash);
> if (!dpg) {
> dpg = xzalloc(sizeof *dpg);
> - hmapx_clone(&dpg->map, &lflow->od);
> + hmapx_clone(&dpg->map, &lflow->od_group);
> hmap_insert(&dp_groups, &dpg->node, hash);
> }
> }
>
> + /* 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);
> +
> /* Push changes to the Logical_Flow table to database. */
> const struct sbrec_logical_flow *sbflow, *next_sbflow;
> SBREC_LOGICAL_FLOW_FOR_EACH_SAFE (sbflow, next_sbflow, ctx->ovnsb_idl) {
> struct sbrec_logical_dp_group *dp_group = sbflow->logical_dp_group;
> - struct ovn_datapath **od;
> + struct ovn_datapath **od, *logical_datapath_od = NULL;
> int n_datapaths = 0;
> size_t i;
>
> @@ -11403,45 +11431,52 @@ build_lflows(struct northd_context *ctx, struct
> hmap *datapaths,
>
> struct sbrec_datapath_binding *dp = sbflow->logical_datapath;
> if (dp) {
> - od[n_datapaths] = ovn_datapath_from_sbrec(datapaths, dp);
> - if (od[n_datapaths] && !ovn_datapath_is_stale(od[n_datapaths])) {
> - n_datapaths++;
> + logical_datapath_od = ovn_datapath_from_sbrec(datapaths, dp);
> + if (logical_datapath_od
> + && ovn_datapath_is_stale(logical_datapath_od)) {
> + logical_datapath_od = NULL;
> }
> }
>
> - if (!n_datapaths) {
> + if (!n_datapaths && !logical_datapath_od) {
> /* This lflow has no valid logical datapaths. */
> sbrec_logical_flow_delete(sbflow);
> free(od);
> continue;
> }
>
> - enum ovn_datapath_type dp_type = od[0]->nbs ? DP_SWITCH : DP_ROUTER;
> enum ovn_pipeline pipeline
> = !strcmp(sbflow->pipeline, "ingress") ? P_IN : P_OUT;
> + enum ovn_datapath_type dp_type;
>
> + if (n_datapaths) {
> + dp_type = od[0]->nbs ? DP_SWITCH : DP_ROUTER;
> + } else {
> + dp_type = logical_datapath_od->nbs ? DP_SWITCH : DP_ROUTER;
> + }
> lflow = ovn_lflow_find(
> - &lflows, ovn_stage_build(dp_type, pipeline, sbflow->table_id),
> + &lflows, logical_datapath_od,
> + ovn_stage_build(dp_type, pipeline, sbflow->table_id),
> sbflow->priority, sbflow->match, sbflow->actions, sbflow->hash);
> if (lflow) {
> /* This is a valid lflow. Checking if the datapath group needs
> * updates. */
> - bool update_datapaths = false;
> + bool update_dp_group = false;
>
> - if (n_datapaths != hmapx_count(&lflow->od)) {
> - update_datapaths = true;
> + if (n_datapaths != hmapx_count(&lflow->od_group)) {
> + update_dp_group = true;
> } else {
> for (i = 0; i < n_datapaths; i++) {
> - if (od[i] && !hmapx_contains(&lflow->od, od[i])) {
> - update_datapaths = true;
> + if (od[i] && !hmapx_contains(&lflow->od_group, od[i])) {
> + update_dp_group = true;
> break;
> }
> }
> }
>
> - if (update_datapaths) {
> - ovn_sb_set_lflow_logical_datapaths(ctx, &dp_groups,
> - sbflow, &lflow->od);
> + if (update_dp_group) {
> + ovn_sb_set_lflow_logical_dp_group(ctx, &dp_groups,
> + sbflow, &lflow->od_group);
> }
> /* This lflow updated. Not needed anymore. */
> ovn_lflow_destroy(&lflows, lflow);
> @@ -11457,8 +11492,11 @@ build_lflows(struct northd_context *ctx, struct hmap
> *datapaths,
> uint8_t table = ovn_stage_get_table(lflow->stage);
>
> sbflow = sbrec_logical_flow_insert(ctx->ovnsb_txn);
> - ovn_sb_set_lflow_logical_datapaths(ctx, &dp_groups,
> - sbflow, &lflow->od);
> + if (lflow->od) {
> + sbrec_logical_flow_set_logical_datapath(sbflow, lflow->od->sb);
> + }
> + ovn_sb_set_lflow_logical_dp_group(ctx, &dp_groups,
> + sbflow, &lflow->od_group);
> sbrec_logical_flow_set_pipeline(sbflow, pipeline);
> sbrec_logical_flow_set_table_id(sbflow, table);
> sbrec_logical_flow_set_priority(sbflow, lflow->priority);
> --
> 2.25.4
>
> _______________________________________________
> 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
