On 1/10/24 20:29, Frode Nordahl wrote: > Initialize the cooperative multitasking module for the > ovsdb-server. > > The server side schema conversion process used for storage > engines such as RAFT is time consuming, yield during > processing. > > After the schema conversion is done, the processing of JSON-RPC > sessions and OVSDB monitors for reconnecting clients can > overrun the configured election timer. > > The JSON serialization of the database contents has been > identified as one of the primary offenders. Make use of > yielding versions of JSON object create and destroy functions > to mitigate. > > This series has been tested by checking success of schema > conversion, ensuring no involuntary leader change occurs with > election timer configurations as low as 750 msec, on a 75MB > database with ~ 100 connected clients as produced by the > ovn-heater ocp-120-density-light test scenario.
Nice! > > Signed-off-by: Frode Nordahl <[email protected]> > --- > NEWS | 8 ++++++++ > ovsdb/file.c | 2 ++ > ovsdb/jsonrpc-server.c | 2 ++ > ovsdb/monitor.c | 12 ++++++++---- > ovsdb/ovsdb-server.c | 5 +++++ > ovsdb/trigger.c | 3 +++ > 6 files changed, 28 insertions(+), 4 deletions(-) > > diff --git a/NEWS b/NEWS > index 270ed6673..643fddc98 100644 > --- a/NEWS > +++ b/NEWS > @@ -6,6 +6,9 @@ Post-v3.2.0 > from older version is supported but it may trigger more leader > elections > during the process, and error logs complaining unrecognized fields may > be observed on old nodes. > + * Make use of cooperative multitasking to ensure stable maintenance of > + RAFT cluster during long running processing such as online schema > + conversion. > - OpenFlow: > * NXT_CT_FLUSH extension is updated to support flushing connections > based on mark and labels. 'ct-flush' command of ovs-ofctl updated > @@ -36,6 +39,11 @@ Post-v3.2.0 > The existing behaviour is maintained and a non key:value pair value > will be applied to all other PMD thread cores.'pmd-sleep-show' is > updated to show the maximum sleep for each PMD thread core. > + - lib: > + * Introduce cooperative multitasking module which allow us to interleave > + important processing with long running tasks while avoiding the > + additional resource consumption of threads and complexity of > + asynchronous state machines. NEWS are mostly for user-visible changes. They don't need to know about a new internal library. Entry in the OVSDB section is enough. > > > v3.2.0 - 17 Aug 2023 > diff --git a/ovsdb/file.c b/ovsdb/file.c > index 8bd1d4af3..fb4cbfb77 100644 > --- a/ovsdb/file.c > +++ b/ovsdb/file.c > @@ -23,6 +23,7 @@ > > #include "bitmap.h" > #include "column.h" > +#include "cooperative-multitasking.h" > #include "log.h" > #include "openvswitch/json.h" > #include "lockfile.h" > @@ -308,6 +309,7 @@ ovsdb_convert_table(struct ovsdb_txn *txn, > } > > HMAP_FOR_EACH (src_row, hmap_node, &src_table->rows) { > + cooperative_multitasking_yield(); Please, move this after the empty line below variable declaration. > struct ovsdb_row *dst_row = ovsdb_row_create(dst_table); > *ovsdb_row_get_uuid_rw(dst_row) = *ovsdb_row_get_uuid(src_row); > > diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c > index 45f7c8038..f08b5a946 100644 > --- a/ovsdb/jsonrpc-server.c > +++ b/ovsdb/jsonrpc-server.c > @@ -21,6 +21,7 @@ > > #include "bitmap.h" > #include "column.h" > +#include "cooperative-multitasking.h" > #include "openvswitch/dynamic-string.h" > #include "monitor.h" > #include "openvswitch/json.h" > @@ -600,6 +601,7 @@ ovsdb_jsonrpc_session_run_all(struct ovsdb_jsonrpc_remote > *remote) > struct ovsdb_jsonrpc_session *s; > > LIST_FOR_EACH_SAFE (s, node, &remote->sessions) { > + cooperative_multitasking_yield(); An empty line here would be nice. > int error = ovsdb_jsonrpc_session_run(s); > if (error) { > ovsdb_jsonrpc_session_close(s); > diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c > index d1e466faa..93cc66b76 100644 > --- a/ovsdb/monitor.c > +++ b/ovsdb/monitor.c > @@ -20,6 +20,7 @@ > > #include "bitmap.h" > #include "column.h" > +#include "cooperative-multitasking.h" > #include "openvswitch/dynamic-string.h" > #include "openvswitch/json.h" > #include "jsonrpc.h" > @@ -262,7 +263,7 @@ ovsdb_monitor_json_cache_flush(struct ovsdb_monitor > *dbmon) > struct ovsdb_monitor_json_cache_node *node; > > HMAP_FOR_EACH_POP(node, hmap_node, &dbmon->json_cache) { > - json_destroy(node->json); > + json_destroy_with_yield(node->json); > free(node); > } > } > @@ -278,7 +279,7 @@ ovsdb_monitor_json_cache_destroy(struct ovsdb_monitor > *dbmon, > = ovsdb_monitor_json_cache_search(dbmon, v, change_set); > if (node) { > hmap_remove(&dbmon->json_cache, &node->hmap_node); > - json_destroy(node->json); > + json_destroy_with_yield(node->json); > free(node); > } > } > @@ -576,6 +577,7 @@ ovsdb_monitor_add_change_set(struct ovsdb_monitor *dbmon, > > struct shash_node *node; > SHASH_FOR_EACH (node, &dbmon->tables) { > + cooperative_multitasking_yield(); Is this needed? I don't see any heavy operations in this loop and the number of tables is usually not that large. > struct ovsdb_monitor_table *mt = node->data; > if (!init_only || (mt->select & OJMS_INITIAL)) { > struct ovsdb_monitor_change_set_for_table *mcst = > @@ -1172,6 +1174,7 @@ ovsdb_monitor_compose_update( > struct ovsdb_monitor_table *mt = mcst->mt; > > HMAP_FOR_EACH_SAFE (row, hmap_node, &mcst->rows) { > + cooperative_multitasking_yield(); > struct json *row_json; > row_json = (*row_update)(mt, condition, OVSDB_MONITOR_ROW, row, > initial, changed, mcst->n_columns); There should be yield in ovsdb_monitor_compose_cond_change_update() as well. > @@ -1286,8 +1289,9 @@ ovsdb_monitor_get_update( > > /* Pre-serializing the object to avoid doing this > * for every client. */ > - json_serialized = > json_serialized_object_create(json); > - json_destroy(json); > + json_serialized = > + json_serialized_object_create_with_yield(json); > + json_destroy_with_yield(json); > json = json_serialized; > } > ovsdb_monitor_json_cache_insert(dbmon, version, mcs, > diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c > index e1d4b1125..84c4834dc 100644 > --- a/ovsdb/ovsdb-server.c > +++ b/ovsdb/ovsdb-server.c > @@ -24,6 +24,7 @@ > > #include "column.h" > #include "command-line.h" > +#include "cooperative-multitasking.h" > #include "daemon.h" > #include "dirs.h" > #include "dns-resolve.h" > @@ -65,6 +66,8 @@ > > VLOG_DEFINE_THIS_MODULE(ovsdb_server); > > +static struct hmap cooperative_multitasking_callbacks; > + > struct db { > char *filename; > struct ovsdb *db; > @@ -340,6 +343,7 @@ main(int argc, char *argv[]) > fatal_ignore_sigpipe(); > process_init(); > dns_resolve_init(true); > + cooperative_multitasking_init(&cooperative_multitasking_callbacks); > > bool active = false; > parse_options(argc, argv, &db_filenames, &remotes, &unixctl_path, > @@ -532,6 +536,7 @@ main(int argc, char *argv[]) > } > dns_resolve_destroy(); > perf_counters_destroy(); > + cooperative_multitasking_destroy(); > service_stop(); > return 0; > } > diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c > index 2a48ccc64..79360d359 100644 > --- a/ovsdb/trigger.c > +++ b/ovsdb/trigger.c > @@ -20,6 +20,7 @@ > #include <limits.h> > #include <string.h> > > +#include "cooperative-multitasking.h" > #include "file.h" > #include "openvswitch/json.h" > #include "jsonrpc.h" > @@ -181,6 +182,7 @@ ovsdb_trigger_run(struct ovsdb *db, long long int now) > bool disconnect_all = false; > > LIST_FOR_EACH_SAFE (t, node, &db->triggers) { > + cooperative_multitasking_yield(); An empty line would be nice. > if (run_triggers > || now - t->created >= t->timeout_msec > || t->progress || t->txn_forward) { > @@ -202,6 +204,7 @@ ovsdb_trigger_wait(struct ovsdb *db, long long int now) > struct ovsdb_trigger *t; > > LIST_FOR_EACH (t, node, &db->triggers) { > + cooperative_multitasking_yield(); Waiting is not a heavy operation. And we shouldn't have millions of active triggers. So, this yield is likely unnecessary. > if (t->created < LLONG_MAX - t->timeout_msec) { > long long int t_deadline = t->created + t->timeout_msec; > if (deadline > t_deadline) { _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
