On 24/06/2021 16:33, Dumitru Ceara wrote: > On 6/18/21 10:52 AM, Mark Gray wrote: >> For performance measurement, it is useful to understand the >> length of time required to complete a number of key code paths >> in ovn-northd.c. Add stopwatches to measure these timings. >> >> Signed-off-by: Mark Gray <mark.d.g...@redhat.com> >> --- > > Acked-by: Dumitru Ceara <dce...@redhat.com> > > I only have one real nit on this patch (below). Except for that here > are some more random thoughts for potential follow ups. > > I think we might benefit from a even more granular measurement, e.g., in > some of the tests I was doing a while ago build_datapaths() was also > taking up a significant amount of time.
Yes, I think so too. However, I don't want to start cluttering the code with stopwatches put in arbitrary locations. I would rather it was driven by actual need. I don't feel particularly qualified to make that decision and I figured this patch is more about establishing the precedent. > > Some more interesting ones to measure are in > build_lswitch_and_lrouter_flows(), the loops that call > build_lswitch_*_by_od/op(). I don't know however how we could deal with > the parallel case though. I could add these as an additional patch if you are convinced of their utility? > > Thanks, > Dumitru > >> northd/ovn-northd-ddlog.c | 15 +++++++++++++++ >> northd/ovn-northd.c | 20 ++++++++++++++++++++ >> 2 files changed, 35 insertions(+) >> >> diff --git a/northd/ovn-northd-ddlog.c b/northd/ovn-northd-ddlog.c >> index a4f2960bdcb8..7c552d516550 100644 >> --- a/northd/ovn-northd-ddlog.c >> +++ b/northd/ovn-northd-ddlog.c >> @@ -37,6 +37,7 @@ >> #include "ovsdb-parser.h" >> #include "ovsdb-types.h" >> #include "simap.h" >> +#include "stopwatch.h" >> #include "stream-ssl.h" >> #include "stream.h" >> #include "unixctl.h" >> @@ -50,6 +51,10 @@ VLOG_DEFINE_THIS_MODULE(ovn_northd); >> #include "northd/ovn-northd-ddlog-nb.inc" >> #include "northd/ovn-northd-ddlog-sb.inc" >> >> +#define NORTHD_LOOP_STOPWATCH_NAME "ovn-northd-loop" >> +#define OVNNB_DB_RUN_STOPWATCH_NAME "ovnnb_db_run" >> +#define OVNSB_DB_RUN_STOPWATCH_NAME "ovnsb_db_run" > > A bit of a nit: would it make sense to not duplicate these in both > northd versions and just add them to a (potentially new) common header file? > Not a nit at all. Makes perfect sense to maintain consistency. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
