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 <dce...@redhat.com> >> Signed-off-by: Ilya Maximets <i.maxim...@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 <dce...@redhat.com> > > Thanks, > Dumitru > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev