diff mbox

rcu: actually register threads that have RCU read-side critical sections

Message ID 55B1D380.7020300@cn.fujitsu.com
State New
Headers show

Commit Message

Wen Congyang July 24, 2015, 5:56 a.m. UTC 
On 07/24/2015 12:58 AM, Paolo Bonzini wrote:
> 
> 
> On 23/07/2015 14:59, Wen Congyang wrote:
>>>>
>>>> If the thread doesn't use RCU, rcu_register_thread() is harmless, is
>>>> it right?
>>>
>>> Every rcu_register_thread() makes synchronize_rcu() a little slower.
>>
>> Yes, but synchronize_rcu() is very slow...
> 
> Hmm, worse, rcu_register_thread() if called together with
> synchronize_rcu() it waits for the synchronize_rcu() to finish. :/

What about this modification:
rcu_register_thread() will be a littl slower when it is
called together with synchronize_rcu().

Thanks
Wen Congyang

> 
> Paolo
> 
>>>
>>>>>> be simpler to add an assertion in rcu_register_thread.  I'm just a bit
>>>>>> wary of doing little more than the bare minimum in 2.4, because of the
>>>>>> OS X failure that I didn't quite understand.
>>>> Which problem? I don't find it in the maillist.
>>>
>>> http://article.gmane.org/gmane.comp.emulators.qemu/351548
>>
>> Hmm, I guess rcu_reader is invalid when pthread key is destroyed.
>> pthread key and __thread
>> variable, which is destroyed first? I don't find any document to
>> describe it.
>

Comments

Paolo Bonzini July 24, 2015, 6:22 a.m. UTC | #1 
On 24/07/2015 07:56, Wen Congyang wrote:
> @@ -115,9 +116,12 @@ static void wait_for_readers(void)
>          }
>  
>          /* Wait for one thread to report a quiescent state and
> -         * try again.
> +         * try again. Release rcu_gp_lock, so rcu_(un)register_thread()
> +         * doesn't wait too much time.
>           */
> +        qemu_mutex_unlock(&rcu_gp_lock);
>          qemu_event_wait(&rcu_gp_event);
> +        qemu_mutex_lock(&rcu_gp_lock);
>      }
>  

So in this case rcu_unregister_thread could actually remove the node
from synchronize_rcu's qsreaders, not just from registry.  That's a bit
tricky, but it should work.  Please add a comment, however.

Also, please rename "rcu_gp_lock" as well to rcu_registry_lock.  We'll
get the patches in QEMU 2.5.

Paolo
Wen Congyang July 24, 2015, 6:30 a.m. UTC | #2 
On 07/24/2015 02:22 PM, Paolo Bonzini wrote:
> 
> 
> On 24/07/2015 07:56, Wen Congyang wrote:
>> @@ -115,9 +116,12 @@ static void wait_for_readers(void)
>>          }
>>  
>>          /* Wait for one thread to report a quiescent state and
>> -         * try again.
>> +         * try again. Release rcu_gp_lock, so rcu_(un)register_thread()
>> +         * doesn't wait too much time.
>>           */
>> +        qemu_mutex_unlock(&rcu_gp_lock);
>>          qemu_event_wait(&rcu_gp_event);
>> +        qemu_mutex_lock(&rcu_gp_lock);
>>      }
>>  
> 
> So in this case rcu_unregister_thread could actually remove the node
> from synchronize_rcu's qsreaders, not just from registry.  That's a bit
> tricky, but it should work.  Please add a comment, however.
> 
> Also, please rename "rcu_gp_lock" as well to rcu_registry_lock.  We'll
> get the patches in QEMU 2.5.


OK, I will do it.

Thanks
Wen Congyang

> 
> Paolo
> .
>
diff mbox

Patch

diff --git a/util/rcu.c b/util/rcu.c
index 7270151..ccf8cfa 100644
--- a/util/rcu.c
+++ b/util/rcu.c
@@ -48,6 +48,7 @@  unsigned long rcu_gp_ctr = RCU_GP_LOCKED;
 
 QemuEvent rcu_gp_event;
 static QemuMutex rcu_gp_lock;
+static QemuMutex rcu_sync_lock;
 
 /*
  * Check whether a quiescent state was crossed between the beginning of
@@ -115,9 +116,12 @@  static void wait_for_readers(void)
         }
 
         /* Wait for one thread to report a quiescent state and
-         * try again.
+         * try again. Release rcu_gp_lock, so rcu_(un)register_thread()
+         * doesn't wait too much time.
          */
+        qemu_mutex_unlock(&rcu_gp_lock);
         qemu_event_wait(&rcu_gp_event);
+        qemu_mutex_lock(&rcu_gp_lock);
     }
 
     /* put back the reader list in the registry */
@@ -126,6 +130,7 @@  static void wait_for_readers(void)
 
 void synchronize_rcu(void)
 {
+    qemu_mutex_lock(&rcu_sync_lock);
     qemu_mutex_lock(&rcu_gp_lock);
 
     if (!QLIST_EMPTY(&registry)) {
@@ -150,6 +155,7 @@  void synchronize_rcu(void)
     }
 
     qemu_mutex_unlock(&rcu_gp_lock);
+    qemu_mutex_unlock(&rcu_sync_lock);
 }
 
 
@@ -288,6 +294,7 @@  static void rcu_init_complete(void)
     QemuThread thread;
 
     qemu_mutex_init(&rcu_gp_lock);
+    qemu_mutex_init(&rcu_sync_lock);
     qemu_event_init(&rcu_gp_event, true);
 
     qemu_event_init(&rcu_call_ready_event, false);
@@ -304,12 +311,14 @@  static void rcu_init_complete(void)
 #ifdef CONFIG_POSIX
 static void rcu_init_lock(void)
 {
+    qemu_mutex_lock(&rcu_sync_lock);
     qemu_mutex_lock(&rcu_gp_lock);
 }
 
 static void rcu_init_unlock(void)
 {
     qemu_mutex_unlock(&rcu_gp_lock);
+    qemu_mutex_unlock(&rcu_sync_lock);
 }
 #endif