[ovs-dev] [PATCH V3 1/2] bfd/cfm: Check status change before update status to database.
Alex Wang
alexw at nicira.com
Fri Apr 11 18:20:13 UTC 2014
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 at nicira.com>
---
V2 -> V3:
- rebase.
PATCH -> V2:
- replace the callback function to boolean flag.
- add experiment results.
---
lib/bfd.c | 35 ++++++++++++++++++++++++++++++++---
lib/bfd.h | 1 +
lib/cfm.c | 32 ++++++++++++++++++++++++++++++--
lib/cfm.h | 1 +
ofproto/ofproto-dpif.c | 37 ++++++++++++++++++++++++++-----------
ofproto/ofproto-provider.h | 20 ++++++++++++--------
ofproto/ofproto.c | 19 ++++++++++---------
ofproto/ofproto.h | 6 +++---
vswitchd/bridge.c | 16 ++++++++++++----
9 files changed, 127 insertions(+), 40 deletions(-)
diff --git a/lib/bfd.c b/lib/bfd.c
index 8bfe385..e3e3ae5 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 4e7d4cb..039b4dd 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 ce0c471..6a173a7 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 4213eb5..13fdc60 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 a92fe81..04d5454 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 {
@@ -1801,22 +1804,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
@@ -1841,13 +1850,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 0e22b7c..1ef1e17 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1460,15 +1460,17 @@ 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. EOPNOTSUPP as a return value
+ * indicates that this ofproto_class does not support CFM, as does a
+ * null pointer.
*
* 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);
+ int (*get_cfm_status)(const struct ofport *ofport,
+ struct ofproto_cfm_status *status);
/* Configures BFD on 'ofport'.
*
@@ -1481,8 +1483,10 @@ 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 436a745..86b4a02 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1018,7 +1018,7 @@ 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
+ * OVS database. Has no effect if 'ofp_port' is not an OpenFlow port in
* 'ofproto'. */
int
ofproto_port_get_bfd_status(struct ofproto *ofproto, ofp_port_t ofp_port,
@@ -3699,20 +3699,21 @@ 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, in which case '*status' is indeterminate. 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
+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 ab51365..9ba6354 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 f46d002..44fdb4e 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1836,9 +1836,14 @@ 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);
+ /* Do nothing if there is no status change since last update. */
+ if (error < 0) {
+ // PASS.
+ } 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);
@@ -2246,8 +2251,11 @@ instant_stats_run(void)
iface_refresh_cfm_stats(iface);
smap_init(&smap);
- ofproto_port_get_bfd_status(br->ofproto, iface->ofp_port,
- &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);
}
--
1.7.9.5
More information about the dev
mailing list