Ben, Thanks for the review. I will send out the patch again after fixing
all issues you mentioned in the review.
On Fri, Mar 22, 2013 at 12:51 PM, Ben Pfaff <b...@nicira.com> wrote:
> 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
