From 2f9dd77fcd172e2870d737ede67970836db3eb14 Mon Sep 17 00:00:00 2001 From: Pravin B Shelar Date: Fri, 12 Sep 2014 16:00:50 -0700 Subject: [PATCH] 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 Acked-by: Ben Pfaff --- NEWS | 1 + lib/netdev-bsd.c | 1 - lib/netdev-dpdk.c | 32 +++++++---------------- lib/netdev-dummy.c | 13 ---------- lib/netdev-linux.c | 41 +----------------------------- lib/netdev-provider.h | 8 ------ lib/netdev-vport.c | 1 - lib/netdev.c | 13 ---------- lib/netdev.h | 1 - ofproto/bond.c | 59 ------------------------------------------- ofproto/bond.h | 2 -- vswitchd/bridge.c | 2 -- 12 files changed, 11 insertions(+), 163 deletions(-) diff --git a/NEWS b/NEWS index 274659488..6cbb315c2 100644 --- 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 index 32c04a38c..5b012996a 100644 --- 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 index 4f9c5c2a2..8f1fdb5b6 100644 --- 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 @@ unlock_dpdk: 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 index 1334a674f..ca048121d 100644 --- 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 index e31112253..084af4e09 100644 --- 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 index c08ef3519..7f266fdc5 100644 --- 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 index 6ab90bf04..c258f6bac 100644 --- 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 index 0d065e745..fb176260a 100644 --- 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 index c6249c452..1f91d9ae5 100644 --- 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 index 2d04b4392..cb6b895ed 100644 --- 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 index 0d9a67a2a..c487ec201 100644 --- 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 index 8f99d7dd4..1060719ab 100644 --- 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); -- 2.20.1