[ovs-dev] [PATCH v7 11/16] ovs-thread: Do not quiesce in ovs_mutex_cond_wait().

[Kavanagh, Mark B]

Hi Daniele,

One comment inline.

Cheers,
Mark

>
>ovs_mutex_cond_wait() is used in many functions in dpif-netdev to
>synchronize with pmd threads, but we can't guarantee that the callers do
>not hold RCU references, so it's better to avoid quiescing.

You'll need to update the following comment in lib/rcu.h accordingly:

<snip>
 For example, poll_block() includes a quiescent state, as does ovs_mutex_cond_wait().
</snip>

>
>In system_stats_thread_func() the code relied on ovs_mutex_cond_wait()
>to introduce a quiescent state, so explicit calls to
>ovsrcu_quiesce_start() and ovsrcu_quiesce_end() are added there.
>
>Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
>---
> lib/ovs-thread.c        | 2 --
> vswitchd/system-stats.c | 6 ++++++
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
>diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
>index 3c065cf..26dd928 100644
>--- a/lib/ovs-thread.c
>+++ b/lib/ovs-thread.c
>@@ -253,9 +253,7 @@ ovs_mutex_cond_wait(pthread_cond_t *cond, const struct ovs_mutex *mutex_)
>     struct ovs_mutex *mutex = CONST_CAST(struct ovs_mutex *, mutex_);
>     int error;
>
>-    ovsrcu_quiesce_start();
>     error = pthread_cond_wait(cond, &mutex->lock);
>-    ovsrcu_quiesce_end();
>
>     if (OVS_UNLIKELY(error)) {
>         ovs_abort(error, "pthread_cond_wait failed");
>diff --git a/vswitchd/system-stats.c b/vswitchd/system-stats.c
>index df4971e..129f0cf 100644
>--- a/vswitchd/system-stats.c
>+++ b/vswitchd/system-stats.c
>@@ -37,6 +37,7 @@
> #include "json.h"
> #include "latch.h"
> #include "openvswitch/ofpbuf.h"
>+#include "ovs-rcu.h"
> #include "ovs-thread.h"
> #include "poll-loop.h"
> #include "shash.h"
>@@ -615,7 +616,12 @@ system_stats_thread_func(void *arg OVS_UNUSED)
>
>         ovs_mutex_lock(&mutex);
>         while (!enabled) {
>+            /* The thread is sleeping, potentially for a long time, and it's
>+             * not holding RCU protected references, so it makes sense to
>+             * quiesce */
>+            ovsrcu_quiesce_start();
>             ovs_mutex_cond_wait(&cond, &mutex);
>+            ovsrcu_quiesce_end();
>         }
>         ovs_mutex_unlock(&mutex);
>
>--
>2.1.4