ofproto: Do not update stats on fake bond interface.

There are couple of reasons to remove this support: * This is used in very old OVS use-case. It is much better to read stats directly from OVS. * Forthcoming commit will remove support for setting stats for vport. The stats update depends on stats-set. Signed-off-by: Pravin B Shelar <[email protected]> Acked-by: Ben Pfaff <[email protected]>
krjoshi · Sep 15, 2014 · 2f9dd77 · 2f9dd77
1 parent 00e01dd
commit 2f9dd77
Show file tree

Hide file tree

Showing 12 changed files with 11 additions and 163 deletions.
diff --git a/NEWS b/NEWS
@@ -31,6 +31,7 @@ Post-v2.3.0
    - Experimental support for the Rapid Spanning Tree Protocol
      (IEEE 802.1D-2004).  More conformance and interoperability testing is
      still needed, so this should not be enabled on production environments.
+   - Stats are no longer updated on fake bond interface.
 
 
 v2.3.0 - 14 Aug 2014

diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
@@ -1576,7 +1576,6 @@ netdev_bsd_update_flags(struct netdev *netdev_, enum netdev_flags off,
     NULL, /* get_carrier_resets */                   \
     NULL, /* set_miimon_interval */                  \
     netdev_bsd_get_stats,                            \
-    NULL, /* set_stats */                            \
                                                      \
     GET_FEATURES,                                    \
     NULL, /* set_advertisement */                    \

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
@@ -190,7 +190,6 @@ struct netdev_dpdk {
     int mtu;
     int socket_id;
     int buf_size;
-    struct netdev_stats stats_offset;
     struct netdev_stats stats;
 
     uint8_t hwaddr[ETH_ADDR_LEN];
@@ -950,29 +949,17 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
     ovs_mutex_lock(&dev->mutex);
     rte_eth_stats_get(dev->port_id, &rte_stats);
 
-    *stats = dev->stats_offset;
+    memset(stats, 0, sizeof(*stats));
 
-    stats->rx_packets += rte_stats.ipackets;
-    stats->tx_packets += rte_stats.opackets;
-    stats->rx_bytes += rte_stats.ibytes;
-    stats->tx_bytes += rte_stats.obytes;
-    stats->rx_errors += rte_stats.ierrors;
-    stats->tx_errors += rte_stats.oerrors;
-    stats->multicast += rte_stats.imcasts;
+    stats->rx_packets = rte_stats.ipackets;
+    stats->tx_packets = rte_stats.opackets;
+    stats->rx_bytes = rte_stats.ibytes;
+    stats->tx_bytes = rte_stats.obytes;
+    stats->rx_errors = rte_stats.ierrors;
+    stats->tx_errors = rte_stats.oerrors;
+    stats->multicast = rte_stats.imcasts;
 
-    stats->tx_dropped += dev->stats.tx_dropped;
-    ovs_mutex_unlock(&dev->mutex);
-
-    return 0;
-}
-
-static int
-netdev_dpdk_set_stats(struct netdev *netdev, const struct netdev_stats *stats)
-{
-    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
-
-    ovs_mutex_lock(&dev->mutex);
-    dev->stats_offset = *stats;
+    stats->tx_dropped = dev->stats.tx_dropped;
     ovs_mutex_unlock(&dev->mutex);
 
     return 0;
@@ -1367,7 +1354,6 @@ netdev_dpdk_ring_construct(struct netdev *netdev)
     netdev_dpdk_get_carrier_resets,                           \
     netdev_dpdk_set_miimon,                                   \
     netdev_dpdk_get_stats,                                    \
-    netdev_dpdk_set_stats,                                    \
     netdev_dpdk_get_features,                                 \
     NULL,                       /* set_advertisements */      \
                                                               \

diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
@@ -969,18 +969,6 @@ netdev_dummy_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
     return 0;
 }
 
-static int
-netdev_dummy_set_stats(struct netdev *netdev, const struct netdev_stats *stats)
-{
-    struct netdev_dummy *dev = netdev_dummy_cast(netdev);
-
-    ovs_mutex_lock(&dev->mutex);
-    dev->stats = *stats;
-    ovs_mutex_unlock(&dev->mutex);
-
-    return 0;
-}
-
 static int
 netdev_dummy_get_ifindex(const struct netdev *netdev)
 {
@@ -1058,7 +1046,6 @@ static const struct netdev_class dummy_class = {
     NULL,                       /* get_carrier_resets */
     NULL,                       /* get_miimon */
     netdev_dummy_get_stats,
-    netdev_dummy_set_stats,
 
     NULL,                       /* get_features */
     NULL,                       /* set_advertisements */

diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
@@ -1691,41 +1691,6 @@ netdev_internal_get_stats(const struct netdev *netdev_,
     return error;
 }
 
-static int
-netdev_internal_set_stats(struct netdev *netdev,
-                          const struct netdev_stats *stats)
-{
-    struct ovs_vport_stats vport_stats;
-    struct dpif_linux_vport vport;
-    int err;
-
-    put_32aligned_u64(&vport_stats.rx_packets, stats->rx_packets);
-    put_32aligned_u64(&vport_stats.tx_packets, stats->tx_packets);
-    put_32aligned_u64(&vport_stats.rx_bytes, stats->rx_bytes);
-    put_32aligned_u64(&vport_stats.tx_bytes, stats->tx_bytes);
-    put_32aligned_u64(&vport_stats.rx_errors, stats->rx_errors);
-    put_32aligned_u64(&vport_stats.tx_errors, stats->tx_errors);
-    put_32aligned_u64(&vport_stats.rx_dropped, stats->rx_dropped);
-    put_32aligned_u64(&vport_stats.tx_dropped, stats->tx_dropped);
-
-    dpif_linux_vport_init(&vport);
-    vport.cmd = OVS_VPORT_CMD_SET;
-    vport.name = netdev_get_name(netdev);
-    vport.stats = &vport_stats;
-
-    err = dpif_linux_vport_transact(&vport, NULL, NULL);
-
-    /* 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;
-}
-
 static void
 netdev_linux_read_features(struct netdev_linux *netdev)
 {
@@ -2734,7 +2699,7 @@ netdev_linux_update_flags(struct netdev *netdev_, enum netdev_flags off,
     return error;
 }
 
-#define NETDEV_LINUX_CLASS(NAME, CONSTRUCT, GET_STATS, SET_STATS,  \
+#define NETDEV_LINUX_CLASS(NAME, CONSTRUCT, GET_STATS,          \
                            GET_FEATURES, GET_STATUS)            \
 {                                                               \
     NAME,                                                       \
@@ -2764,7 +2729,6 @@ netdev_linux_update_flags(struct netdev *netdev_, enum netdev_flags off,
     netdev_linux_get_carrier_resets,                            \
     netdev_linux_set_miimon_interval,                           \
     GET_STATS,                                                  \
-    SET_STATS,                                                  \
                                                                 \
     GET_FEATURES,                                               \
     netdev_linux_set_advertisements,                            \
@@ -2807,7 +2771,6 @@ const struct netdev_class netdev_linux_class =
         "system",
         netdev_linux_construct,
         netdev_linux_get_stats,
-        NULL,                    /* set_stats */
         netdev_linux_get_features,
         netdev_linux_get_status);
 
@@ -2816,7 +2779,6 @@ const struct netdev_class netdev_tap_class =
         "tap",
         netdev_linux_construct_tap,
         netdev_tap_get_stats,
-        NULL,                   /* set_stats */
         netdev_linux_get_features,
         netdev_linux_get_status);
 
@@ -2825,7 +2787,6 @@ const struct netdev_class netdev_internal_class =
         "internal",
         netdev_linux_construct,
         netdev_internal_get_stats,
-        netdev_internal_set_stats,
         NULL,                  /* get_features */
         netdev_internal_get_status);
 

diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h
@@ -371,14 +371,6 @@ struct netdev_class {
      * (UINT64_MAX). */
     int (*get_stats)(const struct netdev *netdev, struct netdev_stats *);
 
-    /* Sets the device stats for 'netdev' to 'stats'.
-     *
-     * Most network devices won't support this feature and will set this
-     * function pointer to NULL, which is equivalent to returning EOPNOTSUPP.
-     *
-     * Some network devices might only allow setting their stats to 0. */
-    int (*set_stats)(struct netdev *netdev, const struct netdev_stats *);
-
     /* Stores the features supported by 'netdev' into each of '*current',
      * '*advertised', '*supported', and '*peer'.  Each value is a bitmap of
      * NETDEV_F_* bits.

diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
@@ -789,7 +789,6 @@ get_stats(const struct netdev *netdev, struct netdev_stats *stats)
     NULL,                       /* get_carrier_resets */    \
     NULL,                       /* get_miimon */            \
     get_stats,                                              \
-    NULL,                       /* set_stats */             \
                                                             \
     NULL,                       /* get_features */          \
     NULL,                       /* set_advertisements */    \

diff --git a/lib/netdev.c b/lib/netdev.c
@@ -1239,19 +1239,6 @@ netdev_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
     return error;
 }
 
-/* Attempts to change the stats for 'netdev' to those provided in 'stats'.
- * Returns 0 if successful, otherwise a positive errno value.
- *
- * This will probably fail for most network devices.  Some devices might only
- * allow setting their stats to 0. */
-int
-netdev_set_stats(struct netdev *netdev, const struct netdev_stats *stats)
-{
-    return (netdev->netdev_class->set_stats
-             ? netdev->netdev_class->set_stats(netdev, stats)
-             : EOPNOTSUPP);
-}
-
 /* Attempts to set input rate limiting (policing) policy, such that up to
  * 'kbits_rate' kbps of traffic is accepted, with a maximum accumulative burst
  * size of 'kbits' kb. */

diff --git a/lib/netdev.h b/lib/netdev.h
@@ -251,7 +251,6 @@ struct netdev *netdev_find_dev_by_in4(const struct in_addr *);
 
 /* Statistics. */
 int netdev_get_stats(const struct netdev *, struct netdev_stats *);
-int netdev_set_stats(struct netdev *, const struct netdev_stats *);
 
 /* Quality of service. */
 struct netdev_qos_capabilities {

diff --git a/ofproto/bond.c b/ofproto/bond.c
@@ -132,7 +132,6 @@ struct bond {
     struct hmap pr_rule_ops;     /* Helps to maintain post recirculation rules.*/
 
     /* Legacy compatibility. */
-    long long int next_fake_iface_update; /* LLONG_MAX if disabled. */
     bool lacp_fallback_ab; /* Fallback to active-backup on LACP failure. */
 
     struct ovs_refcount ref_cnt;
@@ -177,8 +176,6 @@ static struct bond_slave *choose_output_slave(const struct bond *,
                                               struct flow_wildcards *,
                                               uint16_t vlan)
     OVS_REQ_RDLOCK(rwlock);
-static void bond_update_fake_slave_stats(struct bond *)
-    OVS_REQ_RDLOCK(rwlock);
 
 /* Attempts to parse 's' as the name of a bond balancing mode.  If successful,
  * stores the mode in '*balance' and returns true.  Otherwise returns false
@@ -228,7 +225,6 @@ bond_create(const struct bond_settings *s, struct ofproto_dpif *ofproto)
     hmap_init(&bond->slaves);
     list_init(&bond->enabled_slaves);
     ovs_mutex_init(&bond->mutex);
-    bond->next_fake_iface_update = LLONG_MAX;
     ovs_refcount_init(&bond->ref_cnt);
 
     bond->recirc_id = 0;
@@ -432,14 +428,6 @@ bond_reconfigure(struct bond *bond, const struct bond_settings *s)
         revalidate = true;
     }
 
-    if (s->fake_iface) {
-        if (bond->next_fake_iface_update == LLONG_MAX) {
-            bond->next_fake_iface_update = time_msec();
-        }
-    } else {
-        bond->next_fake_iface_update = LLONG_MAX;
-    }
-
     if (bond->bond_revalidate) {
         revalidate = true;
         bond->bond_revalidate = false;
@@ -611,12 +599,6 @@ bond_run(struct bond *bond, enum lacp_status lacp_status)
         bond_choose_active_slave(bond);
     }
 
-    /* Update fake bond interface stats. */
-    if (time_msec() >= bond->next_fake_iface_update) {
-        bond_update_fake_slave_stats(bond);
-        bond->next_fake_iface_update = time_msec() + 1000;
-    }
-
     revalidate = bond->bond_revalidate;
     bond->bond_revalidate = false;
     ovs_rwlock_unlock(&rwlock);
@@ -639,10 +621,6 @@ bond_wait(struct bond *bond)
         seq_wait(connectivity_seq_get(), slave->change_seq);
     }
 
-    if (bond->next_fake_iface_update != LLONG_MAX) {
-        poll_timer_wait_until(bond->next_fake_iface_update);
-    }
-
     if (bond->bond_revalidate) {
         poll_immediate_wake();
     }
@@ -1813,40 +1791,3 @@ bond_choose_active_slave(struct bond *bond)
         VLOG_INFO_RL(&rl, "bond %s: all interfaces disabled", bond->name);
     }
 }
