From f4b6076acab233cfe02e7eaefdeafbb69dfae556 Mon Sep 17 00:00:00 2001 From: Jesse Gross Date: Wed, 9 Jun 2010 12:54:34 -0700 Subject: [PATCH] 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. --- include/openvswitch/automake.mk | 1 - include/openvswitch/internal_dev.h | 66 ------------------------------ lib/netdev-gre.c | 2 +- lib/netdev-linux.c | 38 +---------------- lib/netdev-patch.c | 2 +- lib/netdev-vport.c | 34 +++++++++++++++ lib/netdev-vport.h | 1 + vswitchd/bridge.c | 17 ++++++-- 8 files changed, 51 insertions(+), 110 deletions(-) delete mode 100644 include/openvswitch/internal_dev.h diff --git a/include/openvswitch/automake.mk b/include/openvswitch/automake.mk index 3cc83d87..92e07188 100644 --- 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 index 26c73598..00000000 --- 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 -#else -#include -#endif - -#include - -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 55150d8e..0aa587a2 100644 --- 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 31d01780..c0bb2474 100644 --- 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 22353a19..17c509f1 100644 --- 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 a81262a6..58858f90 100644 --- 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 cb467f07..b4016606 100644 --- 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 b43a9bf6..feb1c5d8 100644 --- 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; } } -- 2.30.2