[ovs-dev] [PATCH v7 1/2] dpif-netdev: Refactor PMD performance into dpif-netdev-perf

[Jan Scheurich]

> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets at samsung.com]
> Sent: Thursday, 11 January, 2018 13:15
> To: Jan Scheurich <jan.scheurich at ericsson.com>; dev at openvswitch.org
> Cc: ktraynor at redhat.com; ian.stokes at intel.com; billy.o.mahony at intel.com; Darrell Ball <dlu998 at gmail.com>
> Subject: Re: [PATCH v7 1/2] dpif-netdev: Refactor PMD performance into dpif-netdev-perf
> 
> I guess, this version was compile tested only.

Yes, unfortunately that's right. It was late last night, sorry!

I will send a corrected version, once we have agreed on:
a) Whether or not to add a blank after the colons in pmds-stats-show output
b) The approach the fix the failing ofproto-dpif test case (see below)

BR, Jan

> >
> > -AT_CHECK([ovs-appctl dpif-netdev/pmd-stats-show | sed SED_NUMA_CORE_PATTERN | sed '/cycles/d' | grep pmd -A 5], [0], [dnl
> > +AT_CHECK([ovs-appctl dpif-netdev/pmd-stats-show | sed SED_NUMA_CORE_PATTERN | sed '/cycles/d' | grep pmd -A 8], [0], [dnl
> >  pmd thread numa_id <cleared> core_id <cleared>:
> > +	packets received:0
> > +	packet recirculations:0
> > +	avg. datapath passes per packet:0.00
> >  	emc hits:0
> >  	megaflow hits:0
> > -	avg. subtable lookups per hit:0.00
> > -	miss:0
> > -	lost:0
> > +	avg. subtable lookups per megaflow hit:0.00
> > +	miss(i.e. lookup miss with success upcall):0
> > +	lost(i.e. lookup miss with failed upcall):0
> >  ])
> >
> >  ovs-appctl time/stop
> > @@ -197,13 +200,16 @@ AT_CHECK([cat ovs-vswitchd.log | filter_flow_install | strip_xout], [0], [dnl
> >  recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:77,dst=50:54:00:00:01:78),eth_type(0x0800),ipv4(frag=no),
> actions: <del>
> >  ])
> >
> > -AT_CHECK([ovs-appctl dpif-netdev/pmd-stats-show | sed SED_NUMA_CORE_PATTERN | sed '/cycles/d' | grep pmd -A 5], [0], [dnl
> > +AT_CHECK([ovs-appctl dpif-netdev/pmd-stats-show | sed SED_NUMA_CORE_PATTERN | sed '/cycles/d' | grep pmd -A 8], [0], [dnl
> >  pmd thread numa_id <cleared> core_id <cleared>:
> > +	packets received:20
> > +	packet recirculations:0
> > +	avg. datapath passes per packet:1.00
> >  	emc hits:19
> >  	megaflow hits:0
> > -	avg. subtable lookups per hit:0.00
> > -	miss:1
> > -	lost:0
> > +	avg. subtable lookups per megaflow hit:0.00
> > +	miss(i.e. lookup miss with success upcall):1
> > +	lost(i.e. lookup miss with failed upcall):0
> >  ])
> >
> >  OVS_VSWITCHD_STOP
> >

Tests will be adjusted.

> 
> In addition to not updated tests above, I also see failures of
> test "1165. ofproto-dpif.at:7478: testing ofproto-dpif - patch ports":
> 
> ./ofproto-dpif.at:7514: ovs-appctl dpif/show
> --- -   2018-01-11 15:06:46.324839417 +0300
> +++ /tests/testsuite.dir/at-groups/1165/stdout
> @@ -1,4 +1,4 @@
> -dummy at ovs-dummy: hit:13 missed:2
> +dummy at ovs-dummy: hit:0 missed:2
>         br0:
>                 br0 65534/100: (dummy-internal)
>                 p2 2/2: (dummy)

The root cause for this is two-fold:
1. The injected packet do not hit the EMC because of random insertion
2. The incorrect reporting of datapath stats of dpif-netdev.c:

Ad 1: The unit test is run with default emc-insert-inv-prob=100, so that it is random whether an EMC is created or not. When I change the test to set emc-insert-inv-prob=1, the test passes.

Ad 2: This is the code in ofproto_dpif printing the fetched dp_stats values:

    dpif_get_dp_stats(backer->dpif, &dp_stats);
    ds_put_format(ds, "%s: hit:%"PRIu64" missed:%"PRIu64"\n",
                  dpif_name(backer->dpif), dp_stats.n_hit, dp_stats.n_missed);

This is the current incorrect implementation in dpif-netdev.c:

static int
dpif_netdev_get_stats(const struct dpif *dpif, struct dpif_dp_stats *stats)
{
    struct dp_netdev *dp = get_dp_netdev(dpif);
    struct dp_netdev_pmd_thread *pmd;
    uint64_t pmd_stats[PMD_N_STATS];

    stats->n_flows = stats->n_hit =
            stats->n_mask_hit = stats->n_missed = stats->n_lost = 0;
    CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
        stats->n_flows += cmap_count(&pmd->flow_table);
        pmd_perf_read_counters(&pmd->perf_stats, pmd_stats);
        stats->n_hit += pmd_stats[PMD_STAT_EXACT_HIT];
        stats->n_mask_hit += pmd_stats[PMD_STAT_MASKED_HIT];
        stats->n_missed += pmd_stats[PMD_STAT_MISS];
        stats->n_lost += pmd_stats[PMD_STAT_LOST];
    }
    stats->n_masks = UINT32_MAX;

    return 0;
}

So the hits reported are only the EMC hits. Megaflow hits are ignored as dpif-netdev reports them as n_mask_hit, which according to the dpif-provider header file (and kernel datapath implementation!) should rather contain our PMD_STAT_MASKED_LOOKUP counter.

With the following correction on dpif-netdev.c the test passes even with emc-insert-inv-prob=100:

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index edc5e2e..7f51469 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1447,8 +1447,8 @@ dpif_netdev_get_stats(const struct dpif *dpif, struct dpif_dp_stats *stats)
     CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
         stats->n_flows += cmap_count(&pmd->flow_table);
         pmd_perf_read_counters(&pmd->perf_stats, pmd_stats);
-        stats->n_hit += pmd_stats[PMD_STAT_EXACT_HIT];
-        stats->n_mask_hit += pmd_stats[PMD_STAT_MASKED_HIT];
+        stats->n_hit += pmd_stats[PMD_STAT_EXACT_HIT] + pmd_stats[PMD_STAT_MASKED_HIT];
+        stats->n_mask_hit += pmd_stats[PMD_STAT_MASKED_LOOKUP];
         stats->n_missed += pmd_stats[PMD_STAT_MISS];
         stats->n_lost += pmd_stats[PMD_STAT_LOST];
     }

As this is really a bug in dpif-netdev.c, I suggest we fix it in a separate commit.

BR, Jan