[ovs-dev,v2,4/5] json: Add yielding json destroy function.

Destroying JSON objects may be time consuming.

Add json_destroy_with_yield() function that make use of the
cooperative multitasking module to yield during processing,
allowing time sensitive tasks in other parts of the program
to be completed during processing.

We keep this new function private to OVS by adding a new
lib/json.h header file.

The include guard in the public include/openvswitch/json.h is
updated to contain the OPENVSWITCH prefix to be in line with the
other public header files, allowing us to use the non-prefixed
version in our private lib/json.h.

Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
---
 include/openvswitch/json.h | 10 +++++-----
 lib/automake.mk            |  1 +
 lib/json.c                 | 36 +++++++++++++++++++++++++++---------
 lib/json.h                 | 31 +++++++++++++++++++++++++++++++
 4 files changed, 64 insertions(+), 14 deletions(-)
 create mode 100644 lib/json.h

On 1/13/24 00:00, Frode Nordahl wrote:
> Destroying JSON objects may be time consuming.
> 
> Add json_destroy_with_yield() function that make use of the
> cooperative multitasking module to yield during processing,
> allowing time sensitive tasks in other parts of the program
> to be completed during processing.
> 
> We keep this new function private to OVS by adding a new
> lib/json.h header file.
> 
> The include guard in the public include/openvswitch/json.h is
> updated to contain the OPENVSWITCH prefix to be in line with the
> other public header files, allowing us to use the non-prefixed
> version in our private lib/json.h.
> 
> Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
> ---
>  include/openvswitch/json.h | 10 +++++-----
>  lib/automake.mk            |  1 +
>  lib/json.c                 | 36 +++++++++++++++++++++++++++---------
>  lib/json.h                 | 31 +++++++++++++++++++++++++++++++
>  4 files changed, 64 insertions(+), 14 deletions(-)
>  create mode 100644 lib/json.h

This one looks fine.  Though I disagree with your previous statement
that we don't need to yield while serializing. :)

Running 500-node density-heavy with ovn-heater and election timer
set to 1 second I see that while scaling up the cluster, new clients
are connecting, and somewhere at 300 clients the database size grows
enough, so building an initial reply starts to significantly overrun
the yield threshold json_destory_with_yield() in ovsdb_monitor_get_update().
At this point we need to start yielding in json_serialized_object_create()
as well, so the cluster keeps to be stable.

So, I added back your previous code with slight modifications.  And
with that I can scale 500 nodes just fine.  And I see it yielding
a lot there at high scale:

diff --git a/lib/json.c b/lib/json.c
index 44a62ffe3..001f6e6ab 100644
--- a/lib/json.c
+++ b/lib/json.c
@@ -33,6 +33,12 @@
 #include "util.h"
 #include "uuid.h"
 
+/* Non-public JSSF_* flags.  Must not overlap with public ones defined
+ * in include/openvswitch/json.h. */
+enum {
+    JSSF_YIELD = 1 << 7,
+};
+
 /* The type of a JSON token. */
 enum json_token_type {
     T_EOF = 0,
@@ -191,6 +197,14 @@ json_serialized_object_create(const struct json *src)
     return json;
 }
 