-
-/* Attempts to make the sum of the bond slaves' statistics appear on the fake
- * bond interface. */
-static void
-bond_update_fake_slave_stats(struct bond *bond)
-{
-    struct netdev_stats bond_stats;
-    struct bond_slave *slave;
-    struct netdev *bond_dev;
-
-    memset(&bond_stats, 0, sizeof bond_stats);
-
-    HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
-        struct netdev_stats slave_stats;
-
-        if (!netdev_get_stats(slave->netdev, &slave_stats)) {
-            /* 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;
-        }
-    }
-
-    if (!netdev_open(bond->name, "system", &bond_dev)) {
-        netdev_set_stats(bond_dev, &bond_stats);
-        netdev_close(bond_dev);
-    }
-}
diff --git a/ofproto/bond.h b/ofproto/bond.h
@@ -52,8 +52,6 @@ struct bond_settings {
     int up_delay;               /* ms before enabling an up slave. */
     int down_delay;             /* ms before disabling a down slave. */
 
-    /* Legacy compatibility. */
-    bool fake_iface;            /* Update fake stats for netdev 'name'? */
     bool lacp_fallback_ab_cfg;  /* Fallback to active-backup on LACP failure. */
 };
 

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
@@ -3782,8 +3782,6 @@ port_configure_bond(struct port *port, struct bond_settings *s)
         s->rebalance_interval = 1000;
     }
 
-    s->fake_iface = port->cfg->bond_fake_iface;
-
     s->lacp_fallback_ab_cfg = smap_get_bool(&port->cfg->other_config,
                                        "lacp-fallback-ab", false);