Hi Jakub,

This patch fixes the issue. However, I think the resulting code can lead to some confusion. The name of the first parameter given to ovn_logical_flow_hash() is called "logical_datapath". The result of your patch is that the actual first parameter passed to that function is either a logical switch or logical router UUID, not a logical datapath UUID.

Looking more closely, I think the intent was to pass a SB logical datapath UUID to the function. The error was that ovn_lflow_hash() in ovn-northd.c mistakenly thought that an ovn_datapath's header._uuid is a logical datapath UUID. I think the original intent was probably to pass lflow->od->sb->_header.uuid to ovn_logical_flow_hash() from ovn_lflow_hash().

We can go with your patch, but I would suggest changing the name of the first parameter of ovn_logical_flow_hash() to avoid confusion. Alternately, you can go with the suggestion I made above. The possible advantage it has is that it requires no lookups of external-ids in the southbound datapath.

On 02/22/2018 03:54 PM, Jakub Sitnicki wrote:
Use the logical switch/router UUID as hash input, instead of the UUID of
the Datapath_Binding row, when calculating the hash value for lflows in
the SBDB.

Otherwise the hash value will never match the one computed from NBDB
contents, which will force ovn-northd to constantly drop and attempt to
re-insert all the lflows.

This brings down the performance boost from caching the hash values
computed for logical flows in SBDB down to the expected level:

   Children      Self  Command     Shared Object        Symbol
before:
     76.19%     0.01%  ovn-northd  ovn-northd           [.] ovnnb_db_run
     11.04%     0.43%  ovn-northd  ovn-northd           [.] ovn_lflow_find
after:
     75.16%     0.05%  ovn-northd  ovn-northd           [.] ovnnb_db_run
      2.49%     0.17%  ovn-northd  ovn-northd           [.] ovn_lflow_find

Fixes: 8bf332225d4a ("ovn-northd: Reduce amount of flow hashing.")
Signed-off-by: Jakub Sitnicki <j...@redhat.com>
---
  ovn/lib/ovn-util.c | 8 +++++++-
  1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/ovn/lib/ovn-util.c b/ovn/lib/ovn-util.c
index e9464e926..1761defd9 100644
--- a/ovn/lib/ovn-util.c
+++ b/ovn/lib/ovn-util.c
@@ -339,7 +339,13 @@ sbrec_logical_flow_hash(const struct sbrec_logical_flow 
*lf)
          return 0;
      }
- return ovn_logical_flow_hash(&ld->header_.uuid,
+    struct uuid ld_uuid;
+    if (!smap_get_uuid(&ld->external_ids, "logical-switch", &ld_uuid) &&
+        !smap_get_uuid(&ld->external_ids, "logical-router", &ld_uuid)) {
+        return 0;
+    }
+
+    return ovn_logical_flow_hash(&ld_uuid,
                                   lf->table_id, lf->pipeline,
                                   lf->priority, lf->match, lf->actions);
  }


_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to