| Message ID | 20221125123704.646476-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Commit | e83dad6e53f3fe04ca9c4d6972fcaa7995de2ba2 |
| Headers | show |
| Series | [ovs-dev] ovsdb: Count weak reference objects. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
On 11/25/22 13:37, Ilya Maximets wrote: > OVSDB creates a separate object for each weak reference in order to > track them and there could be a significant amount of these objects > in the database. > > We also had problems with number of these objects growing out of > bounds recently. So, adding them to a memory report seems to be > a good thing. > > Counting them globally to cover all the copied instances in transactions > and the transaction history (even though there should be none). > It's also hard to count them per-database, because weak references > are stored on destination rows and can be destroyed either while > destroying the destination row or while removing the reference from > the source row. Also, not all the involved functions have direct > access to the database object. So, there is no single clear place > where counters should be updated. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks, Dumitru
On Fri, Nov 25, 2022 at 4:36 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > OVSDB creates a separate object for each weak reference in order to > track them and there could be a significant amount of these objects > in the database. > > We also had problems with number of these objects growing out of > bounds recently. So, adding them to a memory report seems to be > a good thing. > > Counting them globally to cover all the copied instances in transactions > and the transaction history (even though there should be none). > It's also hard to count them per-database, because weak references > are stored on destination rows and can be destroyed either while > destroying the destination row or while removing the reference from > the source row. Also, not all the involved functions have direct > access to the database object. So, there is no single clear place > where counters should be updated. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > ovsdb/ovsdb.c | 4 ++++ > ovsdb/ovsdb.h | 4 ++++ > ovsdb/row.c | 5 ++++- > ovsdb/transaction.c | 2 ++ > 4 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c > index 1c011fab0..11786f376 100644 > --- a/ovsdb/ovsdb.c > +++ b/ovsdb/ovsdb.c > @@ -43,6 +43,8 @@ > #include "openvswitch/vlog.h" > VLOG_DEFINE_THIS_MODULE(ovsdb); > > +size_t n_weak_refs = 0; > + > struct ovsdb_schema * > ovsdb_schema_create(const char *name, const char *version, const char *cksum) > { > @@ -546,6 +548,8 @@ ovsdb_get_memory_usage(const struct ovsdb *db, struct simap *usage) > if (db->storage) { > ovsdb_storage_get_memory_usage(db->storage, usage); > } > + > + simap_put(usage, "n-weak-refs", n_weak_refs); > } > > struct ovsdb_table * > diff --git a/ovsdb/ovsdb.h b/ovsdb/ovsdb.h > index d05e7c64a..13d8bf407 100644 > --- a/ovsdb/ovsdb.h > +++ b/ovsdb/ovsdb.h > @@ -125,6 +125,10 @@ struct ovsdb { > struct ovsdb_compaction_state *snap_state; > }; > > +/* Total number of 'weak reference' objects in all databases > + * and transactions. */ > +extern size_t n_weak_refs; > + > struct ovsdb *ovsdb_create(struct ovsdb_schema *, struct ovsdb_storage *); > void ovsdb_destroy(struct ovsdb *); > > diff --git a/ovsdb/row.c b/ovsdb/row.c > index 3f0bb8acf..d7bfbdd36 100644 > --- a/ovsdb/row.c > +++ b/ovsdb/row.c > @@ -21,8 +21,9 @@ > > #include "openvswitch/dynamic-string.h" > #include "openvswitch/json.h" > -#include "ovsdb-error.h" > #include "openvswitch/shash.h" > +#include "ovsdb-error.h" > +#include "ovsdb.h" > #include "sort.h" > #include "table.h" > #include "util.h" > @@ -78,6 +79,7 @@ ovsdb_weak_ref_clone(struct ovsdb_weak_ref *src) > ovsdb_type_clone(&weak->type, &src->type); > weak->column_idx = src->column_idx; > weak->by_key = src->by_key; > + n_weak_refs++; > return weak; > } > > @@ -130,6 +132,7 @@ ovsdb_weak_ref_destroy(struct ovsdb_weak_ref *weak) > } > ovsdb_type_destroy(&weak->type); > free(weak); > + n_weak_refs--; > } > > struct ovsdb_row * > diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c > index 5d7c70a51..03541af85 100644 > --- a/ovsdb/transaction.c > +++ b/ovsdb/transaction.c > @@ -613,6 +613,8 @@ add_weak_ref(const struct ovsdb_row *src, const struct ovsdb_row *dst_, > weak->column_idx = column->index; > hmap_node_nullify(&weak->dst_node); > ovs_list_push_back(ref_list, &weak->src_node); > + > + n_weak_refs++; > } > > static void > -- > 2.38.1 > Acked-by: Han Zhou <hzhou@ovn.org>
On 12/5/22 07:23, Han Zhou wrote: > > > On Fri, Nov 25, 2022 at 4:36 AM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> wrote: >> >> OVSDB creates a separate object for each weak reference in order to >> track them and there could be a significant amount of these objects >> in the database. >> >> We also had problems with number of these objects growing out of >> bounds recently. So, adding them to a memory report seems to be >> a good thing. >> >> Counting them globally to cover all the copied instances in transactions >> and the transaction history (even though there should be none). >> It's also hard to count them per-database, because weak references >> are stored on destination rows and can be destroyed either while >> destroying the destination row or while removing the reference from >> the source row. Also, not all the involved functions have direct >> access to the database object. So, there is no single clear place >> where counters should be updated. >> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> >> --- >> ovsdb/ovsdb.c | 4 ++++ >> ovsdb/ovsdb.h | 4 ++++ >> ovsdb/row.c | 5 ++++- >> ovsdb/transaction.c | 2 ++ >> 4 files changed, 14 insertions(+), 1 deletion(-) >> > Acked-by: Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>> Applied. Thanks! Best regards, Ilya Maximets.
diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c index 1c011fab0..11786f376 100644 --- a/ovsdb/ovsdb.c +++ b/ovsdb/ovsdb.c @@ -43,6 +43,8 @@ #include "openvswitch/vlog.h" VLOG_DEFINE_THIS_MODULE(ovsdb); +size_t n_weak_refs = 0; + struct ovsdb_schema * ovsdb_schema_create(const char *name, const char *version, const char *cksum) { @@ -546,6 +548,8 @@ ovsdb_get_memory_usage(const struct ovsdb *db, struct simap *usage) if (db->storage) { ovsdb_storage_get_memory_usage(db->storage, usage); } + + simap_put(usage, "n-weak-refs", n_weak_refs); } struct ovsdb_table * diff --git a/ovsdb/ovsdb.h b/ovsdb/ovsdb.h index d05e7c64a..13d8bf407 100644 --- a/ovsdb/ovsdb.h +++ b/ovsdb/ovsdb.h @@ -125,6 +125,10 @@ struct ovsdb { struct ovsdb_compaction_state *snap_state; }; +/* Total number of 'weak reference' objects in all databases + * and transactions. */ +extern size_t n_weak_refs; + struct ovsdb *ovsdb_create(struct ovsdb_schema *, struct ovsdb_storage *); void ovsdb_destroy(struct ovsdb *); diff --git a/ovsdb/row.c b/ovsdb/row.c index 3f0bb8acf..d7bfbdd36 100644 --- a/ovsdb/row.c +++ b/ovsdb/row.c @@ -21,8 +21,9 @@ #include "openvswitch/dynamic-string.h" #include "openvswitch/json.h" -#include "ovsdb-error.h" #include "openvswitch/shash.h" +#include "ovsdb-error.h" +#include "ovsdb.h" #include "sort.h" #include "table.h" #include "util.h" @@ -78,6 +79,7 @@ ovsdb_weak_ref_clone(struct ovsdb_weak_ref *src) ovsdb_type_clone(&weak->type, &src->type); weak->column_idx = src->column_idx; weak->by_key = src->by_key; + n_weak_refs++; return weak; } @@ -130,6 +132,7 @@ ovsdb_weak_ref_destroy(struct ovsdb_weak_ref *weak) } ovsdb_type_destroy(&weak->type); free(weak); + n_weak_refs--; } struct ovsdb_row * diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c index 5d7c70a51..03541af85 100644 --- a/ovsdb/transaction.c +++ b/ovsdb/transaction.c @@ -613,6 +613,8 @@ add_weak_ref(const struct ovsdb_row *src, const struct ovsdb_row *dst_, weak->column_idx = column->index; hmap_node_nullify(&weak->dst_node); ovs_list_push_back(ref_list, &weak->src_node); + + n_weak_refs++; } static void
OVSDB creates a separate object for each weak reference in order to track them and there could be a significant amount of these objects in the database. We also had problems with number of these objects growing out of bounds recently. So, adding them to a memory report seems to be a good thing. Counting them globally to cover all the copied instances in transactions and the transaction history (even though there should be none). It's also hard to count them per-database, because weak references are stored on destination rows and can be destroyed either while destroying the destination row or while removing the reference from the source row. Also, not all the involved functions have direct access to the database object. So, there is no single clear place where counters should be updated. Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- ovsdb/ovsdb.c | 4 ++++ ovsdb/ovsdb.h | 4 ++++ ovsdb/row.c | 5 ++++- ovsdb/transaction.c | 2 ++ 4 files changed, 14 insertions(+), 1 deletion(-)