On 01/18/2018 10:58 AM, Aaron Conole wrote:
Mark Michelson <[email protected]> writes:
This modifies ovn-controller to measure the amount of time it takes to
detect a change in the southbound database, generate the resulting flow
table, and write the flow table down to vswitchd.
The statistics can be queried using:
ovs-appctl -t ovn-controller performance/show ovn-controller-loop
Signed-off-by: Mark Michelson <[email protected]>
---
ovn/controller/ovn-controller.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index c286ccbca..f51d5660f 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -56,6 +56,9 @@
#include "stream.h"
#include "unixctl.h"
#include "util.h"
+#include "timeval.h"
+#include "timer.h"
+#include "performance.h"
VLOG_DEFINE_THIS_MODULE(main);
@@ -66,6 +69,8 @@ static unixctl_cb_func inject_pkt;
#define DEFAULT_BRIDGE_NAME "br-int"
#define DEFAULT_PROBE_INTERVAL_MSEC 5000
+#define CONTROLLER_LOOP_PERFORMANCE_NAME "ovn-controller-loop"
+
static void update_probe_interval(struct controller_ctx *,
const char *ovnsb_remote);
static void parse_options(int argc, char *argv[]);
@@ -637,8 +642,10 @@ main(int argc, char *argv[])
unixctl_command_register("inject-pkt", "MICROFLOW", 1, 1, inject_pkt,
&pending_pkt);
+ performance_create(CONTROLLER_LOOP_PERFORMANCE_NAME);
/* Main loop. */
exiting = false;
+ unsigned int our_seqno = 0;
while (!exiting) {
/* Check OVN SB database. */
char *new_ovnsb_remote = get_ovnsb_remote(ovs_idl_loop.idl);
@@ -657,6 +664,11 @@ main(int argc, char *argv[])
.ovnsb_idl_txn = ovsdb_idl_loop_run(&ovnsb_idl_loop),
};
+ if (our_seqno != ovsdb_idl_get_seqno(ctx.ovnsb_idl) &&
+ performance_start_sample(CONTROLLER_LOOP_PERFORMANCE_NAME)) {
+ our_seqno = ovsdb_idl_get_seqno(ctx.ovnsb_idl);
I'm worried about this. If some programming logic causes
performance_end_sample to be missed once, will the sequence number
updates ever take place?
I didn't look at ovn_controller (just looking at the diff in email), so
disregard if that's not actually a concern.
It's actually not a concern here.
+ }
+
update_probe_interval(&ctx, ovnsb_remote);
update_ssl_config(ctx.ovs_idl);
@@ -726,6 +738,8 @@ main(int argc, char *argv[])
ofctrl_put(&flow_table, &pending_ct_zones,
get_nb_cfg(ctx.ovnsb_idl));
+ performance_end_sample(CONTROLLER_LOOP_PERFORMANCE_NAME);
+
hmap_destroy(&flow_table);
}
if (ctx.ovnsb_idl_txn) {
@@ -790,6 +804,8 @@ main(int argc, char *argv[])
ofctrl_wait();
pinctrl_wait(&ctx);
}
+
+ performance_run();
Maybe the performance_init() should create a new ovs thread which makes
this call. Then there's no need to add it in the main() functions.
Actually, if a new daemon is implemented or an existing daemon ends up
calling a function that does performance counting, without the periodic
call to performance_run(), I think memory will end up growing without
bounds, because no cull will happen.
In my response to your review of patch 1, I suggested a threading model
where a background thread is responsible for storing samples and
calculating statistics. I think if I move to that model, then that
background thread can be responsible for periodically calling
performance_run(), and thus preventing this possibility from occurring.
ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop);
if (ovsdb_idl_loop_commit_and_wait(&ovs_idl_loop) == 1) {
@@ -837,6 +853,7 @@ main(int argc, char *argv[])
poll_block();
}
+ performance_destroy("ovn-controller-loop");
See the comment in previous about using an atexit() handler. Then you
can just iterate through the performance table and delete them all.
Yes, that is a very good idea.
unixctl_server_destroy(unixctl);
lflow_destroy();
ofctrl_destroy();
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
