| Message ID | 1502456479-1484-1-git-send-email-i.maximets@samsung.com |
|---|---|
| State | Accepted |
| Headers | show |
Thanks for fixing this, Ilya! It was correct at the time it was committed but very likely to break afterwards. I should have removed the loop from the start and change the way you propose. Acked-by: Jan Scheurich <jan.scheurich@ericsson.com> > -----Original Message----- > From: Ilya Maximets [mailto:i.maximets@samsung.com] > Sent: Friday, 11 August, 2017 15:01 > To: ovs-dev@openvswitch.org > Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Antonio Fischetti <antonio.fischetti@intel.com>; Darrell Ball <dball@vmware.com>; Ilya > Maximets <i.maximets@samsung.com>; Jan Scheurich <jan.scheurich@ericsson.com> > Subject: [PATCH] dpif-netdev: Fix per packet cycles statistics. > > DP_STAT_LOOKUP_HIT statistics used mistakenly for calculation > of total number of packets. This leads to completely wrong > per packet cycles statistics. > > For example: > > emc hits:0 > megaflow hits:253702308 > avg. subtable lookups per hit:1.50 > miss:0 > lost:0 > avg cycles per packet: 248.32 (157498766585/634255770) > > In this case 634255770 total_packets value used for avg > per packet calculation: > > total_packets = 'megaflow hits' + 'megaflow hits' * 1.5 > > The real value should be 524.38 (157498766585/253702308) > > Fix that by summing only stats that reflects match/not match. > It's decided to make direct summing of required values instead of > disabling some stats in a loop to make calculations more clear and > avoid similar issues in the future. > > CC: Jan Scheurich <jan.scheurich@ericsson.com> > Fixes: 3453b4d62a98 ("dpif-netdev: dpcls per in_port with sorted subtables") > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > --- > lib/dpif-netdev.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index e2cd931..17e1666 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -755,7 +755,7 @@ pmd_info_show_stats(struct ds *reply, > unsigned long long stats[DP_N_STATS], > uint64_t cycles[PMD_N_CYCLES]) > { > - unsigned long long total_packets = 0; > + unsigned long long total_packets; > uint64_t total_cycles = 0; > int i; > > @@ -771,13 +771,12 @@ pmd_info_show_stats(struct ds *reply, > } else { > stats[i] = 0; > } > - > - if (i != DP_STAT_LOST) { > - /* Lost packets are already included in DP_STAT_MISS */ > - total_packets += stats[i]; > - } > } > > + /* Sum of all the matched and not matched packets gives the total. */ > + total_packets = stats[DP_STAT_EXACT_HIT] + stats[DP_STAT_MASKED_HIT] > + + stats[DP_STAT_MISS]; > + > for (i = 0; i < PMD_N_CYCLES; i++) { > if (cycles[i] > pmd->cycles_zero[i]) { > cycles[i] -= pmd->cycles_zero[i]; > -- > 2.7.4
-----Original Message----- From: Ilya Maximets <i.maximets@samsung.com> Date: Friday, August 11, 2017 at 6:01 AM To: "ovs-dev@openvswitch.org" <ovs-dev@openvswitch.org> Cc: Heetae Ahn <heetae82.ahn@samsung.com>, Antonio Fischetti <antonio.fischetti@intel.com>, Darrell Ball <dball@vmware.com>, Ilya Maximets <i.maximets@samsung.com>, Jan Scheurich <jan.scheurich@ericsson.com> Subject: [PATCH] dpif-netdev: Fix per packet cycles statistics. DP_STAT_LOOKUP_HIT statistics used mistakenly for calculation of total number of packets. This leads to completely wrong per packet cycles statistics. It would seem unlikely that the addition of 3 stats numbers, done once in the code, could go wrong. Maybe the root cause here is that we don’t have stats names that makes the meaning clear which is a subcategory of stat; maybe some defined APIs would also help here. For example: emc hits:0 megaflow hits:253702308 avg. subtable lookups per hit:1.50 miss:0 lost:0 avg cycles per packet: 248.32 (157498766585/634255770) In this case 634255770 total_packets value used for avg per packet calculation: total_packets = 'megaflow hits' + 'megaflow hits' * 1.5 The real value should be 524.38 (157498766585/253702308) Fix that by summing only stats that reflects match/not match. It's decided to make direct summing of required values instead of disabling some stats in a loop to make calculations more clear and avoid similar issues in the future. CC: Jan Scheurich <jan.scheurich@ericsson.com> Fixes: 3453b4d62a98 ("dpif-netdev: dpcls per in_port with sorted subtables") Signed-off-by: Ilya Maximets <i.maximets@samsung.com> --- lib/dpif-netdev.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index e2cd931..17e1666 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -755,7 +755,7 @@ pmd_info_show_stats(struct ds *reply, unsigned long long stats[DP_N_STATS], uint64_t cycles[PMD_N_CYCLES]) { - unsigned long long total_packets = 0; + unsigned long long total_packets; uint64_t total_cycles = 0; int i; @@ -771,13 +771,12 @@ pmd_info_show_stats(struct ds *reply, } else { stats[i] = 0; } - - if (i != DP_STAT_LOST) { - /* Lost packets are already included in DP_STAT_MISS */ - total_packets += stats[i]; - } } + /* Sum of all the matched and not matched packets gives the total. */ + total_packets = stats[DP_STAT_EXACT_HIT] + stats[DP_STAT_MASKED_HIT] + + stats[DP_STAT_MISS]; + for (i = 0; i < PMD_N_CYCLES; i++) { if (cycles[i] > pmd->cycles_zero[i]) { cycles[i] -= pmd->cycles_zero[i]; -- 2.7.4
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index e2cd931..17e1666 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -755,7 +755,7 @@ pmd_info_show_stats(struct ds *reply, unsigned long long stats[DP_N_STATS], uint64_t cycles[PMD_N_CYCLES]) { - unsigned long long total_packets = 0; + unsigned long long total_packets; uint64_t total_cycles = 0; int i; @@ -771,13 +771,12 @@ pmd_info_show_stats(struct ds *reply, } else { stats[i] = 0; } - - if (i != DP_STAT_LOST) { - /* Lost packets are already included in DP_STAT_MISS */ - total_packets += stats[i]; - } } + /* Sum of all the matched and not matched packets gives the total. */ + total_packets = stats[DP_STAT_EXACT_HIT] + stats[DP_STAT_MASKED_HIT] + + stats[DP_STAT_MISS]; + for (i = 0; i < PMD_N_CYCLES; i++) { if (cycles[i] > pmd->cycles_zero[i]) { cycles[i] -= pmd->cycles_zero[i];
DP_STAT_LOOKUP_HIT statistics used mistakenly for calculation of total number of packets. This leads to completely wrong per packet cycles statistics. For example: emc hits:0 megaflow hits:253702308 avg. subtable lookups per hit:1.50 miss:0 lost:0 avg cycles per packet: 248.32 (157498766585/634255770) In this case 634255770 total_packets value used for avg per packet calculation: total_packets = 'megaflow hits' + 'megaflow hits' * 1.5 The real value should be 524.38 (157498766585/253702308) Fix that by summing only stats that reflects match/not match. It's decided to make direct summing of required values instead of disabling some stats in a loop to make calculations more clear and avoid similar issues in the future. CC: Jan Scheurich <jan.scheurich@ericsson.com> Fixes: 3453b4d62a98 ("dpif-netdev: dpcls per in_port with sorted subtables") Signed-off-by: Ilya Maximets <i.maximets@samsung.com> --- lib/dpif-netdev.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)