netdev-vport: Use vport set_stats instead of internal dev.

In certain cases we require the ability to provide stats that are
added to the values collected by the kernel (currently only used
by bond fake devices).  Internal devices previously implemented
this directly but now that their stats are now handled by the vport
layer the functionality has been moved there.  This removes the
userspace code to set the stats and replaces it with a mechanism
to access the equivalent functionality in the vport layer.

diff --git a/include/openvswitch/automake.mk b/include/openvswitch/automake.mk
index 3cc83d8..92e0718 100644 (file)
--- a/include/openvswitch/automake.mk
+++ b/include/openvswitch/automake.mk
@@ -1,6 +1,5 @@
 noinst_HEADERS += \
        include/openvswitch/gre.h \
        include/openvswitch/brcompat-netlink.h \
-       include/openvswitch/internal_dev.h \
        include/openvswitch/datapath-protocol.h
 

diff --git a/include/openvswitch/internal_dev.h b/include/openvswitch/internal_dev.h
deleted file mode 100644 (file)
index 26c7359..0000000
--- a/include/openvswitch/internal_dev.h
+++ /dev/null
@@ -1,66 +0,0 @@
-/*
- * Copyright (c) 2010 Nicira Networks.
- *
- * This file is offered under your choice of two licenses: Apache 2.0 or GNU
- * GPL 2.0 or later.  The permission statements for each of these licenses is
- * given below.  You may license your modifications to this file under either
- * of these licenses or both.  If you wish to license your modifications under
- * only one of these licenses, delete the permission text for the other
- * license.
- *
- * ----------------------------------------------------------------------
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at:
- *
- *     http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- * ----------------------------------------------------------------------
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write to the Free Software Foundation, Inc.,
- * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
- * ----------------------------------------------------------------------
- */
-
-/* Ioctl for Open vSwitch "internal ports" to support XAPI, which does not
- * support summing statistics from bond slaves, but still needs to get bond
- * statistics.
- *
- * This is a nasty wart that needs removing. */
-
-#ifndef OPENVSWITCH_INTERNAL_DEV_H
-#define OPENVSWITCH_INTERNAL_DEV_H 1
-
-#ifdef __KERNEL__
-#include <linux/types.h>
-#else
-#include <sys/types.h>
-#endif
-
-#include <linux/sockios.h>
-
-struct internal_dev_stats {
-       __u64 rx_packets;
-       __u64 rx_bytes;
-       __u64 tx_packets;
-       __u64 tx_bytes;
-};
-
-#define INTERNAL_DEV_SET_STATS        SIOCDEVPRIVATE
-
-#endif /* openvswitch/internal_dev.h */

diff --git a/lib/netdev-gre.c b/lib/netdev-gre.c
index 55150d8..0aa587a 100644 (file)
--- a/lib/netdev-gre.c
+++ b/lib/netdev-gre.c
@@ -251,7 +251,7 @@ const struct netdev_class netdev_gre_class = {
     NULL,                       /* get_ifindex */
     netdev_vport_get_carrier,
     netdev_vport_get_stats,
-    NULL,                       /* set_stats */
+    netdev_vport_set_stats,
 
     NULL,                       /* get_features */
     NULL,                       /* set_advertisements */

diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 31d0178..c0bb247 100644 (file)
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -52,7 +52,6 @@
 #include "netlink.h"
 #include "ofpbuf.h"
 #include "openflow/openflow.h"
-#include "openvswitch/internal_dev.h"
 #include "openvswitch/gre.h"
 #include "packets.h"
 #include "poll-loop.h"
@@ -863,41 +862,6 @@ netdev_linux_get_stats(const struct netdev *netdev_,
     return error;
 }
 
-static int
-netdev_linux_set_stats(struct netdev *netdev,
-                       const struct netdev_stats *stats)
-{
-    struct netdev_dev_linux *netdev_dev =
-        netdev_dev_linux_cast(netdev_get_dev(netdev));
-    struct internal_dev_stats dp_dev_stats;
-    struct ifreq ifr;
-
-    /* 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.) */
-    netdev_linux_update_is_pseudo(netdev_dev);
-    if (!netdev_dev->is_internal) {
-        return EOPNOTSUPP;
-    }
-
-    /* This actually only sets the *offset* that the dp_dev applies, but in our
-     * usage for fake bond devices the dp_dev never has any traffic of it own
-     * so it has the same effect. */
-    dp_dev_stats.rx_packets = stats->rx_packets;
-    dp_dev_stats.rx_bytes = stats->rx_bytes;
-    dp_dev_stats.tx_packets = stats->tx_packets;
-    dp_dev_stats.tx_bytes = stats->tx_bytes;
-    ifr.ifr_data = (void *) &dp_dev_stats;
-    return netdev_linux_do_ioctl(netdev_get_name(netdev), &ifr,
-                                 INTERNAL_DEV_SET_STATS,
-                                 "INTERNAL_DEV_SET_STATS");
-}
-
 /* Stores the features supported by 'netdev' into each of '*current',
  * '*advertised', '*supported', and '*peer' that are non-null.  Each value is a
  * bitmap of "enum ofp_port_features" bits, in host byte order.  Returns 0 if
@@ -1633,7 +1597,7 @@ const struct netdev_class netdev_linux_class = {
     netdev_linux_get_ifindex,
     netdev_linux_get_carrier,
     netdev_linux_get_stats,
-    netdev_linux_set_stats,
+    netdev_vport_set_stats,
 
     netdev_linux_get_features,
     netdev_linux_set_advertisements,

diff --git a/lib/netdev-patch.c b/lib/netdev-patch.c
index 22353a1..17c509f 100644 (file)
--- a/lib/netdev-patch.c
+++ b/lib/netdev-patch.c
@@ -202,7 +202,7 @@ const struct netdev_class netdev_patch_class = {
     NULL,                       /* get_ifindex */
     netdev_vport_get_carrier,
     netdev_vport_get_stats,
-    NULL,                       /* set_stats */
+    netdev_vport_set_stats,
 
     NULL,                       /* get_features */
     NULL,                       /* set_advertisements */

diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
index a81262a..58858f9 100644 (file)
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -158,6 +158,40 @@ netdev_vport_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
     return 0;
 }
 
