Acked-by: Joe Stringer <joestrin...@nicira.com>
On 18 April 2014 08:32, Alex Wang <al...@nicira.com> wrote:
> 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 <al...@nicira.com>
> ---
> V3 -> V4:
> - mirror the comment format of putting 'EOPNOTSUPP...' in newline.
> - state in comment that '*status' is indeterminate if get_cfm_status()
> returns non-zero value.
> - fix the comment of get_cfm_status(), since the caller do free the
> 'status->rmps' array.
> - update the comment of ofproto_port_get_bfd_status().
> - always destroy the smap in instant_stats_run().
>
> 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 | 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 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 3648dd7..5b0f6bd 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..a3591df 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 436a745..e3e0444 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -1016,10 +1016,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)
> @@ -3699,20 +3701,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 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..295e1be 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -1836,9 +1836,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);
> @@ -2246,9 +2250,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);
> }
> }
> --
> 1.7.9.5
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
