Applied, thx~
On Thu, Apr 24, 2014 at 5:54 AM, Joe Stringer <joestrin...@nicira.com>wrote:
> 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
