[ovs-dev] [PATCH v6 2/5] Measure timing of ovn-controller loops.

Han Zhou zhouhan at gmail.com
Thu Mar 15 17:04:10 UTC 2018


On Wed, Mar 7, 2018 at 9:01 AM, Mark Michelson <mmichels at redhat.com> wrote:
>
> This modifies ovn-controller to measure the amount of time it takes to
> detect a change in the southbound database and generate the resulting
> flow table. This may require multiple iterations of the ovn-controller
> loop.
>
> The statistics can be queried using:
>
> ovs-appctl -t ovn-controller stopwatch/show ovn-controller-loop
>
> The statistics can be reset using:
>
> ovs-appctl -t ovn-controller stopwatch/reset ovn-controller-loop
>
If the purpose of this patch is for measuring *SB changes* handling time
only, the statistics may be renamed to something like
ovn-controller-loop-sb. Otherwise it is a little bit misleading since the
controller loop handles many other things, such as local interface changes,
Packet-ins, OF messages, etc.

> Signed-off-by: Mark Michelson <mmichels at 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..abd253fca 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 "stopwatch.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_STOPWATCH_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);
>
> +    stopwatch_create(CONTROLLER_LOOP_STOPWATCH_NAME, SW_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)) {
> +            stopwatch_start(CONTROLLER_LOOP_STOPWATCH_NAME,
> +                            time_msec());
When multiple iterations are needed for one SB change handling, the
stopwatch can be started before the previous one is stopped, if seqno
changes in between. Could you explain the consideration here?
> +            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));
>
> +                    stopwatch_stop(CONTROLLER_LOOP_STOPWATCH_NAME,
> +                                   time_msec());
> +

Two issues here:

Firstly, we start the stopwatch only when seqno changes, but we will always
call the stop, which may result in many unnecessary messages to the
stopwatch thread. Maybe it is not a big issue, but it would be better if we
have a variable to control it to stop only when necessary.

Secondly, when multiple iterations are needed for one SB change handling,
the iterations will be like:
- iter1:
    - stopwatch_start
    - ofctrl_put
    - stopwatch stop
- iter2:
    - ofctrl_run
    - (ofctrl_put skipped)
...
- iterN:
    - ofctrl_run
    - ofctrl_put (will not send to OVS, since desired and installed flows
should be same now)
    - stopwatch stop

So putting stopwatch_stop here still measures only the first iteration. I
had a suggestion earlier about checking ofctrl.cur_cfg, but I was wrong
since I realized that nb_cfg changes only upon user requesting waiting for
HV. To really measure the multi-iteration processing, we need a mechanism
that ofctrl takes current SB seqno as input and maintains the cur_seq_no
just like how cur_cfg is maintained.
>                      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.14.3
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Acked-by: Han Zhou <hzhou8 at ebay.com>


More information about the dev mailing list