On 1/12/24 23:10, Frode Nordahl wrote: > On Fri, Jan 12, 2024 at 5:20 PM Ilya Maximets <i.maxim...@ovn.org> wrote: >> >> On 1/12/24 01:27, Ilya Maximets wrote: >>> On 1/10/24 20:29, Frode Nordahl wrote: >>>> During testing of the implementation of cooperative multitasking >>>> in the ovsdb-server we noticed that the very first yield being >>>> called in the jsonrpc-server always fired. >>>> >>>> This indicates that the operations being after/before storage run >>>> is taking too long as it currently is. >>>> >>>> Moving the storage run section to the top of the main loop lead >>>> to successful results using less yield calls as documented in the >>>> commit message of the next commit in this series. >>> >>> I don't really understand what exactly is taking so much time and >>> why moving this part of the code helps. Could you elaborate? >> >> I think, I got it. In the current version of a patch set we have: >> >> main(): >> 0. open_db() >> raft_alloc() <-- register callback >> read_db() <-- takes a lot of time >> main_loop() >> 1. run jsonrpc <-- yields and warns because read_db >> already took a lot of time. >> yield() >> raft_run() <-- updates the callback >> 2. storage_run() >> raft_run() <-- updates the callback >> >> The solution proposed in this patch is to swap 1 and 2, >> so we call raft_run() directly before we yield for the >> first time. >> >> I suppose, my seggestion to not have _register() function would >> solve that problem, because the callback will not be registered >> until the first call to raft_run(). >> >> Is that correct? > > Unfortunately, also when only having the _set() function the first > yield always fires. > > Putting log lines before/after every major function call in the > main_loop provides something like this: > 2024-01-12T22:01:46.968Z|00204|ovsdb_server|DBG|HELLO 0 > 2024-01-12T22:01:46.968Z|00205|ovsdb_server|DBG|HELLO 1 > 2024-01-12T22:01:46.968Z|00206|ovsdb_server|DBG|HELLO 2 > 2024-01-12T22:01:46.968Z|00207|ovsdb_server|DBG|HELLO 3 > 2024-01-12T22:01:46.968Z|00208|ovsdb_server|DBG|HELLO 4 > 2024-01-12T22:01:46.968Z|00209|ovsdb_server|DBG|HELLO 5 > 2024-01-12T22:01:46.968Z|00210|ovsdb_server|DBG|HELLO 6 > 2024-01-12T22:01:47.134Z|00211|ovsdb_server|DBG|HELLO 0 > 2024-01-12T22:01:47.134Z|00212|ovsdb_server|DBG|HELLO 1 > 2024-01-12T22:01:47.134Z|00213|ovsdb_server|DBG|HELLO 2 > 2024-01-12T22:01:47.134Z|00214|ovsdb_server|DBG|HELLO 3 > 2024-01-12T22:01:47.134Z|00215|ovsdb_server|DBG|HELLO 4 > 2024-01-12T22:01:47.134Z|00216|ovsdb_server|DBG|HELLO 5 > 2024-01-12T22:01:47.135Z|00217|cooperative_multitasking|DBG|ovsdb/jsonrpc-server.c:604: > yield for raft_run(0x560b47e225e0): elapsed(167) >= threshold(166), > overrun: 1
But this is a debug log, i.e. it doesn't actually fire. The overrun here is just 1ms, which should be fine. Or am I missing something? > > So I actually wonder if the wait timers set up in raft_wait() are too > optimistic? Then again, those timers can't ever take into account the > number of clients serviced by for example the call to > ovsdb_jsonrpc_server_run(). You seem to use 500ms as a RAFT election timer here, that sounds like a small number to use. I'd not recommend anything smaller than a default 1 second. 300ms ping cadence should be fine. You can't predict the total workload due to unknown number of clients, but hundreds of milliseconds is a lot of time for a modern CPU, so should generally be fine. If I understand the log above correctly, It shows 2 iterations of the loop, right? And between them ovsdb-server sleeps for 168-ish ms in the poll_block. Likely because it is a timeout for a ping timer. I we want the yield to not fire when poll_block wakes up on exact time the ping timer should fire (it always will wake up at that time), we may increase the threshold. E.g. add something like 10-100 ms to it. i.e. mkae yilding slightly less frequent than scheduled wakeups for raft_wait. Does that make sense? > > I know that reordering the whole thing is a bit drastic, and I kind of > regret proposing that. > > Would an alternative be to slip in a yield at the top of the main_loop > function or a SHASH_FOR_EACH_SAFE (node, all_dbs) that just does > ovsdb_storage_run()? > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
