From 88bf179aa3f3fa89822edcd9b882e0f06d39bf08 Mon Sep 17 00:00:00 2001 From: Alex Wang Date: Thu, 3 Apr 2014 10:20:44 -0700 Subject: [PATCH] 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 Acked-by: Joe Stringer --- 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 | 27 +++++++++++++++++---------- ofproto/ofproto.c | 30 +++++++++++++++++------------- ofproto/ofproto.h | 6 +++--- vswitchd/bridge.c | 16 +++++++++++----- 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); } } -- 2.43.0