Re: [ovs-dev] [PATCH v7 11/16] ovs-thread: Do not quiesce in ovs_mutex_cond_wait().
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().
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().
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