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

[Ilya Maximets]

On 12/11/20 6:27 PM, Dumitru Ceara wrote:
> 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

OK.

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

Makes sense.

I'll send v3.

> 
> 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
>