[ovs-dev] dpif-netdev: Fix per packet cycles statistics.

Message ID 1502456479-1484-1-git-send-email-i.maximets@samsung.com
State Accepted
Headers show

Commit Message

Ilya Maximets Aug. 11, 2017, 1:01 p.m.
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(-)

Comments

Jan Scheurich Aug. 11, 2017, 1:10 p.m. | #1
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
Darrell Ball Aug. 11, 2017, 7:02 p.m. | #2
-----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

Patch

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];