[ovs-dev] [PATCH ovn v2] northd: Fix datapath swapping in logical flows.

Dumitru Ceara dceara at redhat.com
Fri Dec 11 17:27:21 UTC 2020


On 12/11/20 12:36 PM, Ilya Maximets 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 <dceara at redhat.com>
> Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
> ---

Hi Ilya,

[...]

>  
> @@ -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 has only one,

Nit: s/has/have

> +     * 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)) {

Nit: It's personal preference but I think it would be clearer to write
this as:

if (logical_datapath_od && ovn_datapath_is_stale(logical_datapath_od))

Otherwise the rest looks fine to me.  With the nits addressed (or at
least the first one):

Acked-by: Dumitru Ceara <dceara at redhat.com>

Thanks,
Dumitru



More information about the dev mailing list