[ovs-dev] [patch_v2 2/3] dpif-netdev: Refactor some pmd stats.
Darrell Ball
dball at vmware.com
Tue Aug 15 02:52:14 UTC 2017
-----Original Message-----
From: Ilya Maximets <i.maximets at samsung.com>
Date: Monday, August 14, 2017 at 5:36 AM
To: "ovs-dev at openvswitch.org" <ovs-dev at openvswitch.org>, Darrell Ball <dball at vmware.com>
Cc: 'Jan Scheurich' <jan.scheurich at ericsson.com>, Antonio Fischetti <antonio.fischetti at 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 <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.
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_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
More information about the dev
mailing list