On Tue, Jun 1, 2021 at 9:33 AM Dumitru Ceara <[email protected]> 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 <[email protected]>
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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
