Looks a great tool! Just some minor comments: On Fri, Feb 9, 2018 at 3:00 PM, Mark Michelson <mmich...@redhat.com> wrote: > > 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 comment is a little inaccurate. The change cannot guarantee measurement of "write the flow table down to vswitchd", since sending the flow-adding messages to vswitchd could be performed in multiple iterations of the main loop, depending on the buffering status of vconn_send(). If it can't be completed in one round, the message sendings will continue in next loops in ofctrl_run(). > > The statistics can be queried using: > > ovs-appctl -t ovn-controller performance/show ovn-controller-loop > > Signed-off-by: Mark Michelson <mmich...@redhat.com> > --- > 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 7592bda25..b9f8950d4 100644 > --- a/ovn/controller/ovn-controller.c > +++ b/ovn/controller/ovn-controller.c > @@ -57,6 +57,9 @@ > #include "stream.h" > #include "unixctl.h" > #include "util.h" > +#include "timeval.h" > +#include "timer.h" > +#include "performance.h" > > VLOG_DEFINE_THIS_MODULE(main); > > @@ -67,6 +70,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[]); > @@ -639,8 +644,10 @@ main(int argc, char *argv[]) > unixctl_command_register("inject-pkt", "MICROFLOW", 1, 1, inject_pkt, > &pending_pkt); > > + performance_create(CONTROLLER_LOOP_PERFORMANCE_NAME, PERF_MS); > /* 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); > @@ -659,6 +666,12 @@ 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)) { Shall we measure when there is no SB change, too? E.g. the loop can be triggered by local ovs-idl change, or by pin-ctrl messages, and these changes will trigger all the flow computations, which contributes a significant cost in ovn-controller, so I think we'd better measure them as well. I am working on an incremental processing framework which would avoid this kind of unnecessary recompute. I hope with this tool the improvements can be measured accurately :) > + performance_start_sample(CONTROLLER_LOOP_PERFORMANCE_NAME, > + time_msec()); > + our_seqno = ovsdb_idl_get_seqno(ctx.ovnsb_idl); > + } > + > update_probe_interval(&ctx, ovnsb_remote); > > update_ssl_config(ctx.ovs_idl); > @@ -728,6 +741,9 @@ 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, > + time_msec()); > + > hmap_destroy(&flow_table); > } > if (ctx.ovnsb_idl_txn) { > @@ -792,6 +808,7 @@ main(int argc, char *argv[]) > ofctrl_wait(); > pinctrl_wait(&ctx); > } > + > ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop); > > if (ovsdb_idl_loop_commit_and_wait(&ovs_idl_loop) == 1) { > -- > 2.13.6 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Acked-by: Han Zhan <zhou...@gmail.com> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev