Hi Ilya! I’m not entirely clear on your point about the impact of this change on performance.
Formally, without my changes, two strings are already added to the hash: action and clone, which are usually much longer than just the stage name. I don’t think the recalculation of flows based on hashes is particularly significant. Could you explain the impact? Is this somehow related to datapath groups? I also thought about an alternative approach where we would know the hashes for each stage name in advance and simply add this hash instead of computing it from the string, since it’s a stateful value. However, ovn_stage_to_str doesn’t access the database but retrieves the string from memory, and this didn’t seem like a significant optimization to me. I conducted some research on the impact of this patch on the performance of upstream version. In a setup with 6000 logical switches and 15,000 ports, I collected some metrics regarding performance. The average utilization metrics for hash maps during recomputation were: • Without the patch: hmap_expand: 5928.750/sec (average), 98.8125/sec (overall average) • With the patch: hmap_expand: 5921.117/sec (average), 98.6853/sec (overall average) Statistics for lflows_to_sb: • With the patch: 154.000000 msec • Without the patch: 151.000000 msec These results seem to fall within the range of normal deviation. On 6 Mar 2025, at 16:50, Ilya Maximets <[email protected]> wrote: On 3/6/25 09:41, Alexandra Rukomoinikova wrote: When changing the order of stages in the LogicalFlow table in SB, some rules will be added with an incorrect stage-name in the external-ids of existing datapath groups. When adding a stage, new lflows in a new table with the same match and action that already existed with the same table-id will remain in the database with unupdated stage names. Signed-off-by: Alexandra Rukomoinikova <[email protected]> --- lib/ovn-util.c | 5 ++++- lib/ovn-util.h | 2 +- northd/lflow-mgr.c | 4 +++- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/ovn-util.c b/lib/ovn-util.c index c65b36bb5..d1572d5ec 100644 --- a/lib/ovn-util.c +++ b/lib/ovn-util.c @@ -646,16 +646,19 @@ sbrec_logical_flow_hash(const struct sbrec_logical_flow *lf) { return ovn_logical_flow_hash(lf->table_id, ovn_pipeline_from_name(lf->pipeline), + smap_get_def(&lf->external_ids, + "stage-name", ""), lf->priority, lf->match, lf->actions); } Hi, Alexandra. Thanks for the patch! Though, IIRC, it was an intentional decision to not include the stage name in the hash/comparison to avoid a significant performance impact that this hashmap lookup and the string hashing bring. The issue only affects debugging capabilities, but the performance impact of hashing the name on each database update and comparing extra strings on lflow generation as well as re-writing most of the flows on stage updates is significant and affects every operation in northd. Did you run some scale tests with this change? What's the impact in current OVN? Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
