[ovs-dev] [PATCH] coverage: Add coverage_try_clear() for performance-critical threads.

Ilya Maximets i.maximets at samsung.com
Fri Aug 14 10:43:33 UTC 2015


I've tested that. Looks good for me.

Acked-by: Ilya Maximets <i.maximets at samsung.com>

On 13.08.2015 21:48, Alex Wang wrote:
> For performance-critical threads like pmd threads, we currently make them
> never call coverage_clear() to avoid contention over the global mutex
> 'coverage_mutex'.  So, even though pmd thread still keeps updating their
> thread-local coverage count, the count is never attributed to the global
> total.  But it is useful to have them available.
> 
> This commit makes this happen by implementing a non-contending version
> of the clear function, coverage_try_clear().  The function will use
> the ovs_mutex_trylock() and return immediately if the mutex cannot
> be acquired.  Since threads like pmd thread are always busy-looping,
> the lock will eventually be acquired.
> 
> Requested-by: Ilya Maximets <i.maximets at samsung.com>
> Signed-off-by: Alex Wang <alexw at nicira.com>
> ---
>  lib/coverage.c    |   33 +++++++++++++++++++++++++++++----
>  lib/coverage.h    |    1 +
>  lib/dpif-netdev.c |    2 ++
>  3 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/coverage.c b/lib/coverage.c
> index 6121956..9584cac 100644
> --- a/lib/coverage.c
> +++ b/lib/coverage.c
> @@ -245,9 +245,14 @@ coverage_read(struct svec *lines)
>  
>  /* Runs approximately every COVERAGE_CLEAR_INTERVAL amount of time to
>   * synchronize per-thread counters with global counters. Every thread maintains
> - * a separate timer to ensure all counters are periodically aggregated. */
> -void
> -coverage_clear(void)
> + * a separate timer to ensure all counters are periodically aggregated.
> + *
> + * Uses 'ovs_mutex_trylock()' if 'trylock' is true.  This is to prevent
> + * multiple performance-critical threads contending over the 'coverage_mutex'.
> + *
> + * */
> +static void
> +coverage_clear__(bool trylock)
>  {
>      long long int now, *thread_time;
>  
> @@ -262,7 +267,15 @@ coverage_clear(void)
>      if (now >= *thread_time) {
>          size_t i;
>  
> -        ovs_mutex_lock(&coverage_mutex);
> +        if (trylock) {
> +            /* Returns if cannot acquire lock. */
> +            if (ovs_mutex_trylock(&coverage_mutex)) {
> +                return;
> +            }
> +        } else {
> +            ovs_mutex_lock(&coverage_mutex);
> +        }
> +
>          for (i = 0; i < n_coverage_counters; i++) {
>              struct coverage_counter *c = coverage_counters[i];
>              c->total += c->count();
> @@ -272,6 +285,18 @@ coverage_clear(void)
>      }
>  }
>  
> +void
> +coverage_clear(void)
> +{
> +    coverage_clear__(false);
> +}
> +
> +void
> +coverage_try_clear(void)
> +{
> +    coverage_clear__(true);
> +}
> +
>  /* Runs approximately every COVERAGE_RUN_INTERVAL amount of time to update the
>   * coverage counters' 'min' and 'hr' array.  'min' array is for cumulating
>   * per second counts into per minute count.  'hr' array is for cumulating per
> diff --git a/lib/coverage.h b/lib/coverage.h
> index 832c433..34a04aa 100644
> --- a/lib/coverage.h
> +++ b/lib/coverage.h
> @@ -88,6 +88,7 @@ void coverage_counter_register(struct coverage_counter*);
>  void coverage_init(void);
>  void coverage_log(void);
>  void coverage_clear(void);
> +void coverage_try_clear(void);
>  void coverage_run(void);
>  
>  #endif /* coverage.h */
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index c144352..f4ff8cb 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -42,6 +42,7 @@
>  #include "fat-rwlock.h"
>  #include "flow.h"
>  #include "cmap.h"
> +#include "coverage.h"
>  #include "latch.h"
>  #include "list.h"
>  #include "match.h"
> @@ -2696,6 +2697,7 @@ reload:
>              lc = 0;
>  
>              emc_cache_slow_sweep(&pmd->flow_cache);
> +            coverage_try_clear();
>              ovsrcu_quiesce();
>  
>              atomic_read_relaxed(&pmd->change_seq, &seq);
> 



More information about the dev mailing list