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

Darrell Ball dball at vmware.com
Thu Aug 17 20:03:32 UTC 2017



On 8/17/17, 6:48 AM, "ovs-dev-bounces at openvswitch.org on behalf of Jan Scheurich" <ovs-dev-bounces at openvswitch.org on behalf of jan.scheurich at ericsson.com> wrote:

    Hi Darrell,
    
    Please find my specific comments below.
    
    I have a general concern for the pmd-stats-show counters: The output refers to the number of processed packets (e.g. processing cycles/pkt etc). But the data actually presented is the total number of *passes of packets through the datapath*. 
    
    In the early OVS days there was little difference, but with increasing amount of recirculations ( for tunnel decap, pop_mpls, bond selection, conntrack), nowadays each packet passes the datapath several times and the output becomes confusing for users, especially as it does not contain any indication of the average number of datapath passes per packet.
    
    I would strongly suggest that we add a new PMD stats counter for the actually received packets, which allows us to calculate and include the average number of passes per packet and true average processing cost per pkt. 
    
    What do you think?


[Darrell] This is an excellent proposal. We have had this level of recirculations for 3+ years, so it is nothing new, but better late to fix this than never.
                 Processing cycles is mainly useful for checking single flows and comparing relative numbers for code changes, however it is definitely 
                 better to use the actual packets received, rather than packet passes. 
                 I can do this enhancement/fix as part of this patch.
                 I will add the counter and go one step further and display both received packets and total recirculations. The processing cycles will be
                 based on received packets, as you pointed out.

    
    BR, Jan
    
    > 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.
    > 
    > The pmd stats test is enhanced to include megaflow stats counting and
    > checking.
    > Also, miss and lost stats are annotated to make it clear what they mean.
    > 
    > Signed-off-by: Darrell Ball <dlu998 at gmail.com>
    > ---
    >  lib/dpif-netdev.c | 78 ++++++++++++++++++++++++++++++++++--------------
    > -------
    >  tests/pmd.at      | 31 +++++++++++++++++-----
    >  2 files changed, 74 insertions(+), 35 deletions(-)
    > 
    > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 17e1666..dfc6684
    > 100644
    > --- a/lib/dpif-netdev.c
    > +++ b/lib/dpif-netdev.c
    > @@ -323,12 +323,19 @@ 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 upcall was
    > +                           done. */
    > +    DP_STAT_LOST,       /* Packets that did not match and upcall was
    > +                           not done. */
    
    If I understand the implementation correctly, this counts packets for which an upcall was done but failed so that the packet was dropped. There are no packets with a miss for which we currently do not do an upcall. Why do you suggest to change the meaning of the counter?


[Darrell] The usage now is:

miss: means an upcall was actually done; meaning we processed at the ofproto layer successfully. This brings this counter in sync with the counter in
the ofproto upcall code itself (n_upcalls++;), which counts ‘successful upcalls’. This is also what the user expects by OVS “miss upcall”; the user expects
a miss upcall to mean the packet was processed by the ofproto layer; the user really does not care that an ‘attempt was made’ to process at the ofproto layer.

lost: now means specifically a failed upcall; I actually had no idea what it might mean (based on the o/p of pmd-stats-show) before I looked at the actual code,
so, I also made this clear in the o/p.

The 2 numbers are non-overlapping now, which is an added bonus.

    
    > +    DP_N_PER_PKT_CNT,   /* The above statistics account for the total
    > +                           number of packets seen and should not be
    > +                           overlapping with each other. */
    
    I find this definition very obscure.

[Darrell] yeah, I agree; I had trouble with the naming as well and was not happy with it either.

 I think it was much clearer (and less likely to break again in the future) to explicitly sum up the above four non-overlapping counters to obtain the total number of packets as suggested by Ilya. It would be good to keep the comment, though.

[Darrell] This refers to the next comment below; let me add my response there instead.


    > +    DP_STAT_MASKED_LOOKUP = DP_N_PER_PKT_CNT,  /* Number of
    > subtable lookups
    > +                                                  for flow table hits. Each
    > +                                                  MASKED_HIT hit will have
    > +                                                  >= 1 MASKED_LOOKUP(s). */
    >      DP_N_STATS
    >  };
    
    +1 for the clarification of the comment.
    
    > +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_PER_PKT_CNT; i++) {
    > +        total_packets += stats[i];
    > +    }
    > +    return total_packets;
    > +}
    
    Please compute the sum explicitly. Much clearer and better maintainable.


