bfd/cfm: Check status change before update status to database.

This commit adds boolean flag in bfd/cfm module for checking
status change.  If there is no status change, the current
update to OVS database will skip the bfd/cfm session.

In the experiment with 5K bfd sessions, when one session is
flapping at rate of every 0.3 second, this patch reduces the
cpu utilization of the ovs-vswitchd thread from 13 to 6.

Signed-off-by: Alex Wang <alexw@nicira.com>
Acked-by: Joe Stringer <joestringer@nicira.com>

9 files changed, 138 insertions, 47 deletions

diff --git a/lib/bfd.c b/lib/bfd.c
index 8bfe38584..e3e3ae5f1 100644
--- a/lib/bfd.c
+++ b/lib/bfd.c
@@ -213,6 +213,10 @@ struct bfd {
     long long int decay_detect_time; /* Decay detection time. */
 
     uint64_t flap_count;          /* Counts bfd forwarding flaps. */
+
+    /* True when the variables returned by bfd_get_status() are changed
+     * since last check. */
+    bool status_changed;
 };
 
 static struct ovs_mutex mutex = OVS_MUTEX_INITIALIZER;
@@ -240,6 +244,7 @@ static void bfd_put_details(struct ds *, const struct bfd *)
 static uint64_t bfd_rx_packets(const struct bfd *) OVS_REQUIRES(mutex);
 static void bfd_try_decay(struct bfd *) OVS_REQUIRES(mutex);
 static void bfd_decay_update(struct bfd *) OVS_REQUIRES(mutex);
+static void bfd_status_changed(struct bfd *) OVS_REQUIRES(mutex);
 
 static void bfd_forwarding_if_rx_update(struct bfd *) OVS_REQUIRES(mutex);
 static void bfd_unixctl_show(struct unixctl_conn *, int argc,
@@ -279,6 +284,20 @@ bfd_account_rx(struct bfd *bfd, const struct dpif_flow_stats *stats)
     }
 }
 
+/* Returns and resets the 'bfd->status_changed'. */
+bool
+bfd_check_status_change(struct bfd *bfd) OVS_EXCLUDED(mutex)
+{
+    bool ret;
+
+    ovs_mutex_lock(&mutex);
+    ret = bfd->status_changed;
+    bfd->status_changed = false;
+    ovs_mutex_unlock(&mutex);
+
+    return ret;
+}
+
 /* Returns a 'smap' of key value pairs representing the status of 'bfd'
  * intended for the OVS database. */
 void
@@ -749,7 +768,7 @@ bfd_process_packet(struct bfd *bfd, const struct flow *flow,
     }
 
     if (bfd->rmt_state != rmt_state) {
-        seq_change(connectivity_seq_get());
+        bfd_status_changed(bfd);
     }
 
     bfd->rmt_disc = ntohl(msg->my_disc);
@@ -872,7 +891,7 @@ bfd_forwarding__(struct bfd *bfd) OVS_REQUIRES(mutex)
                             && bfd->rmt_diag != DIAG_RCPATH_DOWN;
     if (bfd->last_forwarding != last_forwarding) {
         bfd->flap_count++;
-        seq_change(connectivity_seq_get());
+        bfd_status_changed(bfd);
     }
     return bfd->last_forwarding;
 }
@@ -1085,7 +1104,7 @@ bfd_set_state(struct bfd *bfd, enum state state, enum diag diag)
             bfd_decay_update(bfd);
         }
 
-        seq_change(connectivity_seq_get());
+        bfd_status_changed(bfd);
     }
 }
 
@@ -1132,6 +1151,14 @@ bfd_decay_update(struct bfd * bfd) OVS_REQUIRES(mutex)
     bfd->decay_detect_time = MAX(bfd->decay_min_rx, 2000) + time_msec();
 }
 
+/* Records the status change and changes the global connectivity seq. */
+static void
+bfd_status_changed(struct bfd *bfd) OVS_REQUIRES(mutex)
+{
+    seq_change(connectivity_seq_get());
+    bfd->status_changed = true;
+}
+
 static void
 bfd_forwarding_if_rx_update(struct bfd *bfd) OVS_REQUIRES(mutex)
 {
@@ -1279,9 +1306,11 @@ bfd_unixctl_set_forwarding_override(struct unixctl_conn *conn, int argc,
             goto out;
         }
         bfd->forwarding_override = forwarding_override;
+        bfd_status_changed(bfd);
     } else {
         HMAP_FOR_EACH (bfd, node, all_bfds) {
             bfd->forwarding_override = forwarding_override;
+            bfd_status_changed(bfd);
         }
     }
 
