[ovs-dev] [PATCHv4 2/4] ofproto-dpif: Don't poll ports when nothing changes

Ethan Jackson ethan at nicira.com
Thu Dec 12 02:08:00 UTC 2013


Acked-by: Ethan Jackson <ethan at nicira.com>

Could you fold my acked by into the commit message so when you resend
rebased on the new version of the first patch, I know not to review it
again?

Thanks

On Wed, Dec 11, 2013 at 9:10 AM, Joe Stringer <joestringer at nicira.com> wrote:
> Currently, as part of ofproto-dpif run() processing, we loop through all
> ports and poll corresponding devices for changes in carrier, cfm and bfd
> status. This allows us to determine how it may affect bundles. For the
> average case where devices are not going up or down constantly, this is
> a large amount of unnecessary processing.
>
> This patch gets the bfd, cfm, lacp and stp modules to update the global
> netdev change_seq when changes in port/device status occur. We can then
> use this global change_seq to check if anything has changed before
> looping through the ports in ofproto-dpif. In a test environment of 5000
> internal ports and 50 tunnel ports with bfd, this reduces CPU usage from
> about 35% to about 25%.
>
> Signed-off-by: Joe Stringer <joestringer at nicira.com>
> ---
> v4: Ensure that bfd_forwarding__() is called each time through bfd_run().
> ---
>  lib/bfd.c              |    6 +++++-
>  lib/cfm.c              |   13 +++++++++++++
>  lib/lacp.c             |    7 +++++++
>  lib/stp.c              |    4 ++++
>  ofproto/ofproto-dpif.c |   16 +++++++++++++---
>  tests/test-stp.c       |    5 +++++
>  6 files changed, 47 insertions(+), 4 deletions(-)
>
> diff --git a/lib/bfd.c b/lib/bfd.c
> index 1df5acd..2482d37 100644
> --- a/lib/bfd.c
> +++ b/lib/bfd.c
> @@ -21,6 +21,7 @@
>  #include <netinet/ip.h>
>
>  #include "byte-order.h"
> +#include "connectivity.h"
>  #include "csum.h"
>  #include "dpif.h"
>  #include "dynamic-string.h"
> @@ -505,8 +506,8 @@ bfd_run(struct bfd *bfd) OVS_EXCLUDED(mutex)
>
>      if (bfd->state > STATE_DOWN && now >= bfd->detect_time) {
>          bfd_set_state(bfd, STATE_DOWN, DIAG_EXPIRED);
> -        bfd_forwarding__(bfd);
>      }
> +    bfd_forwarding__(bfd);
>
>      /* Decay may only happen when state is STATE_UP, bfd->decay_min_rx is
>       * configured, and decay_detect_time is reached. */
> @@ -851,6 +852,7 @@ bfd_forwarding__(struct bfd *bfd) OVS_REQUIRES(mutex)
>                              && bfd->rmt_diag != DIAG_RCPATH_DOWN;
>      if (bfd->last_forwarding != last_forwarding) {
>          bfd->flap_count++;
> +        connectivity_seq_notify();
>      }
>      return bfd->last_forwarding;
>  }
> @@ -1052,6 +1054,8 @@ bfd_set_state(struct bfd *bfd, enum state state, enum diag diag)
>          if (bfd->state == STATE_UP && bfd->decay_min_rx) {
>              bfd_decay_update(bfd);
>          }
> +
> +        connectivity_seq_notify();
>      }
>  }
>
> diff --git a/lib/cfm.c b/lib/cfm.c
> index 9c65b34..5d27c6f 100644
> --- a/lib/cfm.c
> +++ b/lib/cfm.c
> @@ -22,6 +22,7 @@
>  #include <string.h>
>
>  #include "byte-order.h"
> +#include "connectivity.h"
>  #include "dynamic-string.h"
>  #include "flow.h"
>  #include "hash.h"
> @@ -396,6 +397,7 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex)
>          long long int interval = cfm_fault_interval(cfm);
>          struct remote_mp *rmp, *rmp_next;
>          bool old_cfm_fault = cfm->fault;
> +        bool old_rmp_opup = cfm->remote_opup;
>          bool demand_override;
>          bool rmp_set_opup = false;
>          bool rmp_set_opdown = false;
> @@ -420,6 +422,7 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex)
>                  cfm->health = 0;
>              } else {
>                  int exp_ccm_recvd;
> +                int old_health = cfm->health;
>
>                  rmp = CONTAINER_OF(hmap_first(&cfm->remote_mps),
>                                     struct remote_mp, node);
> @@ -434,6 +437,10 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex)
>                  cfm->health = MIN(cfm->health, 100);
>                  rmp->num_health_ccm = 0;
>                  ovs_assert(cfm->health >= 0 && cfm->health <= 100);
> +
> +                if (cfm->health != old_health) {
> +                    connectivity_seq_notify();
> +                }
>              }
>              cfm->health_interval = 0;
>          }
> @@ -476,6 +483,10 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex)
>              cfm->remote_opup = true;
>          }
>
> +        if (old_rmp_opup != cfm->remote_opup) {
> +            connectivity_seq_notify();
> +        }
> +
>          if (hmap_is_empty(&cfm->remote_mps)) {
>              cfm->fault |= CFM_FAULT_RECV;
>          }
> @@ -497,6 +508,8 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex)
>              if (old_cfm_fault == false || cfm->fault == false) {
>                  cfm->flap_count++;
>              }
> +
> +            connectivity_seq_notify();
>          }
>
>          cfm->booted = true;
> diff --git a/lib/lacp.c b/lib/lacp.c
> index fce65b3..bb6e940 100644
> --- a/lib/lacp.c
> +++ b/lib/lacp.c
> @@ -18,6 +18,7 @@
>
>  #include <stdlib.h>
>
> +#include "connectivity.h"
>  #include "dynamic-string.h"
>  #include "hash.h"
>  #include "hmap.h"
> @@ -509,11 +510,16 @@ lacp_run(struct lacp *lacp, lacp_send_pdu *send_pdu) OVS_EXCLUDED(mutex)
>      ovs_mutex_lock(&mutex);
>      HMAP_FOR_EACH (slave, node, &lacp->slaves) {
>          if (timer_expired(&slave->rx)) {
> +            enum slave_status old_status = slave->status;
> +
>              if (slave->status == LACP_CURRENT) {
>                  slave_set_expired(slave);
>              } else if (slave->status == LACP_EXPIRED) {
>                  slave_set_defaulted(slave);
>              }
> +            if (slave->status != old_status) {
> +                connectivity_seq_notify();
> +            }
>          }
>      }
>
> @@ -544,6 +550,7 @@ lacp_run(struct lacp *lacp, lacp_send_pdu *send_pdu) OVS_EXCLUDED(mutex)
>                          : LACP_SLOW_TIME_TX);
>
>              timer_set_duration(&slave->tx, duration);
> +            connectivity_seq_notify();
>          }
>      }
>      ovs_mutex_unlock(&mutex);
> diff --git a/lib/stp.c b/lib/stp.c
> index 725bfff..05bf0ff 100644
> --- a/lib/stp.c
> +++ b/lib/stp.c
> @@ -26,6 +26,7 @@
>  #include <inttypes.h>
>  #include <stdlib.h>
>  #include "byte-order.h"
> +#include "connectivity.h"
>  #include "ofpbuf.h"
>  #include "packets.h"
>  #include "unixctl.h"
> @@ -1127,6 +1128,7 @@ stp_configuration_update(struct stp *stp) OVS_REQUIRES(mutex)
>  {
>      stp_root_selection(stp);
>      stp_designated_port_selection(stp);
> +    connectivity_seq_notify();
>  }
>
>  static bool
> @@ -1257,6 +1259,7 @@ stp_set_port_state(struct stp_port *p, enum stp_state state)
>          if (p < p->stp->first_changed_port) {
>              p->stp->first_changed_port = p;
>          }
> +        connectivity_seq_notify();
>      }
>      p->state = state;
>  }
> @@ -1275,6 +1278,7 @@ stp_topology_change_detection(struct stp *stp) OVS_REQUIRES(mutex)
>      }
>      stp->fdb_needs_flush = true;
>      stp->topology_change_detected = true;
> +    connectivity_seq_notify();
>      VLOG_INFO_RL(&rl, "%s: detected topology change.", stp->name);
>  }
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index ff77903..a4334cc 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -25,6 +25,7 @@
>  #include "bond.h"
>  #include "bundle.h"
>  #include "byte-order.h"
> +#include "connectivity.h"
>  #include "connmgr.h"
>  #include "coverage.h"
>  #include "cfm.h"
> @@ -511,6 +512,7 @@ struct ofproto_dpif {
>      struct sset ghost_ports;       /* Ports with no datapath port. */
>      struct sset port_poll_set;     /* Queued names for port_poll() reply. */
>      int port_poll_errno;           /* Last errno for port_poll() reply. */
> +    uint64_t change_seq;           /* Connectivity status changes. */
>
>      /* Per ofproto's dpif stats. */
>      uint64_t n_hit;
> @@ -1269,6 +1271,7 @@ construct(struct ofproto *ofproto_)
>      sset_init(&ofproto->ghost_ports);
>      sset_init(&ofproto->port_poll_set);
>      ofproto->port_poll_errno = 0;
> +    ofproto->change_seq = 0;
>
>      SHASH_FOR_EACH_SAFE (node, next, &init_ofp_ports) {
>          struct iface_hint *iface_hint = node->data;
> @@ -1474,8 +1477,8 @@ static int
>  run(struct ofproto *ofproto_)
>  {
>      struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
> -    struct ofport_dpif *ofport;
>      struct ofbundle *bundle;
> +    uint64_t new_seq;
>      int error;
>
>      if (mbridge_need_revalidate(ofproto->mbridge)) {
> @@ -1508,8 +1511,15 @@ run(struct ofproto *ofproto_)
>          dpif_ipfix_run(ofproto->ipfix);
>      }
>
> -    HMAP_FOR_EACH (ofport, up.hmap_node, &ofproto->up.ports) {
> -        port_run(ofport);
> +    new_seq = connectivity_seq_read();
> +    if (ofproto->change_seq != new_seq) {
> +        struct ofport_dpif *ofport;
> +
> +        HMAP_FOR_EACH (ofport, up.hmap_node, &ofproto->up.ports) {
> +            port_run(ofport);
> +        }
> +
> +        ofproto->change_seq = new_seq;
>      }
>      HMAP_FOR_EACH (bundle, hmap_node, &ofproto->bundles) {
>          bundle_run(bundle);
> diff --git a/tests/test-stp.c b/tests/test-stp.c
> index be1b395..a177c87 100644
> --- a/tests/test-stp.c
> +++ b/tests/test-stp.c
> @@ -16,6 +16,7 @@
>
>  #include <config.h>
>
> +#include "connectivity.h"
>  #include "stp.h"
>  #include <assert.h>
>  #include <ctype.h>
> @@ -454,6 +455,8 @@ main(int argc, char *argv[])
>          ovs_fatal(errno, "error opening \"%s\"", file_name);
>      }
>
> +    connectivity_seq_init();
> +
>      tc = new_test_case();
>      for (i = 0; i < 26; i++) {
>          char name[2];
> @@ -654,6 +657,8 @@ main(int argc, char *argv[])
>      }
>      free(token);
>
> +    connectivity_seq_destroy();
> +
>      for (i = 0; i < tc->n_lans; i++) {
>          struct lan *lan = tc->lans[i];
>          free(CONST_CAST(char *, lan->name));
> --
> 1.7.9.5
>



More information about the dev mailing list