i#239: Synch with all other threads on fork.

i#239: Synch with all other threads on fork.

This helps to ensure that all DR locks are released when we begin
execution in the child.  If the app is spawning threads and forking
concurrently, then we may fail to synch with a recently spawned thread
due to issue 26, and so we may still have possible leaks and deadlocks
in the child process.

[Reid Kleckner (google), 2012-05-25 18:40:24]

I measured this change on make inside DR's source tree and it had ~0.8% perf
overhead.  Seems pretty reasonable to me.

My intuition is that apps that fork a lot (like scripts) have fewer threads, so
the synch isn't expensive.  If we encounter a big multithreaded app that wants
to fork lots of little tiny processes, this may become a bottleneck.

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c
File core/linux/os.c (right):

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c#newcode2132
core/linux/os.c:2132: if
(!synch_with_all_threads(THREAD_SYNCH_SUSPENDED_VALID_MCONTEXT_OR_NO_XFER,
Please double check requested state.

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c#newcode2155
core/linux/os.c:2155: mutex_unlock(&all_threads_synch_lock);
Also not sure about this argument.

http://codereview.appspot.com/6247048/diff/7001/core/linux/signal.c
File core/linux/signal.c (right):

http://codereview.appspot.com/6247048/diff/7001/core/linux/signal.c#newcode1470
core/linux/signal.c:1470: global_heap_free(info->itimer, sizeof(*info->itimer)
HEAPACCT(ACCT_OTHER));
Previously this was overwritten in init_timer and leaked.

[bruening, 2012-05-29 17:27:19]

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c
File core/linux/os.c (right):

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c#newcode2132
core/linux/os.c:2132: if
(!synch_with_all_threads(THREAD_SYNCH_SUSPENDED_VALID_MCONTEXT_OR_NO_XFER,
looks right

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c#newcode2135
core/linux/os.c:2135: THREAD_SYNCH_NO_LOCKS_NO_XFER,
you've got valid app state and could re-enter so why not
THREAD_SYNCH_VALID_MCONTEXT?

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c#newcode2137
core/linux/os.c:2137: * risk of deadlock in the child, so its worth
it's

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c#newcode2151
core/linux/os.c:2151: * release these locks.  If not, a rogue thread may suspend
us near the fork
even on success there is a corner case here.  new threads could be created
post-synchall-list and they could start grabbing DR locks once you release
thread_initexit_lock but before the fork syscall issues.  this can happen in a
number of ways on windows.  on linux it's rarer but can still happen with a
non-suspendable client thread or possibly another process using ptrace to run
some code to create a thread.  add comment that there are corner cases that this
code does not handle.

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c#newcode2154
core/linux/os.c:2154: mutex_unlock(&thread_initexit_lock);
if the synchall failed the locks won't be held so don't try to unlock them in
that case (could assert they're not held)

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c#newcode2155
core/linux/os.c:2155: mutex_unlock(&all_threads_synch_lock);
On 2012/05/25 18:40:24, Reid Kleckner (google) wrote:
> Also not sure about this argument.

not sure what you're asking.  slight break of abstraction barrier to directly
unlock these instead of using end_synch_with_all_threads() but seems warranted

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c#newcode2158
core/linux/os.c:2158: /* Happens in after the fork in both the parent and child.
*/
"in after"

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c#newcode2165
core/linux/os.c:2165: mutex_lock(&thread_initexit_lock);
in the child this is called prior to the mutex_fork_reset of this lock which
means we'll hang before we hit the reset if the synchall failed.  that code in
dynamo.c should be moved up before the call os_fork_init().

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c#newcode2177
core/linux/os.c:2177: /* Called before child's new logfiles are set up */
nit: superfluous change (pet peeve of mine)

http://codereview.appspot.com/6247048/diff/7001/suite/tests/pthreads/pthreads...
File suite/tests/pthreads/pthreads_fork.c (right):

http://codereview.appspot.com/6247048/diff/7001/suite/tests/pthreads/pthreads...
suite/tests/pthreads/pthreads_fork.c:83: fprintf(stderr, "child process\n");
convention is to use print() from tools.h in all new tests

http://codereview.appspot.com/6247048/diff/7001/suite/tests/pthreads/pthreads...
suite/tests/pthreads/pthreads_fork.c:96: fprintf(stderr, "child %d exited
non-zero: %x\n", pid, status);
ditto, here and below

http://codereview.appspot.com/6247048/diff/7001/suite/tests/pthreads/pthreads...
suite/tests/pthreads/pthreads_fork.c:145: #ifdef USE_DYNAMO
could remove all this USE_DYNAMO stuff

[Reid Kleckner (google), 2012-05-29 18:36:57]

New diff.

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c
File core/linux/os.c (right):

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c#newcode2135
core/linux/os.c:2135: THREAD_SYNCH_NO_LOCKS_NO_XFER,
On 2012/05/29 17:27:19, bruening wrote:
> you've got valid app state and could re-enter so why not
> THREAD_SYNCH_VALID_MCONTEXT?

Sounds good.

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c#newcode2154
core/linux/os.c:2154: mutex_unlock(&thread_initexit_lock);
On 2012/05/29 17:27:19, bruening wrote:
> if the synchall failed the locks won't be held so don't try to unlock them in
> that case (could assert they're not held)

That's only if I asked the synch to abort on failure.  Without that flag, some
threads are suspended while others are not, and these locks are always held on
return.

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c#newcode2165
core/linux/os.c:2165: mutex_lock(&thread_initexit_lock);
On 2012/05/29 17:27:19, bruening wrote:
> in the child this is called prior to the mutex_fork_reset of this lock which
> means we'll hang before we hit the reset if the synchall failed.  that code in
> dynamo.c should be moved up before the call os_fork_init().

Good catch.

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c#newcode2177
core/linux/os.c:2177: /* Called before child's new logfiles are set up */
On 2012/05/29 17:27:19, bruening wrote:
> nit: superfluous change (pet peeve of mine)

Fixed.  I had something else to say here, but I got rid of it.

[bruening, 2012-05-30 13:53:59]

LGTM w/ comments on comments

http://codereview.appspot.com/6247048/diff/7001/core/dynamo.c
File core/dynamo.c (right):

http://codereview.appspot.com/6247048/diff/7001/core/dynamo.c#newcode756
core/dynamo.c:756: * We can't call ASSIGN_INIT_LOCK_FREE as that clobbers any
contention event
you lost this comment

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c
File core/linux/os.c (right):

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c#newcode2154
core/linux/os.c:2154: mutex_unlock(&thread_initexit_lock);
On 2012/05/29 18:36:57, Reid Kleckner (google) wrote:
> On 2012/05/29 17:27:19, bruening wrote:
> > if the synchall failed the locks won't be held so don't try to unlock them
in
> > that case (could assert they're not held)
> 
> That's only if I asked the synch to abort on failure.  Without that flag, some
> threads are suspended while others are not, and these locks are always held on
> return.

Hmm, I don't think there's currently any use case in DR that asks for that.  Can
you update the line in synch_with_all_threads() header that when using RETRY,
end_synch*() should be called regardless of success value (modify the current
line about caller calling end_)

http://codereview.appspot.com/6247048/diff/11001/core/linux/os.c
File core/linux/os.c (right):

http://codereview.appspot.com/6247048/diff/11001/core/linux/os.c#newcode2152
core/linux/os.c:2152: * injects new threads via ptrace, then releasing these
locks will allow
non-suspendable client thread seems far more likely (in fact Dr. Heapstat has
one) so seems more worth mentioning than ptrace (synchall will still succeed in
that case btw)

http://codereview.appspot.com/6247048/diff/11001/core/linux/os.c#newcode2190
core/linux/os.c:2190: * fork because of synch failure, we can lock and data
resets on a case by
+ "perform"

[Reid Kleckner (google), 2012-05-30 14:40:56]

I uploaded the last comment changes, and I'll commit soon.

http://codereview.appspot.com/6247048/diff/7001/core/dynamo.c
File core/dynamo.c (right):

http://codereview.appspot.com/6247048/diff/7001/core/dynamo.c#newcode756
core/dynamo.c:756: * We can't call ASSIGN_INIT_LOCK_FREE as that clobbers any
contention event
On 2012/05/30 13:53:59, bruening wrote:
> you lost this comment

There's an identical copy in mutex_fork_reset, which is a more appropriate place
to describe the reset implementation.

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c
File core/linux/os.c (right):

http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c#newcode2154
core/linux/os.c:2154: mutex_unlock(&thread_initexit_lock);
On 2012/05/30 13:53:59, bruening wrote:
> On 2012/05/29 18:36:57, Reid Kleckner (google) wrote:
> > On 2012/05/29 17:27:19, bruening wrote:
> > > if the synchall failed the locks won't be held so don't try to unlock them
> in
> > > that case (could assert they're not held)
> > 
> > That's only if I asked the synch to abort on failure.  Without that flag,
some
> > threads are suspended while others are not, and these locks are always held
on
> > return.
> 
> Hmm, I don't think there's currently any use case in DR that asks for that. 
Can
> you update the line in synch_with_all_threads() header that when using RETRY,
> end_synch*() should be called regardless of success value (modify the current
> line about caller calling end_)

I believe the caller always has to call end_synch_with_all_threads() to clean
up, unless the passed THREAD_SYNCH_SUSPEND_FAILURE_ABORT.

http://codereview.appspot.com/6247048/diff/11001/core/linux/os.c
File core/linux/os.c (right):

http://codereview.appspot.com/6247048/diff/11001/core/linux/os.c#newcode2152
core/linux/os.c:2152: * injects new threads via ptrace, then releasing these
locks will allow
On 2012/05/30 13:53:59, bruening wrote:
> non-suspendable client thread seems far more likely (in fact Dr. Heapstat has
> one) so seems more worth mentioning than ptrace (synchall will still succeed
in
> that case btw)

Done.

http://codereview.appspot.com/6247048/diff/11001/core/linux/os.c#newcode2190
core/linux/os.c:2190: * fork because of synch failure, we can lock and data
resets on a case by
On 2012/05/30 13:53:59, bruening wrote:
> + "perform"

Done.

[Reid Kleckner (google), 2012-05-30 15:33:14]

Hm, the pthreads_fork test fails if I run it in a loop and load my machine
to produce other interleavings.

For example, if we try to suspend on fork, we send SUSPEND_SIGNAL to
threads during initialization, and depending on how they add themselves to
all_threads, we may attempt takeover.

I'll work on this some more and upload a new patch.

Reid

On Wed, May 30, 2012 at 10:40 AM, <rnk@google.com> wrote:

> I uploaded the last comment changes, and I'll commit soon.
>
>
>
>
http://codereview.appspot.com/**6247048/diff/7001/core/dynamo.**c<http://code...
> File core/dynamo.c (right):
>
> http://codereview.appspot.com/**6247048/diff/7001/core/dynamo.**
>
c#newcode756<http://codereview.appspot.com/6247048/diff/7001/core/dynamo.c#newcode756>
> core/dynamo.c:756: * We can't call ASSIGN_INIT_LOCK_FREE as that
> clobbers any contention event
> On 2012/05/30 13:53:59, bruening wrote:
>
>> you lost this comment
>>
>
> There's an identical copy in mutex_fork_reset, which is a more
> appropriate place to describe the reset implementation.
>
>
>
http://codereview.appspot.com/**6247048/diff/7001/core/linux/**os.c<http://co...
> File core/linux/os.c (right):
>
> http://codereview.appspot.com/**6247048/diff/7001/core/linux/**
>
os.c#newcode2154<http://codereview.appspot.com/6247048/diff/7001/core/linux/os.c#newcode2154>
> core/linux/os.c:2154: mutex_unlock(&thread_initexit_**lock);
> On 2012/05/30 13:53:59, bruening wrote:
>
>> On 2012/05/29 18:36:57, Reid Kleckner (google) wrote:
>> > On 2012/05/29 17:27:19, bruening wrote:
>> > > if the synchall failed the locks won't be held so don't try to
>>
> unlock them
>
>> in
>> > > that case (could assert they're not held)
>> >
>> > That's only if I asked the synch to abort on failure.  Without that
>>
> flag, some
>
>> > threads are suspended while others are not, and these locks are
>>
> always held on
>
>> > return.
>>
>
>  Hmm, I don't think there's currently any use case in DR that asks for
>>
> that.  Can
>
>> you update the line in synch_with_all_threads() header that when using
>>
> RETRY,
>
>> end_synch*() should be called regardless of success value (modify the
>>
> current
>
>> line about caller calling end_)
>>
>
> I believe the caller always has to call end_synch_with_all_threads() to
> clean up, unless the passed THREAD_SYNCH_SUSPEND_FAILURE_**ABORT.
>
>
>
http://codereview.appspot.com/**6247048/diff/11001/core/linux/**os.c<http://c...
> File core/linux/os.c (right):
>
> http://codereview.appspot.com/**6247048/diff/11001/core/linux/**
>
os.c#newcode2152<http://codereview.appspot.com/6247048/diff/11001/core/linux/os.c#newcode2152>
> core/linux/os.c:2152: * injects new threads via ptrace, then releasing
> these locks will allow
> On 2012/05/30 13:53:59, bruening wrote:
>
>> non-suspendable client thread seems far more likely (in fact Dr.
>>
> Heapstat has
>
>> one) so seems more worth mentioning than ptrace (synchall will still
>>
> succeed in
>
>> that case btw)
>>
>
> Done.
>
>
> http://codereview.appspot.com/**6247048/diff/11001/core/linux/**
>
os.c#newcode2190<http://codereview.appspot.com/6247048/diff/11001/core/linux/os.c#newcode2190>
> core/linux/os.c:2190: * fork because of synch failure, we can lock and
> data resets on a case by
> On 2012/05/30 13:53:59, bruening wrote:
>
>> + "perform"
>>
>
> Done.
>
>
http://codereview.appspot.com/**6247048/<http://codereview.appspot.com/6247048/>
>

[Reid Kleckner (google), 2012-05-30 17:34:09]

The test is stable now, but we still have a problem with concurrently spawning
threads and forking, as described in the issue.

The current change still helps a lot, so I think we should go ahead and commit
it and come up with a full issue 26 solution later.  OK to commit?

http://codereview.appspot.com/6247048/diff/15001/suite/tests/CMakeLists.txt
File suite/tests/CMakeLists.txt (right):

http://codereview.appspot.com/6247048/diff/15001/suite/tests/CMakeLists.txt#n...
suite/tests/CMakeLists.txt:1409: tobuild_ci(client.retaddr
client-interface/retaddr.c "" "" "")
This is an artifact of rebasing, which I did because I was preparing to commit.

[bruening, 2012-05-30 22:04:52]

LGTM w/ one question

http://codereview.appspot.com/6247048/diff/15001/core/linux/signal.c
File core/linux/signal.c (right):

http://codereview.appspot.com/6247048/diff/15001/core/linux/signal.c#newcode3805
core/linux/signal.c:3805: dcontext == NULL &&
if you add this can't you remove the thread_lookup check?

[Reid Kleckner (google), 2012-06-04 15:08:46]

Committing after suite.

http://codereview.appspot.com/6247048/diff/15001/core/linux/signal.c
File core/linux/signal.c (right):

http://codereview.appspot.com/6247048/diff/15001/core/linux/signal.c#newcode3805
core/linux/signal.c:3805: dcontext == NULL &&
On 2012/05/30 22:04:52, bruening wrote:
> if you add this can't you remove the thread_lookup check?

Sure.  The only way that might break is if there was a native thread that had no
dcontext that was present in all_threads but we wanted it to remain native when
we took over.  I can't imagine why we'd want that.