On Fri, Jan 12, 2024 at 1:40 AM Ilya Maximets <[email protected]> wrote:
>
> 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.
Ack.
> >
> >
> > 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.
Ack.
> > 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.
Ack.
> > 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.
I checked logs and it had not fired here for successful runs, so I'll
drop it. Thx!
> > 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.
Thanks!
> > @@ -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.
Ack.
> > 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.
Ack.
> > if (t->created < LLONG_MAX - t->timeout_msec) {
> > long long int t_deadline = t->created + t->timeout_msec;
> > if (deadline > t_deadline) {
>
--
Frode Nordahl
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