+struct json *
+json_serialized_object_create_with_yield(const struct json *src)
+{
+    struct json *json = json_create(JSON_SERIALIZED_OBJECT);
+    json->string = json_to_string(src, JSSF_SORT | JSSF_YIELD);
+    return json;
+}
+
 struct json *
 json_array_create_empty(void)
 {
@@ -1682,6 +1696,10 @@ json_serialize_object(const struct shash *object, struct json_serializer *s)
     s->depth++;
     indent_line(s);
 
+    if (s->flags & JSSF_YIELD) {
+        cooperative_multitasking_yield();
+    }
+
     if (s->flags & JSSF_SORT) {
         const struct shash_node **nodes;
         size_t n, i;
@@ -1715,6 +1733,10 @@ json_serialize_array(const struct json_array *array, struct json_serializer *s)
     ds_put_char(ds, '[');
     s->depth++;
 
+    if (s->flags & JSSF_YIELD) {
+        cooperative_multitasking_yield();
+    }
+
     if (array->n > 0) {
         indent_line(s);
 
diff --git a/lib/json.h b/lib/json.h
index 350943d13..4ad440b39 100644
--- a/lib/json.h
+++ b/lib/json.h
@@ -27,5 +27,6 @@ json_destroy_with_yield(struct json *json)
     }
 }
 
+struct json *json_serialized_object_create_with_yield(const struct json *);
 
 #endif /* JSON_H */
diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
index 718f16db0..c3bfae3d2 100644
--- a/ovsdb/monitor.c
+++ b/ovsdb/monitor.c
@@ -1293,7 +1293,7 @@ ovsdb_monitor_get_update(
                         /* Pre-serializing the object to avoid doing this
                          * for every client. */
                         json_serialized =
-                            json_serialized_object_create(json);
+                            json_serialized_object_create_with_yield(json);
                         json_destroy_with_yield(json);
                         json = json_serialized;
                     }
---

I'm still hitting some overruns at higher scale, but I guess 1 second
is just really not enough there.  My guess is that we spend significant
amount of time in socket I/O...  But that's a story for another time
and likely much more debugging to actually understand what is going on.

For now, I'd suggest to get the changes above.
What do you think?

Best regards, Ilya Maximets.

On Tue, Jan 16, 2024 at 9:44 PM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> On 1/13/24 00:00, Frode Nordahl wrote:
> > Destroying JSON objects may be time consuming.
> >
> > Add json_destroy_with_yield() function that make use of the
> > cooperative multitasking module to yield during processing,
> > allowing time sensitive tasks in other parts of the program
> > to be completed during processing.
> >
> > We keep this new function private to OVS by adding a new
> > lib/json.h header file.
> >
> > The include guard in the public include/openvswitch/json.h is
> > updated to contain the OPENVSWITCH prefix to be in line with the
> > other public header files, allowing us to use the non-prefixed
> > version in our private lib/json.h.
> >
> > Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
> > ---
> >  include/openvswitch/json.h | 10 +++++-----
> >  lib/automake.mk            |  1 +
> >  lib/json.c                 | 36 +++++++++++++++++++++++++++---------
> >  lib/json.h                 | 31 +++++++++++++++++++++++++++++++
> >  4 files changed, 64 insertions(+), 14 deletions(-)
> >  create mode 100644 lib/json.h
>
> This one looks fine.  Though I disagree with your previous statement
> that we don't need to yield while serializing. :)
>
> Running 500-node density-heavy with ovn-heater and election timer
> set to 1 second I see that while scaling up the cluster, new clients
> are connecting, and somewhere at 300 clients the database size grows
> enough, so building an initial reply starts to significantly overrun
> the yield threshold json_destory_with_yield() in ovsdb_monitor_get_update().
> At this point we need to start yielding in json_serialized_object_create()
> as well, so the cluster keeps to be stable.
>
> So, I added back your previous code with slight modifications.  And
> with that I can scale 500 nodes just fine.  And I see it yielding
> a lot there at high scale:
>
> diff --git a/lib/json.c b/lib/json.c
> index 44a62ffe3..001f6e6ab 100644
> --- a/lib/json.c
> +++ b/lib/json.c
> @@ -33,6 +33,12 @@
>  #include "util.h"
>  #include "uuid.h"
>
> +/* Non-public JSSF_* flags.  Must not overlap with public ones defined
> + * in include/openvswitch/json.h. */
> +enum {
> +    JSSF_YIELD = 1 << 7,
> +};
> +
>  /* The type of a JSON token. */
>  enum json_token_type {
>      T_EOF = 0,
> @@ -191,6 +197,14 @@ json_serialized_object_create(const struct json *src)
>      return json;
>  }
>
> +struct json *
> +json_serialized_object_create_with_yield(const struct json *src)
> +{
> +    struct json *json = json_create(JSON_SERIALIZED_OBJECT);
> +    json->string = json_to_string(src, JSSF_SORT | JSSF_YIELD);
> +    return json;
> +}
> +
>  struct json *
>  json_array_create_empty(void)
>  {
> @@ -1682,6 +1696,10 @@ json_serialize_object(const struct shash *object, struct json_serializer *s)
>      s->depth++;
>      indent_line(s);
>
> +    if (s->flags & JSSF_YIELD) {
> +        cooperative_multitasking_yield();
> +    }
> +
>      if (s->flags & JSSF_SORT) {
>          const struct shash_node **nodes;
>          size_t n, i;
> @@ -1715,6 +1733,10 @@ json_serialize_array(const struct json_array *array, struct json_serializer *s)
>      ds_put_char(ds, '[');
>      s->depth++;
>
> +    if (s->flags & JSSF_YIELD) {
> +        cooperative_multitasking_yield();
> +    }
> +
>      if (array->n > 0) {
>          indent_line(s);
>
> diff --git a/lib/json.h b/lib/json.h
> index 350943d13..4ad440b39 100644
> --- a/lib/json.h
> +++ b/lib/json.h
> @@ -27,5 +27,6 @@ json_destroy_with_yield(struct json *json)
>      }
>  }
>
> +struct json *json_serialized_object_create_with_yield(const struct json *);
>
>  #endif /* JSON_H */
> diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
> index 718f16db0..c3bfae3d2 100644
> --- a/ovsdb/monitor.c
> +++ b/ovsdb/monitor.c
> @@ -1293,7 +1293,7 @@ ovsdb_monitor_get_update(
>                          /* Pre-serializing the object to avoid doing this
>                           * for every client. */
>                          json_serialized =
> -                            json_serialized_object_create(json);
> +                            json_serialized_object_create_with_yield(json);
>                          json_destroy_with_yield(json);
>                          json = json_serialized;
>                      }
> ---
>
> I'm still hitting some overruns at higher scale, but I guess 1 second
> is just really not enough there.  My guess is that we spend significant
> amount of time in socket I/O...  But that's a story for another time
> and likely much more debugging to actually understand what is going on.
>
> For now, I'd suggest to get the changes above.
> What do you think?

Yes, I concluded that actually supporting the use of a second or
sub-second election timer would require a different ballpark of effort
than making this work well for reasonably configured clusters.  We
have to start somewhere :)

Thanks for your help in figuring out where to yield in the
json_serialized_object_create() call path, I got a bit lost there in
my initial attempt, I'll incorporate your proposal in v3.