From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32937) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZZbmY-0007FQ-4M for qemu-devel@nongnu.org; Wed, 09 Sep 2015 05:35:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZZbmU-0000W9-4i for qemu-devel@nongnu.org; Wed, 09 Sep 2015 05:35:10 -0400 Received: from mail-wi0-f175.google.com ([209.85.212.175]:33762) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZZbmT-0000Uc-Ve for qemu-devel@nongnu.org; Wed, 09 Sep 2015 05:35:06 -0400 Received: by wiclk2 with SMTP id lk2so149828885wic.0 for ; Wed, 09 Sep 2015 02:35:05 -0700 (PDT) References: <1440375847-17603-1-git-send-email-cota@braap.org> <1440375847-17603-9-git-send-email-cota@braap.org> <877fo0ak7l.fsf@linaro.org> <20150908190342.GA17722@flamenco> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <20150908190342.GA17722@flamenco> Date: Wed, 09 Sep 2015 10:35:03 +0100 Message-ID: <87613k9bqw.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [RFC 08/38] rcu: init rcu_registry_lock after fork List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Emilio G. Cota" Cc: mttcg@listserver.greensocs.com, mark.burton@greensocs.com, a.rigo@virtualopensystems.com, qemu-devel@nongnu.org, guillaume.delbergue@greensocs.com, pbonzini@redhat.com, Frederic Konrad Emilio G. Cota writes: > On Tue, Sep 08, 2015 at 18:34:38 +0100, Alex Bennée wrote: >> Emilio G. Cota 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