> 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 <dlu998 at gmail.com> > --- > 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] / (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++; > + miss_upcall_cnt++; > handle_packet_upcall(pmd, packets[i], &keys[i], &actions, > &put_actions, &lost_cnt, now); > } > @@ -4865,8 +4883,7 @@ fast_path_processing(struct dp_netdev_pmd_thread *pmd, > for (i = 0; i < cnt; i++) { > if (OVS_UNLIKELY(!rules[i])) { > dp_packet_delete(packets[i]); > - lost_cnt++; > - miss_cnt++; > + miss_no_upcall_cnt++; > } > } > } > @@ -4885,10 +4902,11 @@ fast_path_processing(struct dp_netdev_pmd_thread *pmd, > dp_netdev_queue_batches(packet, flow, &keys[i].mf, batches, > n_batches); > } > > - dp_netdev_count_packet(pmd, DP_STAT_MASKED_HIT, cnt - miss_cnt); > - dp_netdev_count_packet(pmd, DP_STAT_LOOKUP_HIT, lookup_cnt); > - dp_netdev_count_packet(pmd, DP_STAT_MISS, miss_cnt); > - dp_netdev_count_packet(pmd, DP_STAT_LOST, lost_cnt); > + dp_netdev_count_packet(pmd, DP_STAT_MASKED_HIT, cnt - miss_upcall_cnt - > + miss_no_upcall_cnt); > + dp_netdev_count_packet(pmd, DP_STAT_MASKED_LOOKUP_HIT, lookup_cnt); > + dp_netdev_count_packet(pmd, DP_STAT_MISS, miss_upcall_cnt); > + dp_netdev_count_packet(pmd, DP_STAT_LOST, miss_no_upcall_cnt); > } > > /* Packets enter the datapath from a port (or from recirculation) here. > -- > 1.9.1 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev