Hi Mark,
On 14/04/2016 05:36, "Kavanagh, Mark B" <mark.b.kavan...@intel.com> wrote:
>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>
You're right, I removed the reference to ovs_mutex_cond_wait() there.
Thanks for noticing this!
>
>>
>>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 <diproiet...@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
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
