This commit removes the 'Instant' stats related logic in bridge.c.
Instead, the corresponding status is updated immediately after the
global connectivity sequence number changes.  This will slightly
increase the update speed, since the 'Instant' stats update period
is 100 millisecond.  Moreover, the netdev's sequence number is used
to avoid updating unchanged netdev status.

In the experiment with 5K internal ports, when one port is flapping
at rate of every 0.3 second, there is no additional cpu consumption
observed.

Signed-off-by: Alex Wang <al...@nicira.com>

---
PATCH -> V2:
- refine the commit message.
- refine the comments.
---
 ofproto/ofproto-dpif.c |    2 +
 vswitchd/bridge.c      |  238 ++++++++++++++++++------------------------------
 2 files changed, 90 insertions(+), 150 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 04d5454..7f327b9 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -375,6 +375,8 @@ ofproto_dpif_send_packet_in(struct ofproto_dpif *ofproto,
         COVERAGE_INC(packet_in_overflow);
         free(CONST_CAST(void *, pin->up.packet));
         free(pin);
+    } else {
+        seq_change(connectivity_seq_get());
     }
 }
 
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 44fdb4e..ad7dc76 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -77,6 +77,7 @@ struct iface {
     char *name;                 /* Host network device name. */
     struct netdev *netdev;      /* Network device. */
     ofp_port_t ofp_port;        /* OpenFlow port number. */
+    uint64_t change_seq;
 
     /* These members are valid only within bridge_reconfigure(). */
     const char *type;           /* Usually same as cfg->type. */
@@ -263,7 +264,8 @@ static void iface_configure_qos(struct iface *, const 
struct ovsrec_qos *);
 static void iface_configure_cfm(struct iface *);
 static void iface_refresh_cfm_stats(struct iface *);
 static void iface_refresh_stats(struct iface *);
-static void iface_refresh_status(struct iface *);
+static void iface_refresh_netdev_status(struct iface *);
+static void iface_refresh_ofproto_status(struct iface *);
 static bool iface_is_synthetic(const struct iface *);
 static ofp_port_t iface_get_requested_ofp_port(
     const struct ovsrec_interface *);
@@ -1506,7 +1508,7 @@ iface_create(struct bridge *br, const struct 
ovsrec_interface *iface_cfg,
 
     /* Populate initial status in database. */
     iface_refresh_stats(iface);
-    iface_refresh_status(iface);
+    iface_refresh_netdev_status(iface);
 
     /* Add bond fake iface if necessary. */
     if (port_is_bond_fake_iface(port)) {
@@ -1764,22 +1766,27 @@ dpid_from_hash(const void *data, size_t n)
 }
 
 static void
-iface_refresh_status(struct iface *iface)
+iface_refresh_netdev_status(struct iface *iface)
 {
     struct smap smap;
 
     enum netdev_features current;
-    int64_t bps;
-    int mtu;
-    int64_t mtu_64;
+    enum netdev_flags flags;
+    const char *link_state;
     uint8_t mac[ETH_ADDR_LEN];
-    int64_t ifindex64;
-    int error;
+    int64_t bps, mtu_64, ifindex64, link_resets;
+    int mtu, error;
 
     if (iface_is_synthetic(iface)) {
         return;
     }
 
+    if (iface->change_seq == netdev_get_change_seq(iface->netdev)) {
+        return;
+    }
+
+    iface->change_seq = netdev_get_change_seq(iface->netdev);
+
     smap_init(&smap);
 
     if (!netdev_get_status(iface->netdev, &smap)) {
@@ -1790,6 +1797,21 @@ iface_refresh_status(struct iface *iface)
 
     smap_destroy(&smap);
 
+    error = netdev_get_flags(iface->netdev, &flags);
+    if (!error) {
+        const char *state = flags & NETDEV_UP ? "up" : "down";
+
+        ovsrec_interface_set_admin_state(iface->cfg, state);
+    } else {
+        ovsrec_interface_set_admin_state(iface->cfg, NULL);
+    }
+
+    link_state = netdev_get_carrier(iface->netdev) ? "up" : "down";
+    ovsrec_interface_set_link_state(iface->cfg, link_state);
+
+    link_resets = netdev_get_carrier_resets(iface->netdev);
+    ovsrec_interface_set_link_resets(iface->cfg, &link_resets, 1);
+
     error = netdev_get_features(iface->netdev, &current, NULL, NULL, NULL);
     bps = !error ? netdev_features_to_bps(current, 0) : 0;
     if (bps) {
@@ -1829,6 +1851,38 @@ iface_refresh_status(struct iface *iface)
     ovsrec_interface_set_ifindex(iface->cfg, &ifindex64, 1);
 }
 
+static void
+iface_refresh_ofproto_status(struct iface *iface)
+{
+    struct smap smap;
+    int current, error;
+
+    if (iface_is_synthetic(iface)) {
+        return;
+    }
+
+    current = ofproto_port_is_lacp_current(iface->port->bridge->ofproto,
+                                           iface->ofp_port);
+    if (current >= 0) {
+        bool bl = current;
+        ovsrec_interface_set_lacp_current(iface->cfg, &bl, 1);
+    } else {
+        ovsrec_interface_set_lacp_current(iface->cfg, NULL, 0);
+    }
+
+    iface_refresh_cfm_stats(iface);
+
+    smap_init(&smap);
+    error = ofproto_port_get_bfd_status(iface->port->bridge->ofproto,
+                                        iface->ofp_port, &smap);
+    /* No need to do the following work if there is no status change. */
+    if (error < 0) {
+        return;
+    }
+    ovsrec_interface_set_bfd_status(iface->cfg, &smap);
+    smap_destroy(&smap);
+}
+
 /* Writes 'iface''s CFM statistics to the database. 'iface' must not be
  * synthetic. */
 static void
@@ -2143,145 +2197,6 @@ refresh_controller_status(void)
     ofproto_free_ofproto_controller_info(&info);
 }
 
-/* "Instant" stats.
- *
- * Some information in the database must be kept as up-to-date as possible to
- * allow controllers to respond rapidly to network outages.  We call these
- * statistics "instant" stats.
- *
- * We wish to update these statistics every INSTANT_INTERVAL_MSEC milliseconds,
- * assuming that they've changed.  The only means we have to determine whether
- * they have changed are:
- *
- *   - Try to commit changes to the database.  If nothing changed, then
- *     ovsdb_idl_txn_commit() returns TXN_UNCHANGED, otherwise some other
- *     value.
- *
- *   - instant_stats_run() is called late in the run loop, after anything that
- *     might change any of the instant stats.
- *
- * We use these two facts together to avoid waking the process up every
- * INSTANT_INTERVAL_MSEC whether there is any change or not.
- */
-
-/* Minimum interval between writing updates to the instant stats to the
- * database. */
-#define INSTANT_INTERVAL_MSEC 100
-
-/* Current instant stats database transaction, NULL if there is no ongoing
- * transaction. */
-static struct ovsdb_idl_txn *instant_txn;
-
-/* Next time (in msec on monotonic clock) at which we will update the instant
- * stats.  */
-static long long int instant_next_txn = LLONG_MIN;
-
-/* True if the run loop has run since we last saw that the instant stats were
- * unchanged, that is, this is true if we need to wake up at 'instant_next_txn'
- * to refresh the instant stats. */
-static bool instant_stats_could_have_changed;
-
-static void
-instant_stats_run(void)
-{
-    enum ovsdb_idl_txn_status status;
-
-    instant_stats_could_have_changed = true;
-
-    if (!instant_txn) {
-        struct bridge *br;
-        uint64_t seq;
-
-        if (time_msec() < instant_next_txn) {
-            return;
-        }
-        instant_next_txn = time_msec() + INSTANT_INTERVAL_MSEC;
-
-        seq = seq_read(connectivity_seq_get());
-        if (seq == connectivity_seqno) {
-            return;
-        }
-        connectivity_seqno = seq;
-
-        instant_txn = ovsdb_idl_txn_create(idl);
-        HMAP_FOR_EACH (br, node, &all_bridges) {
-            struct iface *iface;
-            struct port *port;
-
-            br_refresh_stp_status(br);
-
-            HMAP_FOR_EACH (port, hmap_node, &br->ports) {
-                port_refresh_stp_status(port);
-            }
-
-            HMAP_FOR_EACH (iface, name_node, &br->iface_by_name) {
-                enum netdev_flags flags;
-                struct smap smap;
-                const char *link_state;
-                int64_t link_resets;
-                int current, error;
-
-                if (iface_is_synthetic(iface)) {
-                    continue;
-                }
-
-                current = ofproto_port_is_lacp_current(br->ofproto,
-                                                       iface->ofp_port);
-                if (current >= 0) {
-                    bool bl = current;
-                    ovsrec_interface_set_lacp_current(iface->cfg, &bl, 1);
-                } else {
-                    ovsrec_interface_set_lacp_current(iface->cfg, NULL, 0);
-                }
-
-                error = netdev_get_flags(iface->netdev, &flags);
-                if (!error) {
-                    const char *state = flags & NETDEV_UP ? "up" : "down";
-                    ovsrec_interface_set_admin_state(iface->cfg, state);
-                } else {
-                    ovsrec_interface_set_admin_state(iface->cfg, NULL);
-                }
-
-                link_state = netdev_get_carrier(iface->netdev) ? "up" : "down";
-                ovsrec_interface_set_link_state(iface->cfg, link_state);
-
-                link_resets = netdev_get_carrier_resets(iface->netdev);
-                ovsrec_interface_set_link_resets(iface->cfg, &link_resets, 1);
-
-                iface_refresh_cfm_stats(iface);
-
-                smap_init(&smap);
-                error = ofproto_port_get_bfd_status(br->ofproto, 
iface->ofp_port,
-                                                    &smap);
-                if (error < 0) {
-                    continue;
-                }
-                ovsrec_interface_set_bfd_status(iface->cfg, &smap);
-                smap_destroy(&smap);
-            }
-        }
-    }
-
-    status = ovsdb_idl_txn_commit(instant_txn);
-    if (status != TXN_INCOMPLETE) {
-        ovsdb_idl_txn_destroy(instant_txn);
-        instant_txn = NULL;
-    }
-    if (status == TXN_UNCHANGED) {
-        instant_stats_could_have_changed = false;
-    }
-}
-
-static void
-instant_stats_wait(void)
-{
-    if (instant_txn) {
-        ovsdb_idl_txn_wait(instant_txn);
-    } else if (instant_stats_could_have_changed) {
-        poll_timer_wait_until(instant_next_txn);
-    }
-}
-
 static void
 bridge_run__(void)
 {
@@ -2311,6 +2226,7 @@ bridge_run(void)
 
     bool vlan_splinters_changed;
     struct bridge *br;
+    uint64_t seq;
 
     ovsrec_open_vswitch_init(&null_cfg);
 
@@ -2431,7 +2347,6 @@ bridge_run(void)
 
                     LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
                         iface_refresh_stats(iface);
-                        iface_refresh_status(iface);
                     }
 
                     port_refresh_stp_stats(port);
@@ -2450,8 +2365,32 @@ bridge_run(void)
         iface_stats_timer = time_msec() + IFACE_STATS_INTERVAL;
     }
 
+    /* Check the need to update status. */
+    seq = seq_read(connectivity_seq_get());
+    if (seq != connectivity_seqno) {
+        struct ovsdb_idl_txn *txn;
+
+        connectivity_seqno = seq;
+        txn = ovsdb_idl_txn_create(idl);
+        HMAP_FOR_EACH (br, node, &all_bridges) {
+            struct port *port;
+
+            br_refresh_stp_status(br);
+            HMAP_FOR_EACH (port, hmap_node, &br->ports) {
+                struct iface *iface;
+
+                port_refresh_stp_status(port);
+                LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
+                    iface_refresh_netdev_status(iface);
+                    iface_refresh_ofproto_status(iface);
+                }
+            }
+        }
+        ovsdb_idl_txn_commit(txn);
+        ovsdb_idl_txn_destroy(txn); /* XXX */
+    }
+
     run_system_stats();
-    instant_stats_run();
 }
 
 void
@@ -2482,7 +2421,6 @@ bridge_wait(void)
     }
 
     system_stats_wait();
-    instant_stats_wait();
 }
 
 /* Adds some memory usage statistics for bridges into 'usage', for use with
-- 
1.7.9.5

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to