On Thu, Feb 15, 2024 at 5:10 AM Xavier Simonart <[email protected]> wrote:
>
> Hi Numan
>
> Thanks for the quick review.
>
> On Wed, Feb 14, 2024 at 10:57 PM Numan Siddique <[email protected]> wrote:
>
> > On Wed, Feb 14, 2024 at 1:26 PM Xavier Simonart <[email protected]>
> > wrote:
> > >
> > > When (re)starting ovn-northd with an existing big nbdb,
> > > the first iteration of northd was very slow as trying to
> > > push all flows in a single bucket.
> > >
> > > Fixes: a623606052ea ("northd: Refactor lflow management into a separate
> > module.")
> > >
> > > Signed-off-by: Xavier Simonart <[email protected]>
> >
> > Hi Xavier,
> >
> > Thanks for the patch. Did the first iteration of north become very
> > slow after the lflow-mgr was added ?
> >
> Yes, it was introduced by "northd: Refactor lflow management into a
> separate module"
> We can test this easily using the check-perf tests. Using "check perf
> TESTSUITEFLAGS="-v 2", it takes around 16 seconds (on my system) before the
> patch and 213 seconds after.
> The difference gets exponential if we increase the number of ports.
>
> >
> > If I compare the code before these patches were merged, lflows_temp
> > hmap was not a fast hmap [1].
> > So I'm still confused how this regression was introduced ?
> >
> > [1] -
> > https://github.com/ovn-org/ovn/blob/branch-23.09/northd/northd.c#L16717
> >
> > If you see the code I linked above, lflows_temp is declared and
> > initialized as
> >
> > struct hmap lflows_temp = HMAP_INITIALIZER(&lflows_temp);
> >
> > And later a lflow is removed from the "lflows" hmap and inserted into
> > it. And finally it is swapped with "lflows"
> >
> > hmap_swap(lflows, &lflows_temp);
> > hmap_destroy(&lflows_temp);
> >
> > The same thing is done in the lflow-mgr too.
> >
> > I'm fine with this patch if it speeds up the first iteration. I want
> > to understand if this behavior is a regression or not.
> >
> > Sorry, I should have added more comments in the commit message. The extra
> time spent is not in lflow_table_sync_to_sb.
> But the hmap_swap in lflow_table_sync_to_sb, when the hmap is empty, swaps
> an empty hmap initialized with 128 buckets with an
> empty hmap initialized with one bucket.
> Then, when we build flows the first time, the "fast" hmap has only one
> bucket and could get millions of flows in this poor bucket.
> When initialized with 128 buckets, the initial flow build is not great, but
> still much better ...
> Another fix could have been to set the number of buckets in lflows_temp to
> 128, or to avoid the swap if both hmap are empty.
Now I understand what's going on. Thank you for the fix.
I applied to main and backported to branch-24.03.
Thanks
Numan
>
> > Thanks
> > Numan
> >
> > Thanks
> Xavier
>
> >
> > > ---
> > > northd/lflow-mgr.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
> > > index 61729e903..af072bf70 100644
> > > --- a/northd/lflow-mgr.c
> > > +++ b/northd/lflow-mgr.c
> > > @@ -260,6 +260,9 @@ lflow_table_sync_to_sb(struct lflow_table
> > *lflow_table,
> > > struct hmap *lflows = &lflow_table->entries;
> > > struct ovn_lflow *lflow;
> > >
> > > + fast_hmap_size_for(&lflows_temp,
> > > + lflow_table->max_seen_lflow_size);
> > > +
> > > /* Push changes to the Logical_Flow table to database. */
> > > const struct sbrec_logical_flow *sbflow;
> > > SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_SAFE (sbflow, sb_flow_table) {
> > > --
> > > 2.41.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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
