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

[Daniele Di Proietto]

Hi Mark,

On 14/04/2016 05:36, "Kavanagh, Mark B"  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:
>
>
> For example, poll_block() includes a quiescent state, as does
>ovs_mutex_cond_wait().
>

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 
>>---
>> 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, >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();
>> 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(, );
>>+ovsrcu_quiesce_end();
>> }
>> ovs_mutex_unlock();
>>
>>--
>>2.1.4
>

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [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:


 For example, poll_block() includes a quiescent state, as does 
ovs_mutex_cond_wait().


>
>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 
>---
> 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, >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();
> 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(, );
>+ovsrcu_quiesce_end();
> }
> ovs_mutex_unlock();
>
>--
>2.1.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

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

[Daniele Di Proietto]

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.

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 
---
 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, >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();
 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(, );
+ovsrcu_quiesce_end();
 }
 ovs_mutex_unlock();
 
-- 
2.1.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev