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

Dumitru Ceara dceara at redhat.com
Tue Jun 1 13:32:50 UTC 2021


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>
---
 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



More information about the dev mailing list