[Darrell] The idea here is that if the number of counters change, then the sum is automatically adjusted – meaning this
                 code would not need to be maintained at all. We do this in many places in OVS code.

                 However, this now becomes a moot point. Since this number is packet passes and we want packets received,
                 as discussed above, which will be a single counter, this function is no longer needed.


    
    >      ds_put_format(reply,
    >                    "\temc hits:%llu\n\tmegaflow hits:%llu\n"
    > -                  "\tavg. subtable lookups per hit:%.2f\n"
    > -                  "\tmiss:%llu\n\tlost:%llu\n",
    > +                  "\tavg. subtable lookups per megaflow hit:%.2f\n"
    > +                  "\tmiss(upcall done):%llu\n\tlost(upcall not
    > + done):%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])
    > +                     / stats[DP_STAT_MASKED_HIT]
    >                    : 0,
    >                    stats[DP_STAT_MISS], stats[DP_STAT_LOST]);
    
    We should not lightly break backward compatibility in the output. There are plenty of scripts out there in test labs and production deployments that parse this output for performance monitoring/trouble-shooting purposes.
    
    It is perhaps OK to add new lines, but please do not unnecessarily change existing lines!

[Darrell] I had considered the impact on scripts and also the relative newness of pmd-stats-show.
                The scripts that I know look for a keyword, which all remain the same and a delimiter, in this case, a colon.
                 Adding additional lines makes the o/p longer and less succinct and in future, the keywords will change anyways.
                 Do you really think that if the keywords remain the same and the delimiter remains the same, that scripts will generally break ?
                 If so, I can add a legend instead; it would not be ideal, but would avoid the discussion about which scripts would break, whether scripts
                 should already handle this and so on…. 


    
    > -static inline void
    > +static inline int
    >  handle_packet_upcall(struct dp_netdev_pmd_thread *pmd,
    >                       struct dp_packet *packet,
    >                       const struct netdev_flow_key *key,
    >                       struct ofpbuf *actions, struct ofpbuf *put_actions,
    > -                     int *lost_cnt, long long now)
    > +                     int *miss_no_upcall, long long now)
    >  {
    >      struct ofpbuf *add_actions;
    >      struct dp_packet_batch b;
    >      struct match match;
    >      ovs_u128 ufid;
    > -    int error;
    > +    int error = 0;
    > 
    >      match.tun_md.valid = false;
    >      miniflow_expand(&key->mf, &match.flow); @@ -4749,8 +4765,8 @@
    > handle_packet_upcall(struct dp_netdev_pmd_thread *pmd,
    >                               put_actions);
    >      if (OVS_UNLIKELY(error && error != ENOSPC)) {
    >          dp_packet_delete(packet);
    > -        (*lost_cnt)++;
    > -        return;
    > +        (*miss_no_upcall)++;
    > +        return error;
    >      }
    
    If you return the result of the upcall explicitly, there is no need to also increase the lost_cnt passed by reference. Better step the appropriate counter in the calling function.

[Darrell] Sure


    
    > 
    >      /* The Netlink encoding of datapath flow keys cannot express @@ -
    > 4790,6 +4806,7 @@ handle_packet_upcall(struct dp_netdev_pmd_thread
    > *pmd,
    >          ovs_mutex_unlock(&pmd->flow_mutex);
    >          emc_probabilistic_insert(pmd, key, netdev_flow);
    >      }
    > +    return error;
    >  }
    > 
    >  static inline void
    > @@ -4811,7 +4828,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;
    
    Better upcall_ok_cnt and upcall_drop_cnt?

[Darrell] Sure; these counter names are better
    
    > @@ -4853,9 +4870,12 @@ fast_path_processing(struct
    > dp_netdev_pmd_thread *pmd,
    >                  continue;
    >              }
    > 
    > -            miss_cnt++;
    > -            handle_packet_upcall(pmd, packets[i], &keys[i], &actions,
    > -                                 &put_actions, &lost_cnt, now);
    > +            int error = handle_packet_upcall(pmd, packets[i], &keys[i],
    > +                           &actions, &put_actions, &miss_no_upcall_cnt,
    > + now);
    > +
    > +            if (OVS_LIKELY(!error)) {
    > +                miss_upcall_cnt++;
    > +            }
    
    Better:
    if (OVS_LIKELY(!error)) {
          upcall_ok_cnt++;
    } else {
         upcall_drop_cnt++;
    }

[Darrell] sure


    
    >              if (OVS_UNLIKELY(!rules[i])) {
    >                  dp_packet_delete(packets[i]);
    > -                lost_cnt++;
    > -                miss_cnt++;
    > +                miss_no_upcall_cnt++;
    
    upcall_drop_cnt++;

[Darrell] This is a repeated comment about the counter name.

    
    >              }
    >          }
    >      }
    > @@ -4885,10 +4904,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,
    > 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);
    >  }
    
    dp_netdev_count_packet(pmd, DP_STAT_MASKED_HIT, cnt - upcall_ok_cnt - upcall_drop_cnt);
    dp_netdev_count_packet(pmd, DP_STAT_MISS, upcall_ok_cnt);
    dp_netdev_count_packet(pmd, DP_STAT_LOST, upcall_drop_cnt);


[Darrell] This is a repeated comment about the counter name.

    
    _______________________________________________
    dev mailing list
    dev at openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=Lv3Gpn8nXejG_K9ooDT-_u4KUJveZKC9Vgjopu6O8mk&s=_ZBrNbm1KWhHi3FMz-keJb2ZIHgI9iz1wRx_jWUtzIU&e= 
    



More information about the dev mailing list