On 8/17/17, 1:03 PM, "ovs-dev-boun...@openvswitch.org on behalf of Darrell
Ball" <ovs-dev-boun...@openvswitch.org on behalf of db...@vmware.com> wrote:
On 8/17/17, 6:48 AM, "ovs-dev-boun...@openvswitch.org on behalf of Jan
Scheurich" <ovs-dev-boun...@openvswitch.org on behalf of
jan.scheur...@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
>>>>>>>>>>
[Darrell] JTBC, the above should be “Average processing cycles” rather than
“Processing cycles”.
<<<<<<<<<
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 <dlu...@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
d...@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=
_______________________________________________
dev mailing list
d...@openvswitch.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=0ce8Nqw4k3NHnzX2X9XCgQW0kvr2VQ9RIo1if7szI9I&s=2uR8iN1yvxymlCfcrkgi-4ty6LBZcjcuTvOpwmGEQT4&e=
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