diff --git a/lib/bfd.h b/lib/bfd.h
index 4e7d4cb46..039b4dd96 100644
--- a/lib/bfd.h
+++ b/lib/bfd.h
@@ -49,6 +49,7 @@ void bfd_unref(struct bfd *);
 
 void bfd_account_rx(struct bfd *, const struct dpif_flow_stats *);
 bool bfd_forwarding(struct bfd *);
+bool bfd_check_status_change(struct bfd *);
 void bfd_get_status(const struct bfd *, struct smap *);
 void bfd_set_netdev(struct bfd *, const struct netdev *);
 long long int bfd_wake_time(const struct bfd *);
diff --git a/lib/cfm.c b/lib/cfm.c
index ce0c471e3..6a173a709 100644
--- a/lib/cfm.c
+++ b/lib/cfm.c
@@ -133,6 +133,10 @@ struct cfm {
     struct ovs_refcount ref_cnt;
 
     uint64_t flap_count;       /* Count the flaps since boot. */
+
+    /* True when the variables returned by cfm_get_*() are changed
+     * since last check. */
+    bool status_changed;
 };
 
 /* Remote MPs represent foreign network entities that are configured to have
@@ -343,6 +347,7 @@ cfm_create(const struct netdev *netdev) OVS_EXCLUDED(mutex)
     cfm_generate_maid(cfm);
     hmap_insert(all_cfms, &cfm->hmap_node, hash_string(cfm->name, 0));
     ovs_mutex_unlock(&mutex);
+
     return cfm;
 }
 
@@ -385,6 +390,14 @@ cfm_ref(const struct cfm *cfm_)
     return cfm;
 }
 
+/* Records the status change and changes the global connectivity seq. */
+static void
+cfm_status_changed(struct cfm *cfm) OVS_REQUIRES(mutex)
+{
+    seq_change(connectivity_seq_get());
+    cfm->status_changed = true;
+}
+
 /* Should be run periodically to update fault statistics messages. */
 void
 cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex)
@@ -510,7 +523,7 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex)
             || (old_rmps_array_len != cfm->rmps_array_len || old_rmps_deleted)
             || old_cfm_fault != cfm->fault
             || old_flap_count != cfm->flap_count) {
-            seq_change(connectivity_seq_get());
+            cfm_status_changed(cfm);
         }
 
         cfm->booted = true;
@@ -836,6 +849,20 @@ out:
     ovs_mutex_unlock(&mutex);
 }
 
+/* Returns and resets the 'cfm->status_changed'. */
+bool
+cfm_check_status_change(struct cfm *cfm) OVS_EXCLUDED(mutex)
+{
+    bool ret;
+
+    ovs_mutex_lock(&mutex);
+    ret = cfm->status_changed;
+    cfm->status_changed = false;
+    ovs_mutex_unlock(&mutex);
+
+    return ret;
+}
+
 static int
 cfm_get_fault__(const struct cfm *cfm) OVS_REQUIRES(mutex)
 {
@@ -1029,13 +1056,14 @@ cfm_unixctl_set_fault(struct unixctl_conn *conn, int argc, const char *argv[],
             goto out;
         }
         cfm->fault_override = fault_override;
+        cfm_status_changed(cfm);
     } else {
         HMAP_FOR_EACH (cfm, hmap_node, all_cfms) {
             cfm->fault_override = fault_override;
+            cfm_status_changed(cfm);
         }
     }
 
-    seq_change(connectivity_seq_get());
     unixctl_command_reply(conn, "OK");
 
 out:
diff --git a/lib/cfm.h b/lib/cfm.h
index 4213eb538..13fdc60a6 100644
--- a/lib/cfm.h
+++ b/lib/cfm.h
@@ -76,6 +76,7 @@ void cfm_set_netdev(struct cfm *, const struct netdev *);
 bool cfm_should_process_flow(const struct cfm *cfm, const struct flow *,
                              struct flow_wildcards *);
 void cfm_process_heartbeat(struct cfm *, const struct ofpbuf *packet);
+bool cfm_check_status_change(struct cfm *);
 int cfm_get_fault(const struct cfm *);
 uint64_t cfm_get_flap_count(const struct cfm *);
 int cfm_get_health(const struct cfm *);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 5669cd184..30cbb24c5 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -78,6 +78,9 @@ enum { N_TABLES = 255 };
 enum { TBL_INTERNAL = N_TABLES - 1 };    /* Used for internal hidden rules. */
 BUILD_ASSERT_DECL(N_TABLES >= 2 && N_TABLES <= 255);
 
