On Fri, Sep 9, 2022 at 4:00 AM Ilya Maximets <[email protected]> wrote: > > On 8/25/22 11:03, Dumitru Ceara wrote: > > On 8/24/22 18:32, Ilya Maximets wrote: > >> On 8/24/22 17:57, Dumitru Ceara wrote: > >>> On 8/24/22 17:49, Ilya Maximets wrote: > >>>> On 8/24/22 17:43, Ilya Maximets wrote: > >>>>> On 8/23/22 16:42, Dumitru Ceara wrote: > >>>>>> After removing the possibility of disabling logical datapath groups the > >>>>>> above mentioned state should only be relevant when ovn-northd is > >>>>>> started > >>>>>> with more than one lflow processing threads (--n-threads > 1), to avoid > >>>>>> a very inefficient first lflow processing run due to a suboptimally > >>>>>> sized hmap. > >>>>>> > >>>>>> There's no need to ever go back to this state now that we don't allow > >>>>>> users to disable logical datapath groups. Moreover, the previous code > >>>>>> was wrongfully setting the state to STATE_INIT_HASH_SIZES at every > >>>>>> iteration, essentially disabling parallelization. > >>>>>> > >>>>>> Fixes: 9dea0c090e91 ("nb: Remove possibility of disabling logical > >>>>>> datapath groups.") > >>>>>> Reported-by: Ilya Maximets <[email protected]> > >>>>>> Signed-off-by: Dumitru Ceara <[email protected]> > >>>>>> --- > >>>>> > >>>>> Hi, Dumitru. Thanks for the patch! > >>> > >>> Hi Ilya, > >>> > >>>>> I've been using it in multiple scale tests since it was posted, > >>>>> and it does re-enable parallel lflow build. > >>>>> > >>> > >>> Thanks for trying it out! > >>> > >>>>> One issue though. Not sure how to fix that or what is the desired > >>>>> behavior, but what I noticed is: > >>>>> > >>>>> 1. Cluster with 3 northd instances starts up. > >>>>> 2. After a while all of them has been active at least once, due > >>>>> to leadership transfers or other things. > >>>>> 3. If one instance stays active for long, a lot of database changes > >>>>> can be accumulated. > >>>>> 4. Once the leadership flips, new active northd is not prepared for > >>>>> the amount of logical flows it suddenly needs to manage. So, > >>>>> it starts parallel processing with hmap significantly smaller > >>>>> than it actually needs right now. That iteration takes noticeably > >>>>> more time than a usual one. After the first iteration after > >>>>> becoming active, it can process lflows with a normal speed. > >>>>> > >>>>> So, I wonder if we can fall into a catastrophic scenario where > >>>>> the current hash map size is very small and amount of flows after > >>>>> re-connection is very high, so threads are only fighting for locks > >>>>> all the time. > >>>>> > >>>>> Do you have some thoughts on this? > >>>>> > >>> > >>> This seems like a possible scenario (although I wonder how often we'd > >>> hit it in practice). > >>> > >>> But this was a possible scenario before 9dea0c090e91 ("nb: Remove > >>> possibility of disabling logical datapath groups.") too, right? > >> > >> Probably, yes. There is no need to block this particular patch as > >> it is fixing the separate bug. So, > >> > >> Acked-by: Ilya Maximets <[email protected]> > >> > > > > Thanks! > > > >>> > >>>>> Should we disable parallelism when northd goes into stand-up state? > >>>> > >>>> * I meant 'standby', of course. :/ > >>>> > >>> > >>> We could, but that means every time northd becomes active it will do one > >>> first inefficient run. Can we instead store the max_seen_lflow_size > >>> value in the SB database and use the one we read from the DB? > >> > >> It's an interesting idea, if it can be implemented efficiently. I'm not > >> sure how much extra traffic or northd wakeups that will generate. > >> It might make more sense to store it in NB, just to avoid spamming all > >> the controllers with monitor-all. Also, NB DB generally has more room > >> performance-wise. > >> > > > > I'll try to work on a patch to store it in NB; we already write to NB > > for other stuff like Logical_Switch_Port.up so it should be acceptable. > > > > I'll run some benchmarks to see if there's any measurable negative > > impact because of this and report back. > > Sounds reasonable. > > Meanwhile, Han, Mark, Numan, can we, please, accept the current fix?
Thanks. I applied this patch to main and to branch-22.09. Numan > > The parallelization is completely broken without it on both the main > branch and brach-22.09 as well. > > Thanks! > Best regards, Ilya Maximets. > _______________________________________________ > 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
