On Mon, Mar 7, 2016 at 5:26 PM, Han Zhou <[email protected]> wrote: > > > On Mon, Mar 7, 2016 at 3:44 PM, Andy Zhou <[email protected]> wrote: > > > > When destroying an ovsdb_jsonrpc_monitor, the jsonrpc monitor still > > holds a reference count to the monitors 'changes' indexed with > > 'unflushed' transaction id. The bug is that the reference count was > > not decremented as it should in the code path. > > > > The bug caused 'changes' that have been flushed to all jsonrpc > > clients to linger around unnecessarily, occupying increasingly > > large amount of memory. See "Reported-at" URL for more details. > > > > This bug is tricky to find since the memory is not leaked; they will > > eventually be freed when monitors are destroyed. > > > > Reported-by: Lei Huang <[email protected]> > > Reported-at: http://openvswitch.org/pipermail/dev/2016-March/067274.html > > Signed-off-by: Andy Zhou <[email protected]> > > --- > > AUTHORS | 1 + > > ovsdb/jsonrpc-server.c | 4 ++-- > > ovsdb/monitor.c | 9 ++++++++- > > ovsdb/monitor.h | 6 ++---- > > 4 files changed, 13 insertions(+), 7 deletions(-) > > > > diff --git a/AUTHORS b/AUTHORS > > index 1184a51..0ba0f58 100644 > > --- a/AUTHORS > > +++ b/AUTHORS > > @@ -119,6 +119,7 @@ Kyle Mestery [email protected] > > Kyle Upton [email protected] > > Lance Richardson [email protected] > > Lars Kellogg-Stedman [email protected] > > +Lei Huang [email protected] > > Leo Alterman [email protected] > > Lilijun [email protected] > > Linda Sun [email protected] > > diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c > > index 15dbc4e..caaf2bf 100644 > > --- a/ovsdb/jsonrpc-server.c > > +++ b/ovsdb/jsonrpc-server.c > > @@ -1265,7 +1265,7 @@ ovsdb_jsonrpc_monitor_create(struct > ovsdb_jsonrpc_session *s, struct ovsdb *db, > > dbmon = ovsdb_monitor_add(m->dbmon); > > if (dbmon != m->dbmon) { > > /* Found an exisiting dbmon, reuse the current one. */ > > - ovsdb_monitor_remove_jsonrpc_monitor(m->dbmon, m); > > + ovsdb_monitor_remove_jsonrpc_monitor(m->dbmon, m, m->unflushed); > > ovsdb_monitor_add_jsonrpc_monitor(dbmon, m); > > m->dbmon = dbmon; > > } > > @@ -1348,7 +1348,7 @@ ovsdb_jsonrpc_monitor_destroy(struct > ovsdb_jsonrpc_monitor *m) > > { > > json_destroy(m->monitor_id); > > hmap_remove(&m->session->monitors, &m->node); > > - ovsdb_monitor_remove_jsonrpc_monitor(m->dbmon, m); > > + ovsdb_monitor_remove_jsonrpc_monitor(m->dbmon, m, m->unflushed); > > free(m); > > } > > > > diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c > > index 0d81d89..6b0d13d 100644 > > --- a/ovsdb/monitor.c > > +++ b/ovsdb/monitor.c > > @@ -959,7 +959,8 @@ ovsdb_monitor_get_initial(const struct ovsdb_monitor > *dbmon) > > > > void > > ovsdb_monitor_remove_jsonrpc_monitor(struct ovsdb_monitor *dbmon, > > - struct ovsdb_jsonrpc_monitor *jsonrpc_monitor) > > + struct ovsdb_jsonrpc_monitor *jsonrpc_monitor, > > + uint64_t unflushed) > > { > > struct jsonrpc_monitor_node *jm; > > > > @@ -971,6 +972,12 @@ ovsdb_monitor_remove_jsonrpc_monitor(struct > ovsdb_monitor *dbmon, > > /* Find and remove the jsonrpc monitor from the list. */ > > LIST_FOR_EACH(jm, node, &dbmon->jsonrpc_monitors) { > > if (jm->jsonrpc_monitor == jsonrpc_monitor) { > > + /* Release the tracked changes. */ > > + struct shash_node *node; > > + SHASH_FOR_EACH (node, &dbmon->tables) { > > + struct ovsdb_monitor_table *mt = node->data; > > + ovsdb_monitor_table_untrack_changes(mt, unflushed); > > Just a question, why passing in "unflushed" as a parameter instead of > using jsonrpc_monitor->unflushed directly? > Because ovsdb_jsonrpc_monitor struct is opaque to monitor.c.
> > Otherwise looks good, and it is verified in the scale test env. > Thanks, I have added your name to Test-by: Really appreciate the help. > > > + } > > list_remove(&jm->node); > > free(jm); > > > > diff --git a/ovsdb/monitor.h b/ovsdb/monitor.h > > index fb10435..9fea831 100644 > > --- a/ovsdb/monitor.h > > +++ b/ovsdb/monitor.h > > @@ -46,10 +46,8 @@ void ovsdb_monitor_add_jsonrpc_monitor(struct > ovsdb_monitor *dbmon, > > struct ovsdb_jsonrpc_monitor *jsonrpc_monitor); > > > > void ovsdb_monitor_remove_jsonrpc_monitor(struct ovsdb_monitor *dbmon, > > - struct ovsdb_jsonrpc_monitor *jsonrpc_monitor); > > - > > -void ovsdb_monitor_remove_jsonrpc_monitor(struct ovsdb_monitor *dbmon, > > - struct ovsdb_jsonrpc_monitor > *jsonrpc_monitor); > > + struct ovsdb_jsonrpc_monitor > *jsonrpc_monitor, > > + uint64_t unflushed); > > > > void ovsdb_monitor_add_table(struct ovsdb_monitor *m, > > const struct ovsdb_table *table); > > -- > > 1.9.1 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > http://openvswitch.org/mailman/listinfo/dev > > Acked-by: Han Zhou <[email protected]> > Thanks to Liran and Han for reviewing. Pushed to master, branch-2.4 and branch-2.5. > > -- > Best regards, > Han > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
