On Tue, Apr 13, 2010 at 6:16 PM, Ben Pfaff <span dir="ltr"><<a href="mailto:blp@nicira.com">blp@nicira.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Needed by XAPI to accurately report bond statistics.<br>
<br>
Ugh.<br>
<br>
Bug NIC-63.<br></blockquote><div><br></div><div><div>This is really ugly but it is the same as what I was planning on doing to solve this problem.</div><div><br></div><div>Unfortunately this is all one big merge conflict with my stuff.</div>
</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
@@ -18,6 +24,12 @@ struct dp_dev {<br>
struct net_device *dev;<br>
struct net_device_stats stats;<br>
struct pcpu_lstats *lstats;<br>
+<br>
+ /* This is warty support for XAPI, which does not support summing bond<br>
+ * device statistics itself. The array lists ifindexes of network<br>
+ * devices whose statistics should be added to our own, so that<br>
+ * e.g. bond0's stats can be the sum of eth0 and eth1. */<br>
+ struct pcpu_lstats extra_stats;<br>
};<br></blockquote><div><br></div><div>I'm guessing that this comment was from the way that you were originally planning on implementing this.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+<br>
+struct dp_dev_stats {<br>
+ __u64 rx_packets;<br>
+ __u64 rx_bytes;<br>
+ __u64 tx_packets;<br>
+ __u64 tx_bytes;<br>
+};<br></blockquote><div><br></div><div>Obviously this doesn't do errors but then neither does the dp_dev.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
@@ -84,7 +85,7 @@ enum {<br>
VALID_IN6 = 1 << 3,<br>
VALID_MTU = 1 << 4,<br>
VALID_CARRIER = 1 << 5,<br>
- VALID_IS_INTERNAL = 1 << 6<br>
+ VALID_IS_XXX = 1 << 6 /* Represents is_internal and is_tap. */<br>
};<br></blockquote><div><br></div><div>I think we really need to find a better name than _XXX. Maybe _PSEUDO?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+/* Brings the 'is_internal' and 'is_tap' members of 'netdev_dev' up-to-date. */<br>
+static void<br>
+netdev_linux_update_is_xxx(struct netdev *netdev,<br>
+ struct netdev_dev_linux *netdev_dev)<br>
+{<br>
+ if (!(netdev_dev->cache_valid & VALID_IS_XXX)) {<br>
+ netdev_dev->is_internal = false;<br>
+ netdev_dev->is_tap = !strcmp(netdev_get_type(netdev), "tap");<br></blockquote><div><br></div><div>Not that it really matters but there is also a netdev_dev_get_type as well. I think this is the only reason why you need to get and pass around netdev.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+ /* We must reject this call if 'netdev' is not an Open vSwitch internal<br>
+ * port, because the ioctl that we are about to execute is in the "device<br>
+ * private ioctls" range, which means that executing it on a device that<br>
+ * is not the type we expect could do any random thing.<br>
+ *<br>
+ * (Amusingly, these ioctl numbers are commented "THESE IOCTLS ARE<br>
+ * _DEPRECATED_ AND WILL DISAPPEAR IN 2.5.X" in linux/sockios.h. I guess<br>
+ * DaveM is a little behind on that.) */<br></blockquote><div><br></div><div>Hmm, I don't they are going away anytime soon...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
@@ -2036,6 +2081,7 @@ const struct netdev_class netdev_tap_class = {<br>
netdev_linux_get_ifindex,<br>
netdev_linux_get_carrier,<br>
netdev_linux_get_stats,<br>
+ netdev_linux_set_stats,<br>
<br>
netdev_linux_get_features,<br>
netdev_linux_set_advertisements,<br>
@@ -2084,6 +2130,7 @@ const struct netdev_class netdev_gre_class = {<br>
netdev_linux_get_ifindex,<br>
netdev_linux_get_carrier,<br>
netdev_linux_get_stats,<br>
+ netdev_linux_set_stats,<br>
<br></blockquote><div><br></div><div>Only "system" devices can be internal so there is no need for the set_stats function in the tap and GRE classes.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+ /* Sets the device stats for 'netdev' to 'stats'.<br>
+ *<br>
+ * Most network devices won't support this feature and will set this<br>
+ * function pointer to 0, which is equivalent to returning EOPNOTSUPP.<br>
+ * Additional, some network devices might only allow setting their stats to<br>
+ * 0. */<br>
+ int (*set_stats)(struct netdev *netdev, const struct netdev_stats *);<br>
+<br></blockquote><div><br></div><div>Obviously it's the same thing, but it seems a little more canonical to refer to setting a pointer to null rather than 0.</div></div>