[ovs-dev] [PATCH 2/2] dpif-linux: collect and display mega flow mask stats

[Andy Zhou]

On Fri, Oct 11, 2013 at 4:09 PM, Ben Pfaff <blp at nicira.com> wrote:

> On Fri, Oct 11, 2013 at 03:55:07PM -0700, Andy Zhou wrote:
> >
> > Signed-off-by: Andy Zhou <azhou at 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20131011/79f08d3e/attachment-0003.html>