+/* No bfd/cfm status change. */
+#define NO_STATUS_CHANGE -1
+
 struct flow_miss;
 
 struct rule_dpif {
@@ -1813,22 +1816,28 @@ out:
     return error;
 }
 
-static bool
+static int
 get_cfm_status(const struct ofport *ofport_,
                struct ofproto_cfm_status *status)
 {
     struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
+    int ret = 0;
 
     if (ofport->cfm) {
-        status->faults = cfm_get_fault(ofport->cfm);
-        status->flap_count = cfm_get_flap_count(ofport->cfm);
-        status->remote_opstate = cfm_get_opup(ofport->cfm);
-        status->health = cfm_get_health(ofport->cfm);
-        cfm_get_remote_mpids(ofport->cfm, &status->rmps, &status->n_rmps);
-        return true;
+        if (cfm_check_status_change(ofport->cfm)) {
+            status->faults = cfm_get_fault(ofport->cfm);
+            status->flap_count = cfm_get_flap_count(ofport->cfm);
+            status->remote_opstate = cfm_get_opup(ofport->cfm);
+            status->health = cfm_get_health(ofport->cfm);
+            cfm_get_remote_mpids(ofport->cfm, &status->rmps, &status->n_rmps);
+        } else {
+            ret = NO_STATUS_CHANGE;
+        }
     } else {
-        return false;
+        ret = ENOENT;
     }
+
+    return ret;
 }
 
 static int
