[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