On Fri, Oct 11, 2013 at 4:09 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Fri, Oct 11, 2013 at 03:55:07PM -0700, Andy Zhou wrote:
> >
> > Signed-off-by: Andy Zhou <az...@nicira.com>
>
> It would be nice to add something to ovs-dpctl(8) to explain these new
> stats (as well as the old ones, for that matter).
>
> I would usually wrap this:
> struct ovs_dp_megaflow_stats
> megaflow_stats; /* OVS_DP_ATTR_MEGAFLOW_STATS. */
> as:
> struct ovs_dp_megaflow_stats megaflow_stats;
> /* OVS_DP_ATTR_MEGAFLOW_STATS. */
>
> O.K. Will change to match.
> In dpif_linux_dp_from_ofpbuf(), here we might repeat the same comment
> as for the OVS_DP_ATTR_STATS case immediately above it, just to hammer
> the point home:
> > + if (a[OVS_DP_ATTR_MEGAFLOW_STATS]) {
> > + memcpy(&dp->megaflow_stats,
> nl_attr_get(a[OVS_DP_ATTR_MEGAFLOW_STATS]),
> > + sizeof dp->megaflow_stats);
> > + }
> > +
> > return 0;
> > }
>
> O.K. Will add it back. I removed it to avoid repetition.
> Here we test n_masks against 0 and UINT64_MAX, presumably in case the
> datapath doesn't support megaflows, but I didn't notice code that
> initializes n_masks to UINT64_MAX (or to 0) if the attribute is
> missing (maybe I just missed it):
> > + if (stats.n_masks && stats.n_masks != UINT64_MAX) {
> > + uint64_t n_pkts = stats.n_hit + stats.n_missed;
> > + double avg = (double) stats.n_mask_hit / n_pkts;
> > +
> > + printf("\tmasks: hit:%"PRIu64" total:%"PRIu64"
> hit/pkt:%.2f\n",
> > + stats.n_mask_hit, stats.n_masks, avg);
> > + }
> > }
>
Actually, 0 should be supported, in case number of masks reduce to 0 when
all flow expires, but
the stats can still be useful. I will re-write this logic and also address
the initialization bug.
>
> I don't see an update to dpif-netdev. I guess that at least
> dpif_netdev_get_stats() should set the new members to 0 or UINT64_MAX.
>
> The new stats don't really apply to netdev. It is still a good idea to
initialize it as you pointed out.
> Thanks,
>
> Ben.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
