diff options
author | Daniele Di Proietto <diproiettod@vmware.com> | 2016-03-23 16:37:47 -0700 |
---|---|---|
committer | Daniele Di Proietto <diproiettod@vmware.com> | 2016-03-25 11:38:59 -0700 |
commit | 88b567e66aafc3509ecd6aae6bebd498701b0b83 (patch) | |
tree | cf1d09f7fa38f2b63346425269d40dff34f6248b | |
parent | 2f22d6a2c8e58c986ec41f6074959a6dc47c35b2 (diff) | |
download | openvswitch-88b567e66aafc3509ecd6aae6bebd498701b0b83.tar.gz |
ovs-thread: Do not always end quiescent state in ovs_thread_create().
A new thread must be started in a non quiescent state. There is a call
to ovsrcu_quiesce_end() in ovsthread_wrapper(), to enforce this.
ovs_thread_create(), instead, is executed in the parent thread. It must
call ovsrcu_quiesce_end() on its first invocation, to put the main
thread in a non quiescent state. On every other invocation, it doesn't
make sense to alter the calling thread state, so this commits wraps the
call to ovsrcu_quiesce_end() in an ovsthread_once construct.
This fixes a bug in ovs-rcu where the first call in the process to
ovsrcu_quiesce_start() will not be honored, because the calling thread
will need to create the 'urcu' thread (and creating a thread will
wrongly end its quiescent state).
ovsrcu_quiesce_start()
ovs_rcu_quiesced()
if (ovsthread_once_start(&once)) {
ovs_thread_create("urcu") /*This will end the quiescent state*/
}
This bug affects in particular ovs-vswitchd with DPDK.
In the DPDK case the first threads created are "vhost_thread" and
"dpdk_watchdog". If dpdk_watchdog is the first to call
ovsrcu_quiesce_start() (via xsleep()), the call is not honored and
the RCU grace period lasts at least for DPDK_PORT_WATCHDOG_INTERVAL
(5s on current master). If vhost_thread, on the other hand, is the
first to call ovsrcu_quiesce_start(), the call is not honored and the
RCU grace period lasts undefinitely, because no more calls to
ovsrcu_quiesce_start() are issued from vhost_thread.
For some reason (it's a race condition after all), on current master,
dpdk_watchdog will always be the first to call ovsrcu_quiesce_start(),
but with the upcoming DPDK database configuration changes, sometimes
vhost_thread will issue the first call to ovsrcu_quiesce_start().
Sample ovs-vswitchd.log:
2016-03-23T22:34:28.532Z|00004|ovs_rcu(urcu3)|WARN|blocked 8000 ms
waiting for vhost_thread2 to quiesce
2016-03-23T22:34:30.501Z|00118|ovs_rcu|WARN|blocked 8000 ms waiting for
vhost_thread2 to quiesce
2016-03-23T22:34:36.532Z|00005|ovs_rcu(urcu3)|WARN|blocked 16000 ms
waiting for vhost_thread2 to quiesce
2016-03-23T22:34:38.501Z|00119|ovs_rcu|WARN|blocked 16000 ms waiting for
vhost_thread2 to quiesce
The commit also adds a test for the ovs-rcu module to make sure that:
* A new thread is started in a non quiescent state.
* The first call to ovsrcu_quiesce_start() is honored.
* When a process becomes multithreaded the main thread is put in an
active state
Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com>
Acked-by: Ben Pfaff <blp@ovn.org>
-rw-r--r-- | lib/ovs-rcu.h | 3 | ||||
-rw-r--r-- | lib/ovs-thread.c | 17 | ||||
-rw-r--r-- | tests/automake.mk | 1 | ||||
-rw-r--r-- | tests/library.at | 4 | ||||
-rw-r--r-- | tests/test-rcu.c | 52 |
5 files changed, 75 insertions, 2 deletions
diff --git a/lib/ovs-rcu.h b/lib/ovs-rcu.h index 110129c89..41f8210b9 100644 --- a/lib/ovs-rcu.h +++ b/lib/ovs-rcu.h @@ -56,7 +56,8 @@ * * Brackets a time period during which the current thread is quiescent. * - * A newly created thread is initially active, not quiescent. + * A newly created thread is initially active, not quiescent. When a process + * becomes multithreaded, the main thread becomes active, not quiescent. * * When a quiescient state has occurred in every thread, we say that a "grace * period" has occurred. Following a grace period, all of the callbacks diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c index 7c1888dfe..240c0ae91 100644 --- a/lib/ovs-thread.c +++ b/lib/ovs-thread.c @@ -327,13 +327,28 @@ ovsthread_wrapper(void *aux_) pthread_t ovs_thread_create(const char *name, void *(*start)(void *), void *arg) { + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; struct ovsthread_aux *aux; pthread_t thread; int error; forbid_forking("multiple threads exist"); multithreaded = true; - ovsrcu_quiesce_end(); + + if (ovsthread_once_start(&once)) { + /* The first call to this function has to happen in the main thread. + * Before the process becomes multithreaded we make sure that the + * main thread is considered non quiescent. + * + * For other threads this is done in ovs_thread_wrapper(), but the + * main thread has no such wrapper. + * + * There's no reason to call ovsrcu_quiesce_end() in subsequent + * invocations of this function and it might introduce problems + * for other threads. */ + ovsrcu_quiesce_end(); + ovsthread_once_done(&once); + } aux = xmalloc(sizeof *aux); aux->start = start; diff --git a/tests/automake.mk b/tests/automake.mk index c115289a4..983835f0b 100644 --- a/tests/automake.mk +++ b/tests/automake.mk @@ -229,6 +229,7 @@ tests_ovstest_SOURCES = \ tests/test-odp.c \ tests/test-packets.c \ tests/test-random.c \ + tests/test-rcu.c \ tests/test-reconnect.c \ tests/test-sflow.c \ tests/test-sha1.c \ diff --git a/tests/library.at b/tests/library.at index 01607473c..82cf2bdb7 100644 --- a/tests/library.at +++ b/tests/library.at @@ -187,3 +187,7 @@ AT_CLEANUP AT_SETUP([snprintf]) AT_CHECK([ovstest test-util snprintf]) AT_CLEANUP + +AT_SETUP([test rcu]) +AT_CHECK([ovstest test-rcu-quiesce], [0], []) +AT_CLEANUP diff --git a/tests/test-rcu.c b/tests/test-rcu.c new file mode 100644 index 000000000..e66367e83 --- /dev/null +++ b/tests/test-rcu.c @@ -0,0 +1,52 @@ +/* + * Copyright (c) 2016 Nicira, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include <config.h> +#undef NDEBUG +#include "fatal-signal.h" +#include "ovs-rcu.h" +#include "ovs-thread.h" +#include "ovstest.h" +#include "util.h" + +static void * +quiescer_main(void *aux OVS_UNUSED) +{ + /* A new thread must be not be quiescent */ + ovs_assert(!ovsrcu_is_quiescent()); + ovsrcu_quiesce_start(); + /* After the above call it must be quiescent */ + ovs_assert(ovsrcu_is_quiescent()); + + return NULL; +} + +static void +test_rcu_quiesce(int argc OVS_UNUSED, char *argv[] OVS_UNUSED) +{ + pthread_t quiescer; + + fatal_signal_init(); + quiescer = ovs_thread_create("quiescer", quiescer_main, NULL); + + /* This is the main thread of the process. After spawning its first + * thread it must not be quiescent. */ + ovs_assert(!ovsrcu_is_quiescent()); + + xpthread_join(quiescer, NULL); +} + +OVSTEST_REGISTER("test-rcu-quiesce", test_rcu_quiesce); |