[ovs-dev] [PATCHv3] monitor: Replace monitor_seq with periodic wakeup
Alex Wang
alexw at nicira.com
Fri Dec 13 18:40:37 UTC 2013
yeah, I can only reproduce it on my VM setup by running command like this:
for i in `seq 1 1000`; do make check TESTSUITEFLAGS=12 1>/dev/null
2>file_make_check; if cat file_make_check | grep "ERROR: 1 test was run";
then echo FAIL; break; fi; done
and wait a long time.
But the issue is valid. Looks good to me,
On Fri, Dec 13, 2013 at 10:34 AM, Joe Stringer <joestringer at nicira.com>wrote:
> Curious. I can't seem to reproduce the occasional fault in my VM
> setup. I'm happy to fold in the change that you suggested though Alex.
> (Incremental below)
>
> diff --git a/tests/cfm.at b/tests/cfm.at
> index 620e3e0..fe6778a 100644
> --- a/tests/cfm.at
> +++ b/tests/cfm.at
> @@ -90,12 +90,9 @@ CFM_VSCTL_LIST_IFACE([p0], [cfm_flap_count],
> [cfm_flap_count : 1])
> CFM_VSCTL_LIST_IFACE([p1], [cfm_flap_count], [cfm_flap_count : [[]]])
>
> # turn cfm on p1 on again, should increment the cfm_flap_count on p0.
> -# After p1 is configured, but before it is first run, it will receive
> -# a ccm with the rdi bit set, and detect a flap as well.
> AT_CHECK([ovs-vsctl set interface p1 cfm_mpid=2])
> for i in `seq 0 10`; do ovs-appctl time/warp 100; done
> CFM_VSCTL_LIST_IFACE([p0], [cfm_flap_count], [cfm_flap_count : 2])
> -CFM_VSCTL_LIST_IFACE([p1], [cfm_flap_count], [cfm_flap_count : 2])
>
> OVS_VSWITCHD_STOP
>
> On 13 December 2013 08:59, Joe Stringer <joestringer at nicira.com> wrote:
> > Thanks for the detail, I'll look into it.
> >
> > On 12 December 2013 21:50, Alex Wang <alexw at nicira.com> wrote:
> >> Yes, I could reproduce it on my VM setup,
> >>
> >> It is the timing issue again, if the p1 starts sending ccm first, it
> will
> >> have flap count = 0.
> >>
> >> If p1 receives ccm from p0 first, it will go to [rdi] and then []. so
> it
> >> will have flap count = 2.
> >>
> >> I think the safest way is to drop the "CFM_VSCTL_LIST_IFACE([p1],
> >> [cfm_flap_count], [cfm_flap_count : 0])" and don't check
> >> p1 flap count.
> >>
> >>
> >>
> >>
> >> On Thu, Dec 12, 2013 at 8:20 PM, Ethan Jackson <ethan at nicira.com>
> wrote:
> >>>
> >>> This patch occasionally fails the cfm flap count test for me. Can
> >>> either of you reproduce it?
> >>>
> >>> Ethan
> >>>
> >>> On Thu, Dec 12, 2013 at 5:52 PM, Joe Stringer <joestringer at nicira.com>
> >>> wrote:
> >>> > Currently, every time a monitoring port is added or reconfigured, the
> >>> > main thread notifies the monitoring thread to wake up immediately
> using
> >>> > monitor_seq. When adding a large number of ports at once, this causes
> >>> > contention as the threads fight over access to the monitor heap---one
> >>> > thread adding new ports, the other popping and processing the new
> ports.
> >>> >
> >>> > This patch removes this mechanism in favour of a simple periodic
> wakeup
> >>> > in the monitor thread. When configuring batches of 500 tunnels at
> once,
> >>> > up to a total of 5000 tunnels, this reduces configuration time by up
> to
> >>> > 35%. New port monitor port configuration may not take effect for as
> long
> >>> > as ~100ms, although it may be less if the timer has not recently
> timed
> >>> > out, or if a monitoring port is due to send a packet.
> >>> >
> >>> > Signed-off-by: Joe Stringer <joestringer at nicira.com>
> >>> > ---
> >>> > v3: Rebase
> >>> > ---
> >>> > ofproto/ofproto-dpif-monitor.c | 18 ++++++++++--------
> >>> > tests/bfd.at | 16 +++++++---------
> >>> > tests/cfm.at | 8 +++++---
> >>> > 3 files changed, 22 insertions(+), 20 deletions(-)
> >>> >
> >>> > diff --git a/ofproto/ofproto-dpif-monitor.c
> >>> > b/ofproto/ofproto-dpif-monitor.c
> >>> > index d06b2e1..2a833bb 100644
> >>> > --- a/ofproto/ofproto-dpif-monitor.c
> >>> > +++ b/ofproto/ofproto-dpif-monitor.c
> >>> > @@ -63,7 +63,6 @@ static pthread_t monitor_tid;
> >>> > /* True if the monitor thread is running. */
> >>> > static bool monitor_running;
> >>> >
> >>> > -static struct seq *monitor_seq;
> >>> > static struct latch monitor_exit_latch;
> >>> > static struct ovs_rwlock monitor_rwlock = OVS_RWLOCK_INITIALIZER;
> >>> >
> >>> > @@ -149,10 +148,9 @@ mport_update(struct mport *mport, struct bfd
> *bfd,
> >>> > struct cfm *cfm,
> >>> > memcpy(mport->hw_addr, hw_addr, ETH_ADDR_LEN);
> >>> > }
> >>> > /* If bfd/cfm is added or reconfigured, move the mport on top of
> >>> > the heap
> >>> > - * and wakes up the monitor thread. */
> >>> > + * so that the monitor thread can run the mport next time it
> wakes
> >>> > up. */
> >>> > if (mport->bfd || mport->cfm) {
> >>> > heap_change(&monitor_heap, &mport->heap_node, LLONG_MAX);
> >>> > - seq_change(monitor_seq);
> >>> > }
> >>> > }
> >>> >
> >>> > @@ -165,7 +163,6 @@ monitor_init(void)
> >>> >
> >>> > if (ovsthread_once_start(&once)) {
> >>> > hmap_init(&monitor_hmap);
> >>> > - monitor_seq = seq_create();
> >>> > ovsthread_once_done(&once);
> >>> > }
> >>> > }
> >>> > @@ -177,17 +174,18 @@ monitor_main(void * args OVS_UNUSED)
> >>> > set_subprogram_name("monitor");
> >>> > VLOG_INFO("monitor thread created");
> >>> > while (!latch_is_set(&monitor_exit_latch)) {
> >>> > - uint64_t seq = seq_read(monitor_seq);
> >>> > -
> >>> > monitor_run();
> >>> > latch_wait(&monitor_exit_latch);
> >>> > - seq_wait(monitor_seq, seq);
> >>> > poll_block();
> >>> > }
> >>> > VLOG_INFO("monitor thread terminated");
> >>> > return NULL;
> >>> > }
> >>> >
> >>> > +/* The monitor thread should wake up this often to ensure that newly
> >>> > added or
> >>> > + * reconfigured monitoring ports are run in a timely manner. */
> >>> > +#define MONITOR_INTERVAL_MSEC 100
> >>> > +
> >>> > /* Checks the sending of control packets on mports that have timed
> out.
> >>> > * Sends the control packets if needed. Executes bfd and cfm
> periodic
> >>> > * functions (run, wait) on those mports. */
> >>> > @@ -234,7 +232,11 @@ monitor_run(void)
> >>> >
> >>> > /* Waits on the earliest next wakeup time. */
> >>> > if (!heap_is_empty(&monitor_heap)) {
> >>> > -
> >>> >
> poll_timer_wait_until(PRIO_TO_MSEC(heap_max(&monitor_heap)->priority));
> >>> > + long long int next_timeout, next_mport_wakeup;
> >>> > +
> >>> > + next_timeout = time_msec() + MONITOR_INTERVAL_MSEC;
> >>> > + next_mport_wakeup =
> >>> > PRIO_TO_MSEC(heap_max(&monitor_heap)->priority);
> >>> > + poll_timer_wait_until(MIN(next_timeout, next_mport_wakeup));
> >>> > }
> >>> > ovs_rwlock_unlock(&monitor_rwlock);
> >>> > ofpbuf_uninit(&packet);
> >>> > diff --git a/tests/bfd.at b/tests/bfd.at
> >>> > index ccb62b5..6583dd5 100644
> >>> > --- a/tests/bfd.at
> >>> > +++ b/tests/bfd.at
> >>> > @@ -334,6 +334,7 @@ BFD_CHECK([p1], [true], [false], [none], [up],
> [No
> >>> > Diagnostic], [none], [up], [N
> >>> > # for decay_min_rx < 2000ms, the decay detection time is set to
> 2000ms.
> >>> > # this should reset the min_rx.
> >>> > AT_CHECK([ovs-vsctl set Interface p0 bfd:decay_min_rx=1000])
> >>> > +ovs-appctl time/warp 100
> >>> > BFD_CHECK_TX([p0], [500ms], [300ms], [500ms])
> >>> > BFD_CHECK_RX([p0], [500ms], [300ms], [500ms])
> >>> >
> >>> > @@ -360,8 +361,7 @@ for i in `seq 0 9`; do ovs-appctl time/warp 500;
> >>> > done
> >>> > # Test-4 BFD decay: set min_rx to 800ms.
> >>> > # this should firstly reset the min_rx and then re-decay to 1000ms.
> >>> > AT_CHECK([ovs-vsctl set Interface p0 bfd:min_rx=800])
> >>> > -BFD_CHECK_TX([p0], [500ms], [300ms], [500ms])
> >>> > -BFD_CHECK_RX([p0], [800ms], [800ms], [500ms])
> >>> > +ovs-appctl time/warp 100
> >>> >
> >>> > # for the following 1600ms, there should be no decay,
> >>> > # since the decay detection time is set to 2000ms.
> >>> > @@ -385,8 +385,7 @@ for i in `seq 0 9`; do ovs-appctl time/warp 500;
> >>> > done
> >>> >
> >>> > # Test-5 BFD decay: set min_rx to 300ms and decay_min_rx to 5000ms
> >>> > together.
> >>> > AT_CHECK([ovs-vsctl set Interface p0 bfd:min_rx=300
> >>> > bfd:decay_min_rx=5000])
> >>> > -BFD_CHECK_TX([p0], [500ms], [300ms], [500ms])
> >>> > -BFD_CHECK_RX([p0], [500ms], [300ms], [500ms])
> >>> > +ovs-appctl time/warp 100
> >>> >
> >>> > # for decay_min_rx > 2000ms, the decay detection time is set to
> >>> > # decay_min_rx (5000ms).
> >>> > @@ -412,9 +411,8 @@ for i in `seq 0 9`; do ovs-appctl time/warp 500;
> >>> > done
> >>> >
> >>> > # Test-6 BFD decay: set decay_min_rx to 0 to disable bfd decay.
> >>> > AT_CHECK([ovs-vsctl set Interface p0 bfd:decay_min_rx=0])
> >>> > -# min_rx is reset.
> >>> > -BFD_CHECK_TX([p0], [500ms], [300ms], [500ms])
> >>> > -BFD_CHECK_RX([p0], [500ms], [300ms], [500ms])
> >>> > +ovs-appctl time/warp 100
> >>> > +
> >>> > for i in `seq 0 20`
> >>> > do
> >>> > ovs-appctl time/warp 500
> >>> > @@ -444,6 +442,8 @@ do
> >>> > done
> >>> > # reset the p1's min_tx to 500ms.
> >>> > AT_CHECK([ovs-vsctl set Interface p1 bfd:min_tx=500])
> >>> > +ovs-appctl time/warp 100
> >>> > +
> >>> > # since p0 has been in decay, now the RX will show 3000ms.
> >>> > BFD_CHECK_TX([p0], [500ms], [300ms], [500ms])
> >>> > BFD_CHECK_RX([p0], [3000ms], [3000ms], [500ms])
> >>> > @@ -629,8 +629,6 @@ done
> >>> >
> >>> > # reconfigure the decay_min_rx to 1000ms.
> >>> > AT_CHECK([ovs-vsctl set interface p0 bfd:decay_min_rx=1000])
> >>> > -BFD_CHECK_TX([p0], [500ms], [300ms], [500ms])
> >>> > -BFD_CHECK_RX([p0], [500ms], [300ms], [500ms])
> >>> >
> >>> > # wait for 5000ms to decay.
> >>> > for i in `seq 0 9`; do ovs-appctl time/warp 500; done
> >>> > diff --git a/tests/cfm.at b/tests/cfm.at
> >>> > index 9e351d0..620e3e0 100644
> >>> > --- a/tests/cfm.at
> >>> > +++ b/tests/cfm.at
> >>> > @@ -83,17 +83,19 @@ for i in `seq 0 100`; do ovs-appctl time/warp
> 100;
> >>> > done
> >>> > CFM_CHECK_EXTENDED([p0], [1], [100], [up], [up], [100ms], [2], [up])
> >>> > CFM_CHECK_EXTENDED([p1], [2], [100], [up], [up], [100ms], [1], [up])
> >>> >
> >>> > -# turn cfm on p1 off, should increment the cfm_flap_count on p1.
> >>> > +# turn cfm on p1 off, should increment the cfm_flap_count on p0.
> >>> > AT_CHECK([ovs-vsctl remove interface p1 cfm_mpid 2])
> >>> > for i in `seq 0 10`; do ovs-appctl time/warp 100; done
> >>> > CFM_VSCTL_LIST_IFACE([p0], [cfm_flap_count], [cfm_flap_count :
> 1])
> >>> > CFM_VSCTL_LIST_IFACE([p1], [cfm_flap_count], [cfm_flap_count :
> >>> > [[]]])
> >>> >
> >>> > -# turn cfm on p1 on again, should increment the cfm_flap_count on
> p1.
> >>> > +# turn cfm on p1 on again, should increment the cfm_flap_count on
> p0.
> >>> > +# After p1 is configured, but before it is first run, it will
> receive
> >>> > +# a ccm with the rdi bit set, and detect a flap as well.
> >>> > AT_CHECK([ovs-vsctl set interface p1 cfm_mpid=2])
> >>> > for i in `seq 0 10`; do ovs-appctl time/warp 100; done
> >>> > CFM_VSCTL_LIST_IFACE([p0], [cfm_flap_count], [cfm_flap_count :
> 2])
> >>> > -CFM_VSCTL_LIST_IFACE([p1], [cfm_flap_count], [cfm_flap_count :
> 0])
> >>> > +CFM_VSCTL_LIST_IFACE([p1], [cfm_flap_count], [cfm_flap_count :
> 2])
> >>> >
> >>> > OVS_VSWITCHD_STOP
> >>> > AT_CLEANUP
> >>> > \ No newline at end of file
> >>> > --
> >>> > 1.7.9.5
> >>> >
> >>
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20131213/cb7f8373/attachment-0003.html>
More information about the dev
mailing list