On Mon, Feb 12, 2018 at 6:16 AM, Mark Michelson <mmich...@redhat.com> wrote:
>
> On 02/09/2018 07:36 PM, Han Zhou wrote:
>>
>> Looks a great tool! Just some minor comments:
>>
>> On Fri, Feb 9, 2018 at 3:00 PM, Mark Michelson <mmich...@redhat.com
<mailto: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().
>
>
> Thanks for the review Han. I attempted to account for this in the code by
only ending the sample in the case that ofctrl_can_put() returns true. This
way, we might start a measurement in one loop iteration, but the
measurement might not end until a later iteration if ofctrl_can_put()
returns false. Do you have suggestions for other factors that I should take
into account for determining when to end a measurement?
>
I think one way to address this, if we really want to, is to compare
ofctrl_get_cur_cfg() with get_nb_cfg(ctx.ovnsb_idl). If they are equal, it
means ovs-vswitchd caught up with SB with all required flows installed.
I suggest to have separate measurements, one for main loop execution time,
one for SB change handling up until flows installed to OVS. They could be
separate patches. What do you think?
>
>>
>> >
>> > The statistics can be queried using:
>> >
>> > ovs-appctl -t ovn-controller performance/show ovn-controller-loop
>> >
>> > Signed-off-by: Mark Michelson <mmich...@redhat.com <mailto:
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 <mailto:d...@openvswitch.org>
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>> Acked-by: Han Zhan <zhou...@gmail.com <mailto:zhou...@gmail.com>>
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
