1121269 - [nuwa] __wrap_pthread_key_create causes deadlock in jemalloc3

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Eric Rahm [:erahm]]

See bug 762449, comment 24 for details. On B2G emulators we're seeing a deadlock in where we initialize jemalloc, that in turn calls __wrap_pthread_key_create, which then tries to use a std::map that then tries to initialize jemalloc again.

Comment 1

[Eric Rahm [:erahm]]

Attachment: Use static buffer for first allocation in __wrap_pthread_key_create

This is the same solution we used in mozglue/build/BionicGlue.cpp.

Comment 2

[Eric Rahm [:erahm]]

https://tbpl.mozilla.org/?tree=Try&rev=ee3d46d5fc5e

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8548550 [details] [diff] [review]
Use static buffer for first allocation in __wrap_pthread_key_create

Review of attachment 8548550 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/build/Nuwa.cpp
@@ +225,5 @@
> + * static buffer for small sizes, and force the initial vector capacity to
> + * a size enough to store one atfork function table.
> + */
> +template <typename T>
> +struct SpecialAllocator: public std::allocator<T>

Since this is the exact same class as in BionicGlue.cpp, please move it in a header. And put that header somewhere under memory/, like memory/build/.

@@ +259,5 @@
> + * NB: stlport seems to require an allocator for the internal node type rather
> + *     than just a std::pair<K,V>.
> + */
> +typedef std::map<pthread_key_t, void (*)(void *), std::less<pthread_key_t>,
> +        SpecialAllocator<std::priv::_Rb_tree_node<std::pair<const int, void (*)(void*)> > > > TLSKeySet;

This sucks, but let's keep it that way for the moment. Please file a followup to clean this up some way or another. I don't like to rely on private types.

Comment 4

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: An alternative fix

This is another way to fix the problem without resorting to a special allocator. We can skip the bookkeeping part in __wrap_pthread_key_create() for non-Nuwa processes. This should fix the problem during jemalloc initialization when the process is not recognize itself as Nuwa.

Comment 5

[Mike Hommey [:glandium]]

