[ovs-dev] [PATCH] Update fake bond devices' statistics with the sum of bond slaves' stats.

Jesse Gross jesse at nicira.com
Tue Apr 13 23:14:55 UTC 2010


On Tue, Apr 13, 2010 at 6:16 PM, Ben Pfaff <blp at nicira.com> wrote:

> Needed by XAPI to accurately report bond statistics.
>
> Ugh.
>
> Bug NIC-63.
>

This is really ugly but it is the same as what I was planning on doing to
solve this problem.

Unfortunately this is all one big merge conflict with my stuff.

@@ -18,6 +24,12 @@ struct dp_dev {
>        struct net_device *dev;
>        struct net_device_stats stats;
>        struct pcpu_lstats *lstats;
> +
> +       /* This is warty support for XAPI, which does not support summing
> bond
> +        * device statistics itself.  The array lists ifindexes of network
> +        * devices whose statistics should be added to our own, so that
> +        * e.g. bond0's stats can be the sum of eth0 and eth1.  */
> +       struct pcpu_lstats extra_stats;
>  };
>

I'm guessing that this comment was from the way that you were originally
planning on implementing this.


> +
> +struct dp_dev_stats {
> +       __u64 rx_packets;
> +       __u64 rx_bytes;
> +       __u64 tx_packets;
> +       __u64 tx_bytes;
> +};
>

Obviously this doesn't do errors but then neither does the dp_dev.


> @@ -84,7 +85,7 @@ enum {
>     VALID_IN6 = 1 << 3,
>     VALID_MTU = 1 << 4,
>     VALID_CARRIER = 1 << 5,
> -    VALID_IS_INTERNAL = 1 << 6
> +    VALID_IS_XXX = 1 << 6       /* Represents is_internal and is_tap. */
>  };
>

I think we really need to find a better name than _XXX.  Maybe _PSEUDO?


> +/* Brings the 'is_internal' and 'is_tap' members of 'netdev_dev'
> up-to-date. */
> +static void
> +netdev_linux_update_is_xxx(struct netdev *netdev,
> +                           struct netdev_dev_linux *netdev_dev)
> +{
> +    if (!(netdev_dev->cache_valid & VALID_IS_XXX)) {
> +        netdev_dev->is_internal = false;
> +        netdev_dev->is_tap = !strcmp(netdev_get_type(netdev), "tap");
>

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.


> +    /* We must reject this call if 'netdev' is not an Open vSwitch
> internal
> +     * port, because the ioctl that we are about to execute is in the
> "device
> +     * private ioctls" range, which means that executing it on a device
> that
> +     * is not the type we expect could do any random thing.
> +     *
> +     * (Amusingly, these ioctl numbers are commented "THESE IOCTLS ARE
> +     * _DEPRECATED_ AND WILL DISAPPEAR IN 2.5.X" in linux/sockios.h.  I
> guess
> +     * DaveM is a little behind on that.) */
>

Hmm, I don't they are going away anytime soon...


> @@ -2036,6 +2081,7 @@ const struct netdev_class netdev_tap_class = {
>     netdev_linux_get_ifindex,
>     netdev_linux_get_carrier,
>     netdev_linux_get_stats,
> +    netdev_linux_set_stats,
>
>     netdev_linux_get_features,
>     netdev_linux_set_advertisements,
> @@ -2084,6 +2130,7 @@ const struct netdev_class netdev_gre_class = {
>     netdev_linux_get_ifindex,
>     netdev_linux_get_carrier,
>     netdev_linux_get_stats,
> +    netdev_linux_set_stats,
>
>
Only "system" devices can be internal so there is no need for the set_stats
function in the tap and GRE classes.


> +    /* Sets the device stats for 'netdev' to 'stats'.
> +     *
> +     * Most network devices won't support this feature and will set this
> +     * function pointer to 0, which is equivalent to returning EOPNOTSUPP.
> +     * Additional, some network devices might only allow setting their
> stats to
> +     * 0. */
> +    int (*set_stats)(struct netdev *netdev, const struct netdev_stats *);
> +
>

Obviously it's the same thing, but it seems a little more canonical to refer
to setting a pointer to null rather than 0.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20100413/21522ff5/attachment-0003.html>


More information about the dev mailing list