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"? Also, I'd start the commit message with "netdev-linux:" instead of "netdev:" because it only changes Linux-specific netdev code. 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. 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. 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. Thanks, Ben. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