(In reply to Cervantes Yu from comment #4)
> Created attachment 8548728 [details] [diff] [review]
> An alternative fix
> 
> This is another way to fix the problem without resorting to a special
> allocator. We can skip the bookkeeping part in __wrap_pthread_key_create()
> for non-Nuwa processes. This should fix the problem during jemalloc
> initialization when the process is not recognize itself as Nuwa.

This changes what is stored in sTLSKeys, which changes what is saved and restored by SaveTLSInfo and RestoreTLSInfo. This may well be what was intended all along, but this is definitely a change in the current behavior. Note that pthread_key_delete only affects sTLSKeys in Nuwa processes.

So if in fact all pthread_keys need to be saved, Nuwa or not (and aiui, they should be), pthread_key_delete is currently wrong.

Comment 6

[Thinker Li [:sinker]]

Comment on attachment 8548728 [details] [diff] [review]
An alternative fix

Review of attachment 8548728 [details] [diff] [review]:
-----------------------------------------------------------------

In fact, the number of these keys are small.  A linear search at a static array is even faster than hash.  With a reasonable assertion on the maximum size, I would suggest to use a simple array XD

Comment 7

[Cervantes Yu [:cyu] [:cervantes]]

(In reply to Mike Hommey [:glandium] from comment #5)
> This changes what is stored in sTLSKeys, which changes what is saved and
> restored by SaveTLSInfo and RestoreTLSInfo. This may well be what was
> intended all along, but this is definitely a change in the current behavior.
> Note that pthread_key_delete only affects sTLSKeys in Nuwa processes.
> 
> So if in fact all pthread_keys need to be saved, Nuwa or not (and aiui, they
> should be), pthread_key_delete is currently wrong.

Oh! you are right. The change may make us lose jemalloc's TSD for the recreated thread in the threads that are cloned from Nuwa. Sounds pretty undesirable.

Comment 8

[Cervantes Yu [:cyu] [:cervantes]]

Comment on attachment 8548728 [details] [diff] [review]
An alternative fix

Obsolete the patch that is incorrect.

Comment 9

[Eric Rahm [:erahm]]

(In reply to Thinker Li [:sinker] from comment #6)
> Comment on attachment 8548728 [details] [diff] [review]
> An alternative fix
> 
> Review of attachment 8548728 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In fact, the number of these keys are small.  A linear search at a static
> array is even faster than hash.  With a reasonable assertion on the maximum
> size, I would suggest to use a simple array XD

I'm not sure we should set a limit on the number of threads a process can have.

Comment 10

[Mike Hommey [:glandium]]

Attachment: Use a LinkedList to store TLS keys for Nuwa, and allocate the first element in a static buffer

Comment 11

[Mike Hommey [:glandium]]

Attachment: Use a LinkedList to store TLS keys for Nuwa, and allocate the first element in a static buffer

Here is a version that, more than compile, links.

Comment 12

[Mike Hommey [:glandium]]

Attachment: Use a LinkedList to store TLS keys for Nuwa, and allocate the first element in a static buffer

Using AlignedStorage

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8549208 [details] [diff] [review]
Use a LinkedList to store TLS keys for Nuwa, and allocate the first element in a static buffer

Review of attachment 8549208 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/build/Nuwa.cpp
@@ +227,5 @@
> +  : std::pair<pthread_key_t, void (*)(void*)>(aKey, aDestructor)
> +  {}
> +
> +  static void* operator new(size_t size) {
> +    if (!sUsed)

Not attaching a new patch for this, but this should be if (sUsed) instead of if (!sUsed). Fixed locally.

Comment 14

[Mike Hommey [:glandium]]

Attachment: Use a LinkedList to store TLS keys for Nuwa, and allocate the first element in a static buffer

And avoid leaking the linked list elements at shutdown (and crash on debug builds because the list is not empty). Note that AllThreadListType was leaking its elements. (clear() doesn't delete them)

Comment 15

[Mike Hommey [:glandium]]

Attachment: Remove TLS keys from bookkeeping in non-Nuwa processes too

And this addresses comment 5.

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8549219 [details] [diff] [review]
Use a LinkedList to store TLS keys for Nuwa, and allocate the first element in a static buffer

Waldo, how would you feel about AutoCleanLinkedList from this patch being added to LinkedList.h?

Comment 17

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Mike Hommey [:glandium] from comment #15)
> Created attachment 8549220 [details] [diff] [review]
> Remove TLS keys from bookkeeping in non-Nuwa processes too
> 
> And this addresses comment 5.

Can we store the keys in static allocated array? The maximum number of key per process is bounded by PTHREAD_KEYS_MAX, the value is 1024 on my desktop linux machine.

Comment 18

[Mike Hommey [:glandium]]

(In reply to Patrick Wang (Chih-Kai Wang) (:kk1fff) from comment #17)
> (In reply to Mike Hommey [:glandium] from comment #15)
> > Created attachment 8549220 [details] [diff] [review]
> > Remove TLS keys from bookkeeping in non-Nuwa processes too
> > 
> > And this addresses comment 5.
> 
> Can we store the keys in static allocated array? The maximum number of key
> per process is bounded by PTHREAD_KEYS_MAX, the value is 1024 on my desktop
> linux machine.

Do we really need to allocate 8k on arm and 16k on arm64 for something that probably only uses a few items?

Comment 19

[Thinker Li [:sinker]]

Comment on attachment 8549219 [details] [diff] [review]
Use a LinkedList to store TLS keys for Nuwa, and allocate the first element in a static buffer

Review of attachment 8549219 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/build/Nuwa.cpp
@@ +247,5 @@
> +  static AlignedStorage2<TLSKey> sFirstElement;
> +};
> +
> +bool TLSKey::sUsed = false;
> +AlignedStorage2<TLSKey> TLSKey::sFirstElement;

I think this is a very fragile solution.  It is very likely we need |sSecondElement| in future, and we need to spend a bunch of time to figure it out again.  So, please make sure that the process would crash at first moment once it is happened.

Comment 20

[Mike Hommey [:glandium]]

(In reply to Thinker Li [:sinker] from comment #19)
> Comment on attachment 8549219 [details] [diff] [review]
> Use a LinkedList to store TLS keys for Nuwa, and allocate the first element
> in a static buffer
> 
> Review of attachment 8549219 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozglue/build/Nuwa.cpp
> @@ +247,5 @@
> > +  static AlignedStorage2<TLSKey> sFirstElement;
> > +};
> > +
> > +bool TLSKey::sUsed = false;
> > +AlignedStorage2<TLSKey> TLSKey::sFirstElement;
> 
> I think this is a very fragile solution.  It is very likely we need
> |sSecondElement| in future, and we need to spend a bunch of time to figure
> it out again.  So, please make sure that the process would crash at first
> moment once it is happened.

The only way a second element would be needed in the future is if jemalloc changes to require more than one pthread_key during its initialization, which would be detected by a new dead lock happening after a jemalloc upgrade. What would crashing add?

Comment 21

[Thinker Li [:sinker]]

I mean to add some code to check if we need more than one tls key before it is ready, and crash/or abort the process intently if it is happened.  So, next time, we will know it's root cause clearly.

Comment 22

[Jeff Walden [:Waldo]]

Comment on attachment 8549219 [details] [diff] [review]
Use a LinkedList to store TLS keys for Nuwa, and allocate the first element in a static buffer

Review of attachment 8549219 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/build/Nuwa.cpp
@@ +250,5 @@
> +bool TLSKey::sUsed = false;
> +AlignedStorage2<TLSKey> TLSKey::sFirstElement;
> +
> +template <typename T>
> +struct AutoCleanLinkedList : public LinkedList<T>

Yeah, this seems reasonable to add to LinkedList.h.  Needs a documentation comment, tho, something like:

"""
A LinkedList that will remove (and delete) each element still within itself on destruction.  Note that because each element is deleted, elements must have been allocated using |new|.
"""

@@ +256,5 @@
> +  ~AutoCleanLinkedList() {
> +    for (T* it = LinkedList<T>::getFirst(); it != nullptr;) {
> +      T* tmp = it;
> +      it = it->getNext();
> +      delete tmp;

So as I look at this, I'm a bit dubious about it.  You're not removing any elements from the linked list.  So when the base class destructor's called, won't this assert?  Maybe this is only being used, currently, in cases where it's always emptied before destruction?

The fix for this is, I think:

  while (T* element = this->popFirst()) {
    delete tmp;
  }

Comment 23

[Mike Hommey [:glandium]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #22)
> So as I look at this, I'm a bit dubious about it.  You're not removing any
> elements from the linked list.  So when the base class destructor's called,
> won't this assert?  Maybe this is only being used, currently, in cases where
> it's always emptied before destruction?

deleting a LinkedListElement always removes it from the list:

154   ~LinkedListElement()
155   {
156     if (!mIsSentinel && isInList()) {
157       remove();
158     }
159   }

Comment 24

[Mike Hommey [:glandium]]

Attachment: Add an AutoCleanLinkedList template that removes and deletes elements upon destruction

Turns out the popFirst version makes the compiler generate better code.

Comment 25

[Mike Hommey [:glandium]]

Attachment: Use a LinkedList to store TLS keys for Nuwa, and allocate the first element in a static buffer

Comment 26

[Thinker Li [:sinker]]

Comment on attachment 8558967 [details] [diff] [review]
Use a LinkedList to store TLS keys for Nuwa, and allocate the first element in a static buffer

Review of attachment 8558967 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/build/Nuwa.cpp
@@ +329,5 @@
>  /**
>   * This mutex protects the access to sTLSKeys, which keeps track of existing
>   * TLS Keys.
>   */
> +static pthread_mutex_t sTLSKeyLock = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;

Since _NP suffix means non-portable, we should avoid it.  Use PTHREAD_ERRORCHECK_MUTEX_INITIALIZER instead of '_NP' one.

@@ +755,5 @@
>  __wrap_pthread_key_create(pthread_key_t *key, void (*destructor)(void*)) {
>    int rv = REAL(pthread_key_create)(key, destructor);
>    if (rv != 0) {
>      return rv;
>    }

static pid_t lockingthread = 0;
if (lockingthread == gettid()) {
  // nested called, bug 1121269 is happened again!
  abort();
}

@@ +756,5 @@
>    int rv = REAL(pthread_key_create)(key, destructor);
>    if (rv != 0) {
>      return rv;
>    }
> +  MOZ_RELEASE_ASSERT(REAL(pthread_mutex_lock)(&sTLSKeyLock) == 0);

lockingthread = gettid();

@@ +757,5 @@
>    if (rv != 0) {
>      return rv;
>    }
> +  MOZ_RELEASE_ASSERT(REAL(pthread_mutex_lock)(&sTLSKeyLock) == 0);
> +  sTLSKeys.insertBack(new TLSKey(*key, destructor));

lockingthread = 0;

Comment 27

[Mike Hommey [:glandium]]

Comment on attachment 8558967 [details] [diff] [review]
Use a LinkedList to store TLS keys for Nuwa, and allocate the first element in a static buffer

(In reply to Thinker Li [:sinker] from comment #26)
> Comment on attachment 8558967 [details] [diff] [review]
> Use a LinkedList to store TLS keys for Nuwa, and allocate the first element
> in a static buffer
> 
> Review of attachment 8558967 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozglue/build/Nuwa.cpp
> @@ +329,5 @@
> >  /**
> >   * This mutex protects the access to sTLSKeys, which keeps track of existing
> >   * TLS Keys.
> >   */
> > +static pthread_mutex_t sTLSKeyLock = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
> 
> Since _NP suffix means non-portable, we should avoid it.  Use
> PTHREAD_ERRORCHECK_MUTEX_INITIALIZER instead of '_NP' one.

$ git grep PTHREAD_ERRORCHECK_MUTEX_INITIALIZER
benchmarks/pthread_benchmark.cpp:  pthread_mutex_t mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
libc/include/pthread.h:#define PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP {__PTHREAD_ERRORCHECK_MUTEX_INIT_VALUE __RESERVED_INITIALIZER}
libc/include/pthread.h:#define PTHREAD_ERRORCHECK_MUTEX_INITIALIZER PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP

There is no non-NP definition in bionic.

> @@ +755,5 @@
> >  __wrap_pthread_key_create(pthread_key_t *key, void (*destructor)(void*)) {
> >    int rv = REAL(pthread_key_create)(key, destructor);
> >    if (rv != 0) {
> >      return rv;
> >    }
> 
> static pid_t lockingthread = 0;
> if (lockingthread == gettid()) {
>   // nested called, bug 1121269 is happened again!
>   abort();
> }

That's what the MOZ_RELEASE_ASSERTs do (pthread_mutex_lock returns an error in that case).

Comment 28

[Thinker Li [:sinker]]

Comment on attachment 8558967 [details] [diff] [review]
Use a LinkedList to store TLS keys for Nuwa, and allocate the first element in a static buffer

Review of attachment 8558967 [details] [diff] [review]:
-----------------------------------------------------------------

Mike, thanks for your explanation.  I got your point about CHECKERROR.

Comment 29

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fcbd0b82ec6
https://hg.mozilla.org/integration/mozilla-inbound/rev/af3cf3f3a899
https://hg.mozilla.org/integration/mozilla-inbound/rev/30cc8c82eb21

Comment 30

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #27)
> $ git grep PTHREAD_ERRORCHECK_MUTEX_INITIALIZER
> benchmarks/pthread_benchmark.cpp:  pthread_mutex_t mutex =
> PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
> libc/include/pthread.h:#define PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP
> {__PTHREAD_ERRORCHECK_MUTEX_INIT_VALUE __RESERVED_INITIALIZER}
> libc/include/pthread.h:#define PTHREAD_ERRORCHECK_MUTEX_INITIALIZER
> PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP

sigh google... until commit 212e0e38248860b151b28877225629a988d95b58 in platform/bionic, it was PTHREAD_ERRORCHECK_MUTEX_INITIALIZER. I also missed that there actually is a PTHREAD_ERRORCHECK_MUTEX_INITIALIZER define, but under a "TODO: remove this namespace pollution".

Anyways, needed a fixup:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0351fbdeb37c

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2fcbd0b82ec6
https://hg.mozilla.org/mozilla-central/rev/af3cf3f3a899
https://hg.mozilla.org/mozilla-central/rev/30cc8c82eb21
https://hg.mozilla.org/mozilla-central/rev/0351fbdeb37c

Comment 32

[Cervantes Yu [:cyu] [:cervantes]]

[Blocking Requested - why for this release]: Bug 1119157, which is 2.1+, depends on this bug.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Try pushes:
b2g34: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ba4e622363e
b2g37: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cebcf60359a

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Both pushes look good (relative to the current state of those branches). Please nominate the patches here for b2g34 and b2g37 approval when you get a chance, Mike :)

Comment 35

[Mike Hommey [:glandium]]

AIUI, only https://hg.mozilla.org/try/rev/1c561ed8c1da is needed for b2g34 and b2g37, am I right?

Comment 36

[Cervantes Yu [:cyu] [:cervantes]]

(In reply to Mike Hommey [:glandium] from comment #35)
> AIUI, only https://hg.mozilla.org/try/rev/1c561ed8c1da is needed for b2g34
> and b2g37, am I right?

Yes, but it depends on the other 2 patches (in the context). You are suggesting uplifting this revision only?

Comment 37

[Mike Hommey [:glandium]]

(In reply to Cervantes Yu from comment #36)
> (In reply to Mike Hommey [:glandium] from comment #35)
> > AIUI, only https://hg.mozilla.org/try/rev/1c561ed8c1da is needed for b2g34
> > and b2g37, am I right?
> 
> Yes, but it depends on the other 2 patches (in the context).

AFAIR, besides the context mismatch, the patch is independent from the others in this bug.

> You are suggesting uplifting this revision only?

Essentially, yes.

Comment 38

[Cervantes Yu [:cyu] [:cervantes]]

Comment on attachment 8549220 [details] [diff] [review]
Remove TLS keys from bookkeeping in non-Nuwa processes too

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 771765
User impact if declined: bug 1119157 is dependent on this patch.
Testing completed: automatic tests on try.
Risk to taking this patch (and alternatives if risky): low. This patch doesn't contain observable behavior changes.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c7b7ab31ccb9
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/e00ae73d67a2
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/e00ae73d67a2