On Mon, Mar 18, 2013 at 05:28:04PM -0700, Andy Zhou wrote:
> This patch adds more flow related stats to the output of
> "ovs-appctl dpif/show". Specifically, the follow information
> are added per ofproto:
>
> - Max flow table size
> - Average flow table size
> - Average flow table add rate
> - Average flow table delete rate
> - Average flow entry life in milliseconds
>
> Feature #15366
>
> Signed-off-by: Andy Zhou <az...@nicira.com>
Thanks for the patch, and I'm sorry that it's taken all week to get
around to reviewing it.
GCC reports:
../ofproto/ofproto-dpif.c: In function 'show_dp_format':
../ofproto/ofproto-dpif.c:8157:19: error: format '%zu' expects argument of
type 'size_t', but argument 6 has type 'uint64_t' [-Werror=format]
Please use "double" instead of "float" everywhere. "float" takes up a
little less space, but we are not allocating enough of them that that
would make a difference, and "float" is much less precise than "double".
This commit adds a member 'created' to struct facet, but it does not
appear to use it anywhere.
The naming of members in ofproto_dpif is a little misleading. The word
"flow" is used in many members that refer to subfacets, and the comments
also talk about flows. The word "flow" has too many meanings, and so it
would be better to use something more specific. Maybe "subfacet", if
that doesn't make the names too long.
A lot of the new members in ofproto_dpif have types with specific
bitwise sizes, e.g. uint32_t. I lean toward using these types mostly in
situations where the bit size is required for some protocol, etc. (This
is, for example, why n_hit and n_missed are uint64_t: these correspond
to struct dpif_dp_stats, which comes almost directly from struct
ovs_dp_stats used in the Netlink protocol to the kernel.) I think it is
usually better to use ordinary C types in other situations. So I would
probably change flow_add_count to unsigned int, total_flow_add_count to
unsigned long long int, etc.
In compute_avg_flow_count(), the computation looks like it unnecessarily
loses precision by dividing one integer by another before dividing by a
real number.
show_dp_rates() makes two calls to ds_put_cstr() before a call to
ds_put_format(). I think that all of that can be written as a single
call to ds_put_format().
I'd like it if exp_mavg() had a few sentences in the comment explaining
the underlying principles.
In update_moving_averages(), I don't think that the casts to 'float' are
necessary.
In update_moving_averages(), this calculation looks wrong to me:
> + /* Update daily average on the hour boundaries. */
> + if ((ofproto->last_minute - ofproto->created) % min_ms == 60) {
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
