Re: [Qemu-devel] [RFC 08/38] rcu: init rcu_registry_lock after fork

[Alex Bennée]

Emilio G. Cota <address@hidden> writes:

> On Tue, Sep 08, 2015 at 18:34:38 +0100, Alex Bennée wrote:
>> Emilio G. Cota <address@hidden> writes:
> (snip)
>> > +static void rcu_init_child(void)
>> > +{
>> > +    qemu_mutex_init(&rcu_registry_lock);
>> > +}
>> >  #endif
>> >  
>> >  void rcu_after_fork(void)
>> > @@ -346,7 +351,7 @@ void rcu_after_fork(void)
>> >  static void __attribute__((__constructor__)) rcu_init(void)
>> >  {
>> >  #ifdef CONFIG_POSIX
>> > -    pthread_atfork(rcu_init_lock, rcu_init_unlock, rcu_init_unlock);
>> > +    pthread_atfork(rcu_init_lock, rcu_init_unlock, rcu_init_child);
>> >  #endif
>> 
>> Hmm previously we unlocked both rcu_sync_lock and rcu_registry_lock, is
>> it somehow different in it's locking rules? If I'm reading the
>> pthread_atfork man page right couldn't we just do:
>> 
>>     pthread_atfork(rcu_init_lock, rcu_init_unlock, rcu_init_lock);
>
> That'd cause the child to deadlock.
>
> Before forking the parent locks those mutexes; then it forks. The child sees
> those mutexes as 'locked', and whatever memory the parent writes to _after_
> fork is not seen by the child. So trying to 'lock' those mutexes from the 
> child
> is never going to succeed, because they're seeing as already locked.

Doh apologies I misread the code. What caught my eye is the old code
locks/unlocks both rcu_registry_lock and rcu_sync_lock so shouldn't the
child function reinit both?

static void rcu_init_child(void)
{
    qemu_mutex_init(&rcu_sync_lock);
    qemu_mutex_init(&rcu_registry_lock);
}


>
> The original idea ("unlock" from the child) is OK, but doesn't work when
> using PTHREAD_MUTEX_ERRORCHECK. Yes, this was removed (24fa90499f) but
> it's too useful to not cherry-pick it when developing MTTCG.
>
> What I think remains to be fixed is the possibility of corruption when
> any call_rcu calls are pending while forking. The child should not mess
> with these; only the parent should. See call_rcu_after_fork_{parent,child}
> from liburcu for details.
>
>               Emilio

-- 
Alex Bennée