[ovs-dev] [PATCH v12 2/3] dpif-netdev: Detailed performance stats for PMDs
Jan Scheurich
jan.scheurich at ericsson.com
Wed Apr 25 12:52:17 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
> -----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