[ovs-dev] [PATCH ovn] northd: Reduce number of logical flow allocations.

Numan Siddique numans at ovn.org
Tue Jun 15 16:16:32 UTC 2021


On Tue, Jun 1, 2021 at 9:33 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> There's no need to allocate a logical flow structure if we're going to
> merge it to an existing one that refers to a datapath group.
>
> This saves a lot of xstrdup() calls followed immediately by free().
>
> Reported-at: https://bugzilla.redhat.com/1962818
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>

Thanks Dumitru.  I applied this patch to the main branch and also to
branch-21.06
as it seems a good candidate for backport due to the improvements
without much code changes.

Thanks
Numan

> ---
>  northd/ovn-northd.c | 99 ++++++++++++++++++++-------------------------
>  1 file changed, 44 insertions(+), 55 deletions(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index bf09eed59..07341f31c 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -4027,18 +4027,11 @@ 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 *,
> -                                                  uint32_t hash);
> -
> -static uint32_t
> -ovn_lflow_hash(const struct ovn_lflow *lflow)
> -{
> -    return ovn_logical_flow_hash(ovn_stage_get_table(lflow->stage),
> -                                 ovn_stage_get_pipeline_name(lflow->stage),
> -                                 lflow->priority, lflow->match,
> -                                 lflow->actions);
> -}
> +static struct ovn_lflow *ovn_lflow_find(const struct hmap *lflows,
> +                                        const struct ovn_datapath *od,
> +                                        enum ovn_stage stage,
> +                                        uint16_t priority, const char *match,
> +                                        const char *actions, uint32_t hash);
>
>  static char *
>  ovn_lflow_hint(const struct ovsdb_idl_row *row)
> @@ -4050,13 +4043,15 @@ ovn_lflow_hint(const struct ovsdb_idl_row *row)
>  }
>
>  static bool
> -ovn_lflow_equal(const struct ovn_lflow *a, const struct ovn_lflow *b)
> +ovn_lflow_equal(const struct ovn_lflow *a, const struct ovn_datapath *od,
> +                enum ovn_stage stage, uint16_t priority, const char *match,
> +                const char *actions)
>  {
> -    return (a->od == b->od
> -            && a->stage == b->stage
> -            && a->priority == b->priority
> -            && !strcmp(a->match, b->match)
> -            && !strcmp(a->actions, b->actions));
> +    return (a->od == od
> +            && a->stage == stage
> +            && a->priority == priority
> +            && !strcmp(a->match, match)
> +            && !strcmp(a->actions, actions));
>  }
>
>  static void
> @@ -4086,22 +4081,32 @@ static struct hashrow_locks lflow_locks;
>   * Version to use when locking is required.
>   */
>  static void
> -do_ovn_lflow_add(struct hmap *lflow_map, bool shared,
> -                 struct ovn_datapath *od,
> -                 uint32_t hash, struct ovn_lflow *lflow)
> +do_ovn_lflow_add(struct hmap *lflow_map, bool shared, struct ovn_datapath *od,
> +                 uint32_t hash, enum ovn_stage stage, uint16_t priority,
> +                 const char *match, const char *actions,
> +                 const struct ovsdb_idl_row *stage_hint,
> +                 const char *where)
>  {
>
>      struct ovn_lflow *old_lflow;
> +    struct ovn_lflow *lflow;
>
>      if (shared && use_logical_dp_groups) {
> -        old_lflow = ovn_lflow_find_by_lflow(lflow_map, lflow, hash);
> +        old_lflow = ovn_lflow_find(lflow_map, NULL, stage, priority, match,
> +                                   actions, hash);
>          if (old_lflow) {
> -            ovn_lflow_destroy(NULL, lflow);
>              hmapx_add(&old_lflow->od_group, od);
>              return;
>          }
>      }
>
> +    lflow = xmalloc(sizeof *lflow);
> +    /* 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);
>      hmapx_add(&lflow->od_group, od);
>      hmap_insert_fast(lflow_map, &lflow->hmap_node, hash);
>  }
> @@ -4115,25 +4120,21 @@ 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 *lflow;
>      uint32_t hash;
>
> -    lflow = xmalloc(sizeof *lflow);
> -    /* 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);
> -
> -    hash = ovn_lflow_hash(lflow);
> +    hash = ovn_logical_flow_hash(ovn_stage_get_table(stage),
> +                                 ovn_stage_get_pipeline_name(stage),
> +                                 priority, match,
> +                                 actions);
>
>      if (use_logical_dp_groups && use_parallel_build) {
>          lock_hash_row(&lflow_locks, hash);
> -        do_ovn_lflow_add(lflow_map, shared, od, hash, lflow);
> +        do_ovn_lflow_add(lflow_map, shared, od, hash, stage, priority, match,
> +                         actions, stage_hint, where);
>          unlock_hash_row(&lflow_locks, hash);
>      } else {
> -        do_ovn_lflow_add(lflow_map, shared, od, hash, lflow);
> +        do_ovn_lflow_add(lflow_map, shared, od, hash, stage, priority, match,
> +                         actions, stage_hint, where);
>      }
>  }
>
> @@ -4167,16 +4168,17 @@ 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, struct ovn_datapath *od,
> +ovn_lflow_find(const struct hmap *lflows, const 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, od, stage, priority,
> -                   CONST_CAST(char *, match), CONST_CAST(char *, actions),
> -                   NULL, NULL);
> -
> -    return ovn_lflow_find_by_lflow(lflows, &target, hash);
> +    struct ovn_lflow *lflow;
> +    HMAP_FOR_EACH_WITH_HASH (lflow, hmap_node, hash, lflows) {
> +        if (ovn_lflow_equal(lflow, od, stage, priority, match, actions)) {
> +            return lflow;
> +        }
> +    }
> +    return NULL;
>  }
>
>  static void
> @@ -4194,19 +4196,6 @@ 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, uint32_t hash)
> -{
> -    struct ovn_lflow *lflow;
> -    HMAP_FOR_EACH_WITH_HASH (lflow, hmap_node, hash, lflows) {
> -        if (ovn_lflow_equal(lflow, target)) {
> -            return lflow;
> -        }
> -    }
> -    return NULL;
> -}
> -
>  /* Appends port security constraints on L2 address field 'eth_addr_field'
>   * (e.g. "eth.src" or "eth.dst") to 'match'.  'ps_addrs', with 'n_ps_addrs'
>   * elements, is the collection of port_security constraints from an
> --
> 2.27.0
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list