+int
+netdev_vport_set_stats(struct netdev *netdev, const struct netdev_stats *stats)
+{
+    struct odp_vport_stats_req ovsr;
+    int err;
+
+    ovs_strlcpy(ovsr.devname, netdev_get_name(netdev), sizeof ovsr.devname);
+
+    ovsr.stats.rx_packets = stats->rx_packets;
+    ovsr.stats.tx_packets = stats->tx_packets;
+    ovsr.stats.rx_bytes = stats->rx_bytes;
+    ovsr.stats.tx_bytes = stats->tx_bytes;
+    ovsr.stats.rx_errors = stats->rx_errors;
+    ovsr.stats.tx_errors = stats->tx_errors;
+    ovsr.stats.rx_dropped = stats->rx_dropped;
+    ovsr.stats.tx_dropped = stats->tx_dropped;
+    ovsr.stats.collisions = stats->collisions;
+    ovsr.stats.rx_over_err = stats->rx_over_errors;
+    ovsr.stats.rx_crc_err = stats->rx_crc_errors;
+    ovsr.stats.rx_frame_err = stats->rx_frame_errors;
+
+    err = netdev_vport_do_ioctl(ODP_VPORT_STATS_SET, &ovsr);
+
+    /* If the vport layer doesn't know about the device, that doesn't mean it
+     * doesn't exist (after all were able to open it when netdev_open() was
+     * called), it just means that it isn't attached and we'll be getting
+     * stats a different way. */
+    if (err == ENODEV) {
+        err = EOPNOTSUPP;
+    }
+
+    return err;
+}
+
 int
 netdev_vport_update_flags(struct netdev *netdev OVS_UNUSED,
                         enum netdev_flags off, enum netdev_flags on OVS_UNUSED,

diff --git a/lib/netdev-vport.h b/lib/netdev-vport.h
index cb467f0..b401660 100644 (file)
--- a/lib/netdev-vport.h
+++ b/lib/netdev-vport.h
@@ -29,6 +29,7 @@ int netdev_vport_get_etheraddr(const struct netdev *,
 int netdev_vport_get_mtu(const struct netdev *, int *mtup);
 int netdev_vport_get_carrier(const struct netdev *, bool *carrier);
 int netdev_vport_get_stats(const struct netdev *, struct netdev_stats *);
+int netdev_vport_set_stats(struct netdev *, const struct netdev_stats *);
 int netdev_vport_update_flags(struct netdev *, enum netdev_flags off,
                               enum netdev_flags on,
                               enum netdev_flags *old_flagsp);

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index b43a9bf..feb1c5d 100644 (file)
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1927,10 +1927,19 @@ bond_update_fake_iface_stats(struct port *port)
         struct netdev_stats slave_stats;
 
         if (!netdev_get_stats(port->ifaces[i]->netdev, &slave_stats)) {
-            bond_stats.rx_packets += slave_stats.rx_packets;
-            bond_stats.rx_bytes += slave_stats.rx_bytes;
-            bond_stats.tx_packets += slave_stats.tx_packets;
-            bond_stats.tx_bytes += slave_stats.tx_bytes;
+            /* XXX: We swap the stats here because they are swapped back when
+             * reported by the internal device.  The reason for this is
+             * internal devices normally represent packets going into the system
+             * but when used as fake bond device they represent packets leaving
+             * the system.  We really should do this in the internal device
+             * itself because changing it here reverses the counts from the
+             * perspective of the switch.  However, the internal device doesn't
+             * know what type of device it represents so we have to do it here
+             * for now. */
+            bond_stats.tx_packets += slave_stats.rx_packets;
+            bond_stats.tx_bytes += slave_stats.rx_bytes;
+            bond_stats.rx_packets += slave_stats.tx_packets;
+            bond_stats.rx_bytes += slave_stats.tx_bytes;
         }
     }