| Message ID | 20211014114614.829400-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ovsdb-idl: Add memory report function. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On Thu, Oct 14, 2021 at 4:46 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > Added new function to return memory usage statistics for database > objects inside IDL. Statistics similar to what ovsdb-server reports. > Not counting _Server database as it should be small, hence doesn't > worth adding extra code to the ovsdb-cs module. Can be added later > if needed. > > ovs-vswitchd is a user in OVS, but this API will be mostly useful for > OVN daemons. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > lib/ovsdb-idl.c | 24 ++++++++++++++++++++++++ > lib/ovsdb-idl.h | 3 +++ > vswitchd/bridge.c | 2 ++ > 3 files changed, 29 insertions(+) > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > index 383a601f6..b22492d5e 100644 > --- a/lib/ovsdb-idl.c > +++ b/lib/ovsdb-idl.c > @@ -42,6 +42,7 @@ > #include "openvswitch/poll-loop.h" > #include "openvswitch/shash.h" > #include "skiplist.h" > +#include "simap.h" > #include "sset.h" > #include "svec.h" > #include "util.h" > @@ -465,6 +466,29 @@ ovsdb_idl_wait(struct ovsdb_idl *idl) > ovsdb_cs_wait(idl->cs); > } > > +/* Returns memory usage statistics. */ > +void > +ovsdb_idl_get_memory_usage(struct ovsdb_idl *idl, struct simap *usage) > +{ > + unsigned int cells = 0; > + > + if (!idl) { > + return; > + } > + > + for (size_t i = 0; i < idl->class_->n_tables; i++) { > + struct ovsdb_idl_table *table = &idl->tables[i]; > + unsigned int n_columns = shash_count(&table->columns); > + unsigned int n_rows = hmap_count(&table->rows); > + > + cells += n_rows * n_columns; > + } > + > + simap_increase(usage, "idl-cells", cells); > + simap_increase(usage, "idl-outstanding-txns", > + hmap_count(&idl->outstanding_txns)); > +} > + > /* Returns a "sequence number" that represents the state of 'idl'. When > * ovsdb_idl_run() changes the database, the sequence number changes. The > * initial fetch of the entire contents of the remote database is considered to > diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h > index 16c8d5f70..d00599616 100644 > --- a/lib/ovsdb-idl.h > +++ b/lib/ovsdb-idl.h > @@ -55,6 +55,7 @@ struct ovsdb_idl_row; > struct ovsdb_idl_column; > struct ovsdb_idl_table; > struct ovsdb_idl_table_class; > +struct simap; > struct uuid; > > struct ovsdb_idl *ovsdb_idl_create(const char *remote, > @@ -73,6 +74,8 @@ void ovsdb_idl_set_leader_only(struct ovsdb_idl *, bool leader_only); > void ovsdb_idl_run(struct ovsdb_idl *); > void ovsdb_idl_wait(struct ovsdb_idl *); > > +void ovsdb_idl_get_memory_usage(struct ovsdb_idl *, struct simap *usage); > + > void ovsdb_idl_set_lock(struct ovsdb_idl *, const char *lock_name); > bool ovsdb_idl_has_lock(const struct ovsdb_idl *); > bool ovsdb_idl_is_lock_contended(const struct ovsdb_idl *); > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index c790a56ad..5223aa897 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -3423,6 +3423,8 @@ bridge_get_memory_usage(struct simap *usage) > HMAP_FOR_EACH (br, node, &all_bridges) { > ofproto_get_memory_usage(br->ofproto, usage); > } > + > + ovsdb_idl_get_memory_usage(idl, usage); > } > > /* QoS unixctl user interface functions. */ > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Hi Ilya, I tested in sandbox: [hanzhou@fedora tutorial]$ ovs-vsctl add-br br-int [hanzhou@fedora tutorial]$ ovs-appctl memory/show handlers:17 ports:1 revalidators:7 rules:5 It didn't show idl-xxx memory for vswitchd. Did I miss something? Thanks, Han
On 10/23/21 08:51, Han Zhou wrote: > > > On Thu, Oct 14, 2021 at 4:46 AM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> wrote: >> >> Added new function to return memory usage statistics for database >> objects inside IDL. Statistics similar to what ovsdb-server reports. >> Not counting _Server database as it should be small, hence doesn't >> worth adding extra code to the ovsdb-cs module. Can be added later >> if needed. >> >> ovs-vswitchd is a user in OVS, but this API will be mostly useful for >> OVN daemons. >> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> >> --- <snip> > > Hi Ilya, I tested in sandbox: > > [hanzhou@fedora tutorial]$ ovs-vsctl add-br br-int > [hanzhou@fedora tutorial]$ ovs-appctl memory/show > handlers:17 ports:1 revalidators:7 rules:5 > > It didn't show idl-xxx memory for vswitchd. Did I miss something? Weird. Are you sure that patch was correctly applied? On my machine: [ovs]# wget -c https://patchwork.ozlabs.org/project/openvswitch/patch/20211014114614.829400-1-i.maximets@ovn.org/mbox/ --content-disposition [ovs]# git am -3 ovs-dev-ovsdb-idl-Add-memory-report-function..patch [ovs]# make -j8 sandbox [tutorial]# ovs-appctl memory/show idl-cells:17 [tutorial]# ovs-vsctl add-br br-int [tutorial]# ovs-appctl memory/show handlers:11 idl-cells:96 ports:1 revalidators:5 rules:5 [tutorial]# ovs-vsctl del-br br-int [tutorial]# ovs-appctl memory/show idl-cells:17 Best regards, Ilya Maximets.
On Mon, Oct 25, 2021 at 5:39 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 10/23/21 08:51, Han Zhou wrote: > > > > > > On Thu, Oct 14, 2021 at 4:46 AM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> wrote: > >> > >> Added new function to return memory usage statistics for database > >> objects inside IDL. Statistics similar to what ovsdb-server reports. > >> Not counting _Server database as it should be small, hence doesn't > >> worth adding extra code to the ovsdb-cs module. Can be added later > >> if needed. > >> > >> ovs-vswitchd is a user in OVS, but this API will be mostly useful for > >> OVN daemons. > >> > >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org <mailto: i.maximets@ovn.org>> > >> --- > > <snip> > > > > > Hi Ilya, I tested in sandbox: > > > > [hanzhou@fedora tutorial]$ ovs-vsctl add-br br-int > > [hanzhou@fedora tutorial]$ ovs-appctl memory/show > > handlers:17 ports:1 revalidators:7 rules:5 > > > > It didn't show idl-xxx memory for vswitchd. Did I miss something? > > Weird. Are you sure that patch was correctly applied? My bad! I forgot that this patch is not in a series :P Acked-by: Han Zhou <hzhou@ovn.org> > > On my machine: > > [ovs]# wget -c https://patchwork.ozlabs.org/project/openvswitch/patch/20211014114614.829400-1-i.maximets@ovn.org/mbox/ --content-disposition > [ovs]# git am -3 ovs-dev-ovsdb-idl-Add-memory-report-function..patch > [ovs]# make -j8 sandbox > [tutorial]# ovs-appctl memory/show > idl-cells:17 > [tutorial]# ovs-vsctl add-br br-int > [tutorial]# ovs-appctl memory/show > handlers:11 idl-cells:96 ports:1 revalidators:5 rules:5 > [tutorial]# ovs-vsctl del-br br-int > [tutorial]# ovs-appctl memory/show > idl-cells:17 > > Best regards, Ilya Maximets.
On 10/14/21 1:46 PM, Ilya Maximets wrote: > Added new function to return memory usage statistics for database > objects inside IDL. Statistics similar to what ovsdb-server reports. > Not counting _Server database as it should be small, hence doesn't > worth adding extra code to the ovsdb-cs module. Can be added later > if needed. > > ovs-vswitchd is a user in OVS, but this API will be mostly useful for > OVN daemons. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > lib/ovsdb-idl.c | 24 ++++++++++++++++++++++++ > lib/ovsdb-idl.h | 3 +++ > vswitchd/bridge.c | 2 ++ > 3 files changed, 29 insertions(+) > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > index 383a601f6..b22492d5e 100644 > --- a/lib/ovsdb-idl.c > +++ b/lib/ovsdb-idl.c > @@ -42,6 +42,7 @@ > #include "openvswitch/poll-loop.h" > #include "openvswitch/shash.h" > #include "skiplist.h" > +#include "simap.h" > #include "sset.h" > #include "svec.h" > #include "util.h" > @@ -465,6 +466,29 @@ ovsdb_idl_wait(struct ovsdb_idl *idl) > ovsdb_cs_wait(idl->cs); > } > > +/* Returns memory usage statistics. */ > +void > +ovsdb_idl_get_memory_usage(struct ovsdb_idl *idl, struct simap *usage) > +{ > + unsigned int cells = 0; > + > + if (!idl) { > + return; > + } > + > + for (size_t i = 0; i < idl->class_->n_tables; i++) { > + struct ovsdb_idl_table *table = &idl->tables[i]; > + unsigned int n_columns = shash_count(&table->columns); Nit: even though this is also constant time we already have the number of columns stored in the table class, we could instead do: unsigned int n_columns = table->class_->n_columns; I guess this can be fixed up at apply time, therefore: Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks, Dumitru
On 11/2/21 20:50, Dumitru Ceara wrote: > On 10/14/21 1:46 PM, Ilya Maximets wrote: >> Added new function to return memory usage statistics for database >> objects inside IDL. Statistics similar to what ovsdb-server reports. >> Not counting _Server database as it should be small, hence doesn't >> worth adding extra code to the ovsdb-cs module. Can be added later >> if needed. >> >> ovs-vswitchd is a user in OVS, but this API will be mostly useful for >> OVN daemons. >> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- >> lib/ovsdb-idl.c | 24 ++++++++++++++++++++++++ >> lib/ovsdb-idl.h | 3 +++ >> vswitchd/bridge.c | 2 ++ >> 3 files changed, 29 insertions(+) >> >> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c >> index 383a601f6..b22492d5e 100644 >> --- a/lib/ovsdb-idl.c >> +++ b/lib/ovsdb-idl.c >> @@ -42,6 +42,7 @@ >> #include "openvswitch/poll-loop.h" >> #include "openvswitch/shash.h" >> #include "skiplist.h" >> +#include "simap.h" >> #include "sset.h" >> #include "svec.h" >> #include "util.h" >> @@ -465,6 +466,29 @@ ovsdb_idl_wait(struct ovsdb_idl *idl) >> ovsdb_cs_wait(idl->cs); >> } >> >> +/* Returns memory usage statistics. */ >> +void >> +ovsdb_idl_get_memory_usage(struct ovsdb_idl *idl, struct simap *usage) >> +{ >> + unsigned int cells = 0; >> + >> + if (!idl) { >> + return; >> + } >> + >> + for (size_t i = 0; i < idl->class_->n_tables; i++) { >> + struct ovsdb_idl_table *table = &idl->tables[i]; >> + unsigned int n_columns = shash_count(&table->columns); > > Nit: even though this is also constant time we already have the number > of columns stored in the table class, we could instead do: > > unsigned int n_columns = table->class_->n_columns; > > I guess this can be fixed up at apply time, therefore: > > Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks, Han and Dumitru! I made the above change and applied the patch. Best regards, Ilya Maximets.
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index 383a601f6..b22492d5e 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -42,6 +42,7 @@ #include "openvswitch/poll-loop.h" #include "openvswitch/shash.h" #include "skiplist.h" +#include "simap.h" #include "sset.h" #include "svec.h" #include "util.h" @@ -465,6 +466,29 @@ ovsdb_idl_wait(struct ovsdb_idl *idl) ovsdb_cs_wait(idl->cs); } +/* Returns memory usage statistics. */ +void +ovsdb_idl_get_memory_usage(struct ovsdb_idl *idl, struct simap *usage) +{ + unsigned int cells = 0; + + if (!idl) { + return; + } + + for (size_t i = 0; i < idl->class_->n_tables; i++) { + struct ovsdb_idl_table *table = &idl->tables[i]; + unsigned int n_columns = shash_count(&table->columns); + unsigned int n_rows = hmap_count(&table->rows); + + cells += n_rows * n_columns; + } + + simap_increase(usage, "idl-cells", cells); + simap_increase(usage, "idl-outstanding-txns", + hmap_count(&idl->outstanding_txns)); +} + /* Returns a "sequence number" that represents the state of 'idl'. When * ovsdb_idl_run() changes the database, the sequence number changes. The * initial fetch of the entire contents of the remote database is considered to diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h index 16c8d5f70..d00599616 100644 --- a/lib/ovsdb-idl.h +++ b/lib/ovsdb-idl.h @@ -55,6 +55,7 @@ struct ovsdb_idl_row; struct ovsdb_idl_column; struct ovsdb_idl_table; struct ovsdb_idl_table_class; +struct simap; struct uuid; struct ovsdb_idl *ovsdb_idl_create(const char *remote, @@ -73,6 +74,8 @@ void ovsdb_idl_set_leader_only(struct ovsdb_idl *, bool leader_only); void ovsdb_idl_run(struct ovsdb_idl *); void ovsdb_idl_wait(struct ovsdb_idl *); +void ovsdb_idl_get_memory_usage(struct ovsdb_idl *, struct simap *usage); + void ovsdb_idl_set_lock(struct ovsdb_idl *, const char *lock_name); bool ovsdb_idl_has_lock(const struct ovsdb_idl *); bool ovsdb_idl_is_lock_contended(const struct ovsdb_idl *); diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index c790a56ad..5223aa897 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -3423,6 +3423,8 @@ bridge_get_memory_usage(struct simap *usage) HMAP_FOR_EACH (br, node, &all_bridges) { ofproto_get_memory_usage(br->ofproto, usage); } + + ovsdb_idl_get_memory_usage(idl, usage); } /* QoS unixctl user interface functions. */
Added new function to return memory usage statistics for database objects inside IDL. Statistics similar to what ovsdb-server reports. Not counting _Server database as it should be small, hence doesn't worth adding extra code to the ovsdb-cs module. Can be added later if needed. ovs-vswitchd is a user in OVS, but this API will be mostly useful for OVN daemons. Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- lib/ovsdb-idl.c | 24 ++++++++++++++++++++++++ lib/ovsdb-idl.h | 3 +++ vswitchd/bridge.c | 2 ++ 3 files changed, 29 insertions(+)