[ovs-dev] [PATCH v12 2/3] dpif-netdev: Detailed performance stats for PMDs

[Stokes, Ian]

> This patch instruments the dpif-netdev datapath to record detailed
> statistics of what is happening in every iteration of a PMD thread.
> 
> The collection of detailed statistics can be controlled by a new
> Open_vSwitch configuration parameter "other_config:pmd-perf-metrics".
> By default it is disabled. The run-time overhead, when enabled, is
> in the order of 1%.

Hi Jan, thanks for the patch, 1 comment below.

[snip]

> +
> +/* This function can be called from the anywhere to clear the stats
> + * of PMD and non-PMD threads. */
> +void
> +pmd_perf_stats_clear(struct pmd_perf_stats *s)
> +{
> +    if (ovs_mutex_trylock(&s->stats_mutex) == 0) {
> +        /* Locking successful. PMD not polling. */
> +        pmd_perf_stats_clear_lock(s);
> +        ovs_mutex_unlock(&s->stats_mutex);
> +    } else {
> +        /* Request the polling PMD to clear the stats. There is no need
> to
> +         * block here as stats retrieval is prevented during clearing. */
> +        s->clear = true;
> +    }
> +}
> +
> +/* Functions recording PMD metrics per iteration. */
> +
> +inline void
> +pmd_perf_start_iteration(struct pmd_perf_stats *s)
> +{

Clang will complain that the mutex must be exclusively held for s->stats_mutex in this function.
I can add this with the following incremental 

--- a/lib/dpif-netdev-perf.c
+++ b/lib/dpif-netdev-perf.c
@@ -417,6 +417,7 @@ pmd_perf_stats_clear(struct pmd_perf_stats *s)
 inline void
 pmd_perf_start_iteration(struct pmd_perf_stats *s)
 {
+    ovs_mutex_lock(&s->stats_mutex);
     if (s->clear) {
         /* Clear the PMD stats before starting next iteration. */
         pmd_perf_stats_clear_lock(s);
@@ -433,6 +434,7 @@ pmd_perf_start_iteration(struct pmd_perf_stats *s)
         /* In case last_tsc has never been set before. */
         s->start_tsc = cycles_counter_update(s);
     }
+    ovs_mutex_unlock(&s->stats_mutex);
 }

But in that case is the ovs_mutex_trylock in function pmd_perf_stats_clear() made redundant? 

Ian

> +    if (s->clear) {
> +        /* Clear the PMD stats before starting next iteration. */
> +        pmd_perf_stats_clear_lock(s);
> +    }
> +    s->iteration_cnt++;
> +    /* Initialize the current interval stats. */
> +    memset(&s->current, 0, sizeof(struct iter_stats));
> +    if (OVS_LIKELY(s->last_tsc)) {
> +        /* We assume here that last_tsc was updated immediately prior at
> +         * the end of the previous iteration, or just before the first
> +         * iteration. */
> +        s->start_tsc = s->last_tsc;
> +    } else {
> +        /* In case last_tsc has never been set before. */
> +        s->start_tsc = cycles_counter_update(s);
> +    }
> +}
> +