@@ -1853,13 +1862,19 @@ static int
 get_bfd_status(struct ofport *ofport_, struct smap *smap)
 {
     struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
+    int ret = 0;
 
     if (ofport->bfd) {
-        bfd_get_status(ofport->bfd, smap);
-        return 0;
+        if (bfd_check_status_change(ofport->bfd)) {
+            bfd_get_status(ofport->bfd, smap);
+        } else {
+            ret = NO_STATUS_CHANGE;
+        }
     } else {
-        return ENOENT;
+        ret = ENOENT;
     }
+
+    return ret;
 }
 
 /* Spanning Tree. */
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 0148fe612..11dcd8268 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1460,15 +1460,19 @@ struct ofproto_class {
      * support CFM, as does a null pointer. */
     int (*set_cfm)(struct ofport *ofport, const struct cfm_settings *s);
 
-    /* Checks the status of CFM configured on 'ofport'.  Returns true if the
-     * port's CFM status was successfully stored into '*status'.  Returns false
-     * if the port did not have CFM configured, in which case '*status' is
-     * indeterminate.
+    /* Checks the status of CFM configured on 'ofport'.  Returns 0 if the
+     * port's CFM status was successfully stored into '*status'.  Returns
+     * negative number if there is no status change since last update.
+     * Returns positive errno otherwise.
      *
-     * The caller must provide and owns '*status', but it does not own and must
-     * not modify or free the array returned in 'status->rmps'. */
-    bool (*get_cfm_status)(const struct ofport *ofport,
-                           struct ofproto_cfm_status *status);
+     * EOPNOTSUPP as a return value indicates that this ofproto_class does not
+     * support CFM, as does a null pointer.
+     *
+     * The caller must provide and own '*status', and it must free the array
+     * returned in 'status->rmps'.  '*status' is indeterminate if the return
+     * value is non-zero. */
+    int (*get_cfm_status)(const struct ofport *ofport,
+                          struct ofproto_cfm_status *status);
 
     /* Configures BFD on 'ofport'.
      *
@@ -1481,8 +1485,11 @@ struct ofproto_class {
     int (*set_bfd)(struct ofport *ofport, const struct smap *cfg);
 
     /* Populates 'smap' with the status of BFD on 'ofport'.  Returns 0 on
-     * success, or a positive errno.  EOPNOTSUPP as a return value indicates
-     * that this ofproto_class does not support BFD, as does a null pointer. */
+     * success.  Returns a negative number if there is no status change since
+     * last update.  Returns a positive errno otherwise.
+     *
+     * EOPNOTSUPP as a return value indicates that this ofproto_class does not
+     * support BFD, as does a null pointer. */
     int (*get_bfd_status)(struct ofport *ofport, struct smap *smap);
 
     /* Configures spanning tree protocol (STP) on 'ofproto' using the
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index f10d3ae68..659990ecf 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1018,10 +1018,12 @@ ofproto_port_set_bfd(struct ofproto *ofproto, ofp_port_t ofp_port,
     }
 }
 
-/* Populates 'status' with key value pairs indicating the status of the BFD
- * session on 'ofp_port'.  This information is intended to be populated in the
- * OVS database.  Has no effect if 'ofp_port' is not na OpenFlow port in
- * 'ofproto'. */
+/* Populates 'status' with the status of BFD on 'ofport'.  Returns 0 on
+ * success.  Returns a negative number if there is no status change since
+ * last update.  Returns a positive errno otherwise.  Has no effect if
+ * 'ofp_port' is not an OpenFlow port in 'ofproto'.
+ *
+ * The caller must provide and own '*status'. */
 int
 ofproto_port_get_bfd_status(struct ofproto *ofproto, ofp_port_t ofp_port,
                             struct smap *status)
@@ -3755,20 +3757,22 @@ ofproto_get_netflow_ids(const struct ofproto *ofproto,
     ofproto->ofproto_class->get_netflow_ids(ofproto, engine_type, engine_id);
 }
 
-/* Checks the status of CFM configured on 'ofp_port' within 'ofproto'.  Returns
- * true if the port's CFM status was successfully stored into '*status'.
- * Returns false if the port did not have CFM configured, in which case
- * '*status' is indeterminate.
+/* Checks the status of CFM configured on 'ofp_port' within 'ofproto'.
+ * Returns 0 if the port's CFM status was successfully stored into
+ * '*status'.  Returns positive errno if the port did not have CFM
+ * configured.  Returns negative number if there is no status change
+ * since last update.
  *
- * The caller must provide and owns '*status', and must free 'status->rmps'. */
-bool
+ * The caller must provide and own '*status', and must free 'status->rmps'.
+ * '*status' is indeterminate if the return value is non-zero. */
+int
 ofproto_port_get_cfm_status(const struct ofproto *ofproto, ofp_port_t ofp_port,
                             struct ofproto_cfm_status *status)
 {
     struct ofport *ofport = ofproto_get_port(ofproto, ofp_port);
-    return (ofport
-            && ofproto->ofproto_class->get_cfm_status
-            && ofproto->ofproto_class->get_cfm_status(ofport, status));
+    return (ofport && ofproto->ofproto_class->get_cfm_status
+            ? ofproto->ofproto_class->get_cfm_status(ofport, status)
+            : EOPNOTSUPP);
 }
 
 static enum ofperr
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index ab51365c8..9ba6354f5 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -419,9 +419,9 @@ struct ofproto_cfm_status {
     size_t n_rmps;
 };
 
-bool ofproto_port_get_cfm_status(const struct ofproto *,
-                                 ofp_port_t ofp_port,
-                                 struct ofproto_cfm_status *);
+int ofproto_port_get_cfm_status(const struct ofproto *,
+                                ofp_port_t ofp_port,
+                                struct ofproto_cfm_status *);
 
 /* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.)
  *
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 45a14911f..e35f34d6b 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1876,9 +1876,13 @@ iface_refresh_cfm_stats(struct iface *iface)
 {
     const struct ovsrec_interface *cfg = iface->cfg;
     struct ofproto_cfm_status status;
+    int error;
 
-    if (!ofproto_port_get_cfm_status(iface->port->bridge->ofproto,
-                                    iface->ofp_port, &status)) {
+    error = ofproto_port_get_cfm_status(iface->port->bridge->ofproto,
+                                        iface->ofp_port, &status);
+    if (error < 0) {
+        /* Do nothing if there is no status change since last update. */
+    } else if (error > 0) {
         ovsrec_interface_set_cfm_fault(cfg, NULL, 0);
         ovsrec_interface_set_cfm_fault_status(cfg, NULL, 0);
         ovsrec_interface_set_cfm_remote_opstate(cfg, NULL);
@@ -2286,9 +2290,11 @@ instant_stats_run(void)
                 iface_refresh_cfm_stats(iface);
 
                 smap_init(&smap);
-                ofproto_port_get_bfd_status(br->ofproto, iface->ofp_port,
-                                            &smap);
-                ovsrec_interface_set_bfd_status(iface->cfg, &smap);
+                error = ofproto_port_get_bfd_status(br->ofproto, iface->ofp_port,
+                                                    &smap);
+                if (error >= 0) {
+                    ovsrec_interface_set_bfd_status(iface->cfg, &smap);
+                }
                 smap_destroy(&smap);
             }
         }