On Mon, Mar 7, 2016 at 3:44 PM, Andy Zhou <az...@ovn.org> 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 <huang.f....@gmail.com> > Reported-at: http://openvswitch.org/pipermail/dev/2016-March/067274.html > Signed-off-by: Andy Zhou <az...@ovn.org> > --- > 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 mest...@mestery.com > Kyle Upton kup...@baymicrosystems.com > Lance Richardson lrich...@redhat.com > Lars Kellogg-Stedman l...@redhat.com > +Lei Huang huang.f....@gmail.com > Leo Alterman lalter...@nicira.com > Lilijun jerry.lili...@huawei.com > Linda Sun l...@vmware.com > 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? Otherwise looks good, and it is verified in the scale test env. > + } > 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 > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev Acked-by: Han Zhou <zhou...@gmail.com> -- Best regards, Han _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev