[ovs-dev] [PATCHv3 3/5] ofproto-dpif: Don't poll ports when nothing changes

Joe Stringer joestringer at nicira.com
Tue Dec 10 20:57:35 UTC 2013


Err, for those without gmail context; I am referring to the bfd
forwarding changes here:

http://git.openvswitch.org/cgi-bin/gitweb.cgi?p=openvswitch;a=commitdiff;h=a1aeea86475db086ce95679962fb6d03d0a645f3

On 10 December 2013 12:56, Joe Stringer <joestringer at nicira.com> wrote:
> The merged version of the above patchset doesn't quite address the
> testsuite failure, for reasons described in that thread. Below is an
> incremental to fix this issue. I'm happy to fold in and repost, but I
> might as well wait until I get further feedback on the rest of this
> set.
>
> diff --git a/lib/bfd.c b/lib/bfd.c
> index 7f677ee..2482d37 100644
> --- a/lib/bfd.c
> +++ b/lib/bfd.c
> @@ -506,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. */
>
> On 25 November 2013 14:32, Joe Stringer <joestringer at nicira.com> wrote:
>> The patch series posted below fixes the cause of these test failures:-
>>
>> http://openvswitch.org/pipermail/dev/2013-November/034359.html
>>
>> On 25 November 2013 13:44, 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, and cause
>>> revalidation on bridges when necessary. 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 connectivity_seq
>>> when changes in port/device status occur. We can then use this 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 average CPU usage from around 35%->25%.
>>>
>>> Signed-off-by: Joe Stringer <joestringer at nicira.com>
>>> ---
>>> v3: Rebase against bfd_forwarding__() changes
>>>     Track ofproto-dpif change separate from ofproto
>>>     Track cfm->health
>>>     Fix STP test failures
>>>     Track STP more comprehensively
>>>     Notify if slave status changes
>>> v2: Fixed mutex safety in netdev_vport_changed()
>>>     Track STP changes
>>>
>>> NB: This introduces test failures for BFD. This is due to a subtle interaction
>>> between this patch and recent bfd changes. A forthcoming patch will fix this
>>> issue.
>>> ---
>>>  lib/bfd.c              |    4 ++++
>>>  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, 46 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/lib/bfd.c b/lib/bfd.c
>>> index 4582f2e..c334e88 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"
>>> @@ -831,6 +832,7 @@ bfd_forwarding__(struct bfd *bfd) OVS_REQUIRES(mutex)
>>>                         && bfd->rmt_diag != DIAG_RCPATH_DOWN;
>>>      if (bfd->forwarding != forwarding_old) {
>>>          bfd->flap_count++;
>>> +        connectivity_seq_notify();
>>>      }
>>>      return bfd->forwarding;
>>>  }
>>> @@ -1032,6 +1034,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();
>>>      }
>>>
>>>      /* Updates the forwarding flag. */
>>> 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 6e1efd0..ecf37c1 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 6653049..c06dc20 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"
>>> @@ -510,6 +511,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;
>>> @@ -1268,6 +1270,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;
>>> @@ -1473,8 +1476,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)) {
>>> @@ -1507,8 +1510,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 28e9a6e..1bcd12f 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