On Tue, Feb 28, 2012 at 5:09 PM, Ben Pfaff <b...@nicira.com> wrote: > On Fri, Feb 24, 2012 at 04:10:02PM -0800, Pravin B Shelar wrote: >> There is no need to retrieve linux system stats for internal devices >> as all relevant stats for virtual device like internal device are >> already reported by OVS over vport-stats. >> >> Signed-off-by: Pravin B Shelar <pshe...@nicira.com> > > It looks OK to me. Thanks. > > Is this really a fix, though, as the first line of the commit message > says? I don't think that there is a bug, except a performance, so > maybe "Optimize" instead of "Fix"?
ok, I forgot to mention. It does fixes a bug as it does not count error stats twice for internal devices. > > Also, I'd start the commit message with "netdev-linux:" instead of > "netdev:" because it only changes Linux-specific netdev code. > ok. > It would be nice to put a comment on the "vport_stats_error" member, > maybe just as simple as /* 0 or an errno value. */, since it took me a > moment to understand its meaning. > ok > I noticed an oddity in existing code while I was reading it. It looks > like netdev_linux_get_stats() will always log a (rate-limited) error > the first time that we call it on any given netdev that is not a vport > (or each time the cache gets invalidated). Does it look like that to > you, too? Maybe it doesn't show up because we only call this function > on netdevs that are vports. > I am not sure in which case vport wont be there for given netdev. > Some of your new code uses the new return value from > get_stats_via_vport(), some of your new code uses > netdev_dev->vport_stats_error after calling get_stats_via_vport(). It > is a little inconsistent. I would be inclined to not add the new > return value and use netdev_dev->vport_stats_error in each case, but > the existing code looks correct and so it's not a big deal. > ok, I fixed it. I will send out v2. Thanks, Pravin. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
