| Message ID | 20210601133250.29369-1-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] northd: Reduce number of logical flow allocations. | expand |
On Tue, Jun 1, 2021 at 9:33 AM Dumitru Ceara <dceara@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@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 6/15/21 6:16 PM, Numan Siddique wrote: > On Tue, Jun 1, 2021 at 9:33 AM Dumitru Ceara <dceara@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@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 a lot!
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
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@redhat.com> --- northd/ovn-northd.c | 99 ++++++++++++++++++++------------------------- 1 file changed, 44 insertions(+), 55 deletions(-)