[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