Re: [ovs-dev] [patch_v2 2/3] dpif-netdev: Refactor some pmd stats.

2017-08-14 Thread Darrell Ball 


-Original Message-
From: Ilya Maximets <i.maxim...@samsung.com>
Date: Monday, August 14, 2017 at 5:36 AM
To: "ovs-dev@openvswitch.org" <ovs-dev@openvswitch.org>, Darrell Ball 
<db...@vmware.com>
Cc: 'Jan Scheurich' <jan.scheur...@ericsson.com>, Antonio Fischetti 
<antonio.fische...@intel.com>
Subject: Re: Re: [ovs-dev] [patch_v2 2/3] dpif-netdev: Refactor some pmd stats.

> The per packets stats are presently overlapping in that
> miss stats include lost stats; make these stats non-overlapping
> for clarity and make this clear in the dp_stat_type enum.  This
> also eliminates the need to increment two 'miss' stats for a
> single packet.
> 
> The subtable lookup stats is renamed to make it
> clear that it relates to masked lookups.
> 
> The stats that total to the number of packets seen are defined
> in dp_stat_type and an api is created to total the stats in case
> these stats are further divided in the future.
> 
> Signed-off-by: Darrell Ball 
> ---
>  lib/dpif-netdev.c | 58 
---
>  1 file changed, 38 insertions(+), 20 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 17e1666..38f5203 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -323,12 +323,21 @@ static struct dp_netdev_port 
*dp_netdev_lookup_port(const struct dp_netdev *dp,
>  OVS_REQUIRES(dp->port_mutex);
>  
>  enum dp_stat_type {
> -DP_STAT_EXACT_HIT,  /* Packets that had an exact match 
(emc). */?
> -DP_STAT_MASKED_HIT, /* Packets that matched in the flow 
table. */
> -DP_STAT_MISS,   /* Packets that did not match. */
> -DP_STAT_LOST,   /* Packets not passed up to the client. 
*/
> -DP_STAT_LOOKUP_HIT, /* Number of subtable lookups for flow 
table
> -   hits */
> +DP_STAT_EXACT_HIT,  /* Packets that had an exact match (emc). */
> +DP_STAT_MASKED_HIT, /* Packets that matched in the flow table. */
> +DP_STAT_MISS,   /* Packets that did not match and were passed
> +   up to the client. */

This definition of 'DP_STAT_MISS' looks illogical for me. 'MISS' means
the cache miss (EMC or CLS). But with the new definition the name became
meaningless.
Also, there is some informal concept in OVS: 'execute a miss' which
means the upcall execution. And users expects the miss counter to reflect
the number of upcalls executed.

The intention was that the definition of DP_STAT_MISS  to mean that there
was a miss in the cache (which includes exact match and masked caches) and an 
upcall
really happened. So, this ‘is’ what the user expects by ‘execute a miss’ in ovs.
I see 2 related bugs here; I fixed them.
Also, I think the o/p of pmd-stats-show should have some text making this 
clear; I will add them.

Maybe we can define something like DP_N_CACHE_STAT to split the cache
(EMC and CLS) hit/miss stats from others?

This is what DP_N_TOT_PKT_STAT is for. Cache hit/miss addition should total to 
the number of
packets seen. I just realized that this name does not need _STAT and I removed 
it.


> +DP_STAT_LOST,   /* Packets that did not match and were not
> +   passed up to client. */
> +DP_N_TOT_PKT_STAT,  /* The above statistics account for the total
> +   number of packets seen and should be
> +   non overlapping with each other. */
> +DP_STAT_MASKED_LOOKUP_HIT = DP_N_TOT_PKT_STAT,  /* Number of subtable
> +   lookups for flow 
table
> +   hits. Each 
MASKED_HIT
> +   hit will have >= 1
> +   MASKED_LOOKUP_HIT
> +   hit(s). */

Do we need the '_HIT' prefix for DP_STAT_MASKED_LOOKUP_HIT?
This kind of strange name because we're counting not hits but lookups.

I agree
_HIT is not correct as part of this name; I can fix it as part of this patch.


>  DP_N_STATS
>  };
>  
> @@ -749,13 +758,22 @@ enum pmd_info_type {
>  PMD_INFO_SHOW_RXQ /* Show poll-lists of pmd threads. */
>  };
>  
> +static unsigned long long
> +dp_netdev_calcul_total_packets(unsigned long long *stats)
> +{
> +unsigned long long total_packets = 0;
> +for (uint8_t i = 0; i < DP

Re: [ovs-dev] [patch_v2 2/3] dpif-netdev: Refactor some pmd stats.

2017-08-14 Thread Ilya Maximets 
> The per packets stats are presently overlapping in that
> miss stats include lost stats; make these stats non-overlapping
> for clarity and make this clear in the dp_stat_type enum.  This
> also eliminates the need to increment two 'miss' stats for a
> single packet.
> 
> The subtable lookup stats is renamed to make it
> clear that it relates to masked lookups.
> 
> The stats that total to the number of packets seen are defined
> in dp_stat_type and an api is created to total the stats in case
> these stats are further divided in the future.
> 
> Signed-off-by: Darrell Ball 
> ---
>  lib/dpif-netdev.c | 58 
> ---
>  1 file changed, 38 insertions(+), 20 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 17e1666..38f5203 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -323,12 +323,21 @@ static struct dp_netdev_port 
> *dp_netdev_lookup_port(const struct dp_netdev *dp,
>  OVS_REQUIRES(dp->port_mutex);
>  
>  enum dp_stat_type {
> -DP_STAT_EXACT_HIT,  /* Packets that had an exact match (emc). */
> -DP_STAT_MASKED_HIT, /* Packets that matched in the flow table. */
> -DP_STAT_MISS,   /* Packets that did not match. */
> -DP_STAT_LOST,   /* Packets not passed up to the client. */
> -DP_STAT_LOOKUP_HIT, /* Number of subtable lookups for flow table
> -   hits */
> +DP_STAT_EXACT_HIT,  /* Packets that had an exact match (emc). */
> +DP_STAT_MASKED_HIT, /* Packets that matched in the flow table. */
> +DP_STAT_MISS,   /* Packets that did not match and were passed
> +   up to the client. */

This definition of 'DP_STAT_MISS' looks illogical for me. 'MISS' means
the cache miss (EMC or CLS). But with the new definition the name became
meaningless.
Also, there is some informal concept in OVS: 'execute a miss' which
means the upcall execution. And users expects the miss counter to reflect
the number of upcalls executed.

Maybe we can define something like DP_N_CACHE_STAT to split the cache
(EMC and CLS) hit/miss stats from others?

> +DP_STAT_LOST,   /* Packets that did not match and were not
> +   passed up to client. */
> +DP_N_TOT_PKT_STAT,  /* The above statistics account for the total
> +   number of packets seen and should be
> +   non overlapping with each other. */
> +DP_STAT_MASKED_LOOKUP_HIT = DP_N_TOT_PKT_STAT,  /* Number of subtable
> +   lookups for flow table
> +   hits. Each MASKED_HIT
> +   hit will have >= 1
> +   MASKED_LOOKUP_HIT
> +   hit(s). */

Do we need the '_HIT' prefix for DP_STAT_MASKED_LOOKUP_HIT?
This kind of strange name because we're counting not hits but lookups.


>  DP_N_STATS
>  };
>  
> @@ -749,13 +758,22 @@ enum pmd_info_type {
>  PMD_INFO_SHOW_RXQ /* Show poll-lists of pmd threads. */
>  };
>  
> +static unsigned long long
> +dp_netdev_calcul_total_packets(unsigned long long *stats)
> +{
> +unsigned long long total_packets = 0;
> +for (uint8_t i = 0; i < DP_N_TOT_PKT_STAT; i++) {
> +total_packets += stats[i];
> +}
> +return total_packets;
> +}
> +
>  static void
>  pmd_info_show_stats(struct ds *reply,
>  struct dp_netdev_pmd_thread *pmd,
>  unsigned long long stats[DP_N_STATS],
>  uint64_t cycles[PMD_N_CYCLES])
>  {
> -unsigned long long total_packets;
>  uint64_t total_cycles = 0;
>  int i;
>  
> @@ -773,10 +791,6 @@ pmd_info_show_stats(struct ds *reply,
>  }
>  }
>  
> -/* 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];
> @@ -804,7 +818,8 @@ pmd_info_show_stats(struct ds *reply,
>"\tmiss:%llu\n\tlost:%llu\n",
>stats[DP_STAT_EXACT_HIT], stats[DP_STAT_MASKED_HIT],
>stats[DP_STAT_MASKED_HIT] > 0
> -  ? (1.0*stats[DP_STAT_LOOKUP_HIT])/stats[DP_STAT_MASKED_HIT]
> +  ? (1.0 * stats[DP_STAT_MASKED_LOOKUP_HIT])
> + / stats[DP_STAT_MASKED_HIT]
>: 0,
>stats[DP_STAT_MISS], stats[DP_STAT_LOST]);
>  
> @@ -820,6 +835,9 @@ pmd_info_show_stats(struct ds *reply,
>cycles[PMD_CYCLES_PROCESSING],
>cycles[PMD_CYCLES_PROCESSING] /

[ovs-dev] [patch_v2 2/3] dpif-netdev: Refactor some pmd stats.

2017-08-13 Thread Darrell Ball 
The per packets stats are presently overlapping in that
miss stats include lost stats; make these stats non-overlapping
for clarity and make this clear in the dp_stat_type enum.  This
also eliminates the need to increment two 'miss' stats for a
single packet.

The subtable lookup stats is renamed to make it
clear that it relates to masked lookups.

The stats that total to the number of packets seen are defined
in dp_stat_type and an api is created to total the stats in case
these stats are further divided in the future.

Signed-off-by: Darrell Ball 
---
 lib/dpif-netdev.c | 58 ---
 1 file changed, 38 insertions(+), 20 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 17e1666..38f5203 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -323,12 +323,21 @@ static struct dp_netdev_port *dp_netdev_lookup_port(const 
struct dp_netdev *dp,
 OVS_REQUIRES(dp->port_mutex);
 
 enum dp_stat_type {
-DP_STAT_EXACT_HIT,  /* Packets that had an exact match (emc). */
-DP_STAT_MASKED_HIT, /* Packets that matched in the flow table. */
-DP_STAT_MISS,   /* Packets that did not match. */
-DP_STAT_LOST,   /* Packets not passed up to the client. */
-DP_STAT_LOOKUP_HIT, /* Number of subtable lookups for flow table
-   hits */
+DP_STAT_EXACT_HIT,  /* Packets that had an exact match (emc). */
+DP_STAT_MASKED_HIT, /* Packets that matched in the flow table. */
+DP_STAT_MISS,   /* Packets that did not match and were passed
+   up to the client. */
+DP_STAT_LOST,   /* Packets that did not match and were not
+   passed up to client. */
+DP_N_TOT_PKT_STAT,  /* The above statistics account for the total
+   number of packets seen and should be
+   non overlapping with each other. */
+DP_STAT_MASKED_LOOKUP_HIT = DP_N_TOT_PKT_STAT,  /* Number of subtable
+   lookups for flow table
+   hits. Each MASKED_HIT
+   hit will have >= 1
+   MASKED_LOOKUP_HIT
+   hit(s). */
 DP_N_STATS
 };
 
@@ -749,13 +758,22 @@ enum pmd_info_type {
 PMD_INFO_SHOW_RXQ /* Show poll-lists of pmd threads. */
 };
 
+static unsigned long long
+dp_netdev_calcul_total_packets(unsigned long long *stats)
+{
+unsigned long long total_packets = 0;
+for (uint8_t i = 0; i < DP_N_TOT_PKT_STAT; i++) {
+total_packets += stats[i];
+}
+return total_packets;
+}
+
 static void
 pmd_info_show_stats(struct ds *reply,
 struct dp_netdev_pmd_thread *pmd,
 unsigned long long stats[DP_N_STATS],
 uint64_t cycles[PMD_N_CYCLES])
 {
-unsigned long long total_packets;
 uint64_t total_cycles = 0;
 int i;
 
@@ -773,10 +791,6 @@ pmd_info_show_stats(struct ds *reply,
 }
 }
 
-/* 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];
@@ -804,7 +818,8 @@ pmd_info_show_stats(struct ds *reply,
   "\tmiss:%llu\n\tlost:%llu\n",
   stats[DP_STAT_EXACT_HIT], stats[DP_STAT_MASKED_HIT],
   stats[DP_STAT_MASKED_HIT] > 0
-  ? (1.0*stats[DP_STAT_LOOKUP_HIT])/stats[DP_STAT_MASKED_HIT]
+  ? (1.0 * stats[DP_STAT_MASKED_LOOKUP_HIT])
+ / stats[DP_STAT_MASKED_HIT]
   : 0,
   stats[DP_STAT_MISS], stats[DP_STAT_LOST]);
 
@@ -820,6 +835,9 @@ pmd_info_show_stats(struct ds *reply,
   cycles[PMD_CYCLES_PROCESSING],
   cycles[PMD_CYCLES_PROCESSING] / (double)total_cycles * 100);
 
+/* Sum of all the matched and not matched packets gives the total.  */
+unsigned long long total_packets =
+ dp_netdev_calcul_total_packets(stats);
 if (total_packets == 0) {
 return;
 }
@@ -4811,7 +4829,7 @@ fast_path_processing(struct dp_netdev_pmd_thread *pmd,
 struct dpcls *cls;
 struct dpcls_rule *rules[PKT_ARRAY_SIZE];
 struct dp_netdev *dp = pmd->dp;
-int miss_cnt = 0, lost_cnt = 0;
+int miss_upcall_cnt = 0, miss_no_upcall_cnt = 0, lost_cnt = 0;
 int lookup_cnt = 0, add_lookup_cnt;
 bool any_miss;
 size_t i;
@@ -4853,7 +4871,7 @@ fast_path_processing(struct dp_netdev_pmd_thread *pmd,
 continue;
 }
 
-miss_cnt++;
+