[ovs-dev] [ovsdb speedup v3 15/19] ovsdb-monitor: add ovsdb_monitor_changes

Ben Pfaff blp at nicira.com
Fri May 29 19:00:46 UTC 2015


On Thu, Apr 09, 2015 at 06:40:24PM -0700, Andy Zhou wrote:
> Currently, each monitor table contains a single hmap 'changes' to
> track updates. This patch introduces a new data structure
> 'ovsdb_monitor_changes' that stores the updates 'rows' tagged by
> its first commit transaction id. Each 'ovsdb_monitor_changes' is
> refenece counted allowing multiple jsonrpc_monitors to share them.
> 
> The next patch will allow each ovsdb monitor table to store a list
> of 'ovsdb_monitor_changes'. This patch stores only one, same as
> before.
> 
> Signed-off-by: Andy Zhou <azhou at nicira.com>
> Acked-by: Ben Pfaff <blp at nicira.com>
> 
> ---
> v1->v2: fix asserts in ovsdb_monitor_table_track_changes()
>         fix a memory leak in ovsdb_monitor_table_add_changes();
> 	remove ovsdb_monitor_renew_tracking_changes(), transaction
> 	number update is now part of ovsdb_monitor_compose_update()
> 
> v2->v3: no change

I'm not sure I understand the sentence beginning 'Generate' in the
following comment.  Do you mean that generating the update when 'n_refs
== 1' will destroy the whole "struct ovsdb_monitor_changes"?  (There's
no object obviously named 'changes'.)

+/* Contains 'struct ovsdb_monitor_row's for rows that have been
+ * updated but not yet flushed to all the jsonrpc connection.
+ *
+ * 'n_refs' represent the number of jsonrpc connections that have
+ * not received updates. Generate the update for the last jsonprc
+ * connection will also remove rows contained in 'changes'.
+ *
+ * 'transaction' stores the first update's transaction id.
+ * */
+struct ovsdb_monitor_changes {
+    struct ovsdb_monitor_table *mt;
+    struct hmap rows;
+    int n_refs;
+    uint64_t transaction;
+};

I'm not sure that the new ovs_assert() in ovsdb_monitor_row_find() is
valuable, because we'll get a segfault anyway in the next line if it
would fail.

I think that the change to ovsdb/jsonrpc-server.c in this patch is only
a stylistic change.  If so, could it be folded into whatever previous
patch previously changed (added?) that code?

It looks like both callers of ovsdb_monitor_changes_destroy_rows()
just afterwards free() the ovsdb_monitor_changes object.  Could
ovsdb_monitor_changes_destroy_rows() be renamed to
ovsdb_monitor_changes_destroy() with that free() call folded in?

I would write ovsdb_monitor_table_untrack_changes() slightly more
compactly, from:
    struct ovsdb_monitor_changes *changes;

    changes = mt->changes;

    if (changes) {
to:
    struct ovsdb_monitor_changes *changes = mt->changes;
    if (changes) {

I notice now that ovsdb_monitor_compose_update() lacks a comment
explaining the 'unflushed' parameter.  It would be nice to add one.

Acked-by: Ben Pfaff <blp at nicira.com>



More information about the dev mailing list