[ovs-dev] [PATCH ovn v3 2/3] ovn-northd: Add useful stopwatches
mark.d.gray at redhat.com
Wed Jun 30 12:49:28 UTC 2021
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.gray at redhat.com>
> Acked-by: Dumitru Ceara <dceara at 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
>> 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.
More information about the dev