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

Stokes, Ian ian.stokes at intel.com
Thu Apr 26 18:49:33 UTC 2018


> Hi Ian,
> 
> Thanks for checking this. I suggest to address Clang's compliant by the
> following incremental:
> 
> diff --git a/lib/dpif-netdev-perf.c b/lib/dpif-netdev-perf.c index
> 47ce2c2..c7b8e7b 100644
> --- a/lib/dpif-netdev-perf.c
> +++ b/lib/dpif-netdev-perf.c
> @@ -442,6 +442,7 @@ pmd_perf_stats_clear(struct pmd_perf_stats *s)
> 
>  inline void
>  pmd_perf_start_iteration(struct pmd_perf_stats *s)
> +    OVS_REQUIRES(s->stats_mutex)
>  {
>      if (s->clear) {
>          /* Clear the PMD stats before starting next iteration. */
> 
> The mutex pmd->perf_stats.stats_mutex is taken by the calling
> pmd_thread_main() while it is in the poll loop. So the pre-requisite for
> pmd_perf_start_iteration() is in place. It is essential for performance
> that pmd_perf_start_iteration () does not take the lock in each iteration.
> 
> I hope that Clang is intelligent enough to recognize this. If not, I
> wouldn't know how to fix it other than by removing OVS_REQUIRES(s-
> >stats_mutex) from pmd_perf_stats_clear_lock() and just rely on comments.
> 
> BR, Jan

Thanks Jan, that resolves the issue and there's a clear travis build now. I'll add this series as part of the next pull request.

Thanks all for the work reviewing and testing this.

Ian
> 
> > -----Original Message-----
> > From: Stokes, Ian [mailto:ian.stokes at intel.com]
> > Sent: Wednesday, 25 April, 2018 11:55
> > To: Jan Scheurich <jan.scheurich at ericsson.com>; dev at openvswitch.org
> > Cc: ktraynor at redhat.com; i.maximets at samsung.com; O Mahony, Billy
> > <billy.o.mahony at intel.com>
> > Subject: RE: [PATCH v12 2/3] dpif-netdev: Detailed performance stats
> > for PMDs
> >
> > > 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);
> > > +    }
> > > +}
> > > +


More information about the dev mailing list