546407 - MMgc deadlock with MMGC_MEMORY_PROFILER enabled and OOM condition

Status: VERIFIED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P2)

Description

[Rob Winchell]

If the memory profiler is enabled, a deadlock is possible if multiple player instances are running and one of the causes an OOM condition.

Example call stacks (2 threads):

VMPI_lockAcquire(vmpi_spin_lock_t * lock=0x06ef06a0)  
MMgc::GCAcquireSpinlockWithRecursion::GCAcquireSpinlockWithRecursion(vmpi_spin_lock_t * sl=0x06ef06a0, void * owner=0x0000164c)  
MMgc::GCHeap::AllocHook(const void * item=0x134eb730, unsigned int askSize=101, unsigned int gotSize=104)
MMgc::FixedAlloc::Alloc(unsigned int size=101, MMgc::FixedMallocOpts opts=kNone)  
MMgc::FixedAllocSafe::Alloc(unsigned int size=101, MMgc::FixedMallocOpts flags=kNone)
MMgc::FixedMalloc::Alloc(unsigned int size=101, MMgc::FixedMallocOpts flags=kNone)  
MMgc::FixedMalloc::OutOfLineAlloc(unsigned int size=101, MMgc::FixedMallocOpts flags=kNone
MMgc::AllocCallInline(unsigned int size=101, MMgc::FixedMallocOpts opts=kNone)  
                Flash10f.ocx!MMgc::TaggedAlloc(unsigned int size=101, MMgc::FixedMallocOpts opts=kNone, unsigned int guard=3216965634
                Flash10f.ocx!MMgc::NewTaggedArray(unsigned int count=93, unsigned int elsize=1, MMgc::FixedMallocOpts opts=kNone, bool isPrimitive=true)  
MMgcConstructTaggedArray<char>(char * __formal=0x00000000, unsigned int count=93, MMgc::FixedMallocOpts opts=kNone
                StrAlloc(unsigned int len=93
                CopyUTF16to8(const unsigned short * pSrc=0x0c70c62c, bool reverseBytes=false)      



VMPI_lockAcquire(vmpi_spin_lock_t * lock=0x06eefe78
MMgc::GCAcquireSpinlock::GCAcquireSpinlock(vmpi_spin_lock_t * spinlock=0x06eefe78)  
MMgc::FixedAllocSafe::QueryOwnsObject(const void * item=0x0a953718)
MMgc::FixedMalloc::EnsureFixedMallocMemory(const void * item=0x0a953718)  
MMgc::FixedMalloc::Free(void * item=0x0a953718) 
MMgc::FixedMalloc::OutOfLineFree(void * p=0x0a953718)  
MMgc::DeleteCallInline(void * p=0x0a953718)  
MMgc::DeleteTaggedArrayWithHeader(void * p=0x0a953718)  
MMgcDestructTaggedArrayChecked<HashTable::Bucket *>()  
HashTable::BucketArrayFree(HashTable::Bucket * * buckets=0x0a953728)  
HashTable::Destroy()  
HashTable::~HashTable()  
avmplus::ListenerHashTable::~ListenerHashTable()  
avmplus::ListenerHashTable::`scalar deleting destructor'() 
MMgcDestructTaggedScalarChecked<avmplus::ListenerHashTable(avmplus::ListenerHashTable * mem=0x09e14ae0)  
avmplus::ListenerNodeList::~ListenerNodeList()  
avmplus::ListenerNodeList::`scalar deleting destructor'()  
MMgc::GCAlloc::Finalize()  
MMgc::GC::Finalize()  
MMgc::GC::Sweep()  
MMgc::GC::FinishIncrementalMark(bool scanStack=false)  
MMgc::GC::Collect(bool scanStack=false
MMgc::GC::memoryStatusChange(MMgc::_MemoryStatus _formal=kFreeMemoryIfPossible, MMgc::_MemoryStatus to=kFreeMemoryIfPossible)  
MMgc::GCHeap::SendFreeMemorySignal(unsigned int minimumBlocksToFree=82945)  
MMgc::GCHeap::Alloc(unsigned int size=82945, unsigned int flags=9)  Locked LOCK2
MMgc::GC::heapAlloc(unsigned int siz=82945, int flags=9)  
MMgc::GC::AllocBlock(int size=82945, int pageType=3, bool zero=false, bool canFail=true)  
MMgc::GCLargeAlloc::Alloc(unsigned int originalSize=339738623, unsigned int requestSize=339738640, int flags=16)  L
MMgc::GC::Alloc(unsigned int size=339738640, int flags=16)  
MMgc::GC::PleaseAlloc(unsigned int size=339738623, int flags=0)

Comment 1

[Tommy Reilly]

My first instinct is to have the profiler use its own lock instead of the GCHeap lock.   There needs to be an invariant that the GCHeap lock is the only lock that can be held when we call out into other code (since it recurses properly).  FixedAllocSafe locks and the largeObjectTracker lock for instance either don't call out while the lock is held (largeObjectTracker) or release it before calling out (FixedAllocSafe::CreateChunk). 

Another fix would be to have FixedAllocSafe unlock before calling AllocHook.  Actually both "fixes" are probably good ideas.

Comment 2

[Tommy Reilly]

Having the profiler use its own lock will prevent us locking the heap lock on every alloc/free which could cause contention.

Comment 3

[Tommy Reilly]

Attachment: don't hold FixedAlloc locks while invoking hooks

Comment 4

[Tommy Reilly]

Attachment: removes AllocHook GCHeap locking

attempts to avoid all locking in the common case, including double checked bool in VMPI_spyCallback.

Comment 5

[Lars T Hansen]

Comment on attachment 427420 [details] [diff] [review]
don't hold FixedAlloc locks while invoking hooks

The code now performs more work than before: it recomputes the gcheap value, where previously it only had to pick it out of a property in the allocator.

I'm also concerned about code size increases from inlining all the hooking code and would like to see an analysis of that.

Comment 6

[Lars T Hansen]

Comment on attachment 427430 [details] [diff] [review]
removes AllocHook GCHeap locking

I'm hardly qualified to review the first change to the winmo platform code, so I'll have to take your word for it.  The rest is all good.

Comment 7

[Tommy Reilly]

(In reply to comment #5)
> (From update of attachment 427420 [details] [diff] [review])
> I'm also concerned about code size increases from inlining all the hooking code
> and would like to see an analysis of that.

44k increase in avmplus.a.  Oops.  If I flip things around and make Alloc/Free out of line functions with the AllocSansHook and Hooks inlined into it, we actually get 12k smaller as the FixedAllocSafe lock acquisition code is no longer inlined into FixedMalloc::Alloc call sites.   Nice catch.  Will post a new patch after investigating the extra work comment.

Comment 8

[Tommy Reilly]

tamarin-redux-argo: changeset:   3723:fcaffd5694c9
tamarin-redux: changeset:   3856:fcaffd5694c9

Leaving bug open to land the FixedAllocSafe lock fixing.

Comment 9

[Tommy Reilly]

Attachment: redo fixed alloc lock changes

Made FixedAllocSafe::Alloc/Free completely inlined (into its one caller, FixedMalloc::Alloc)

Left FixedAlloc::Alloc/Free out of line (this is called by some FixedAlloc instances in the player)

Made some new/delete overrides using FixedMalloc::Alloc/Free use the out of line versions (I think we just missed these).

Code size is slightly smaller (12k smaller ocx release and release debugger)

Also checked that the FixedBlock* calculation is only done once by the compiler once all the inlining is done.

Comment 10

[Lars T Hansen]

Comment on attachment 427772 [details] [diff] [review]
redo fixed alloc lock changes

The call to Alloc and not OutOfLineAlloc in PleaseAlloc was deliberate - we want PleaseAlloc to inline as well as Alloc does.

The GCRoot new and delete operators also called FixedMalloc::Alloc/Delete (and not the OutOfLine versions) deliberately because 'size' is pretty much always known and the inlining should boil most of the code away and we should be left with a call directly to FixedAllocSafe::Alloc or FixedLargeAlloc::Alloc.

In both those cases we could probably go either way; they're not heavily used either statically or dynamically.  But it wasn't an oversight.

The restructuring as large REALLY_INLINE functions is OK but those inline functions should not be in the header file; they're used only in the cpp file so put them there.  Ditto the hook functions, then.

Comment 11

[Tommy Reilly]

I realize that, but I changed FixedAllocSafe to inline so now FixedMalloc::Alloc isn't a small function to inline.   Anyways things get smaller w/ my change because we aren't inlining a size check, branch and a VMPI_lock/release into FixedMalloc::Alloc call sites which was dubious I think.  Now only the uncommon large allocs and times where CreateChunk/FreeChunk are needed require two call frames.   Anyway I measured call times and its a wash in release and a little faster in debugger and its smaller, win win!

So I don't need to put inline functions in the inlines, I'll move to the cpp and repost.

Comment 12

[Tommy Reilly]

(In reply to comment #10)
> The restructuring as large REALLY_INLINE functions is OK but those inline
> functions should not be in the header file; they're used only in the cpp file
> so put them there.  Ditto the hook functions, then.

Actually FixedAllocSafe::Alloc/Free are used externally and since they use the the hook funcs and the SansHook functions I think everything needs to go in the inlines header.  At least that's what gcc is telling me.

Comment 13

[Tommy Reilly]

(In reply to comment #10)
> (From update of attachment 427772 [details] [diff] [review])
> The call to Alloc and not OutOfLineAlloc in PleaseAlloc was deliberate - we
> want PleaseAlloc to inline as well as Alloc does.

Alloc is only used once in FixedMalloc::Alloc, PleaseAlloc (and all the new operators) are used a lot statically and probably shouldn't be inlined.   My thinking is that statically numerous call sites should call out of line functions that are themselves maximally inlined.   I think this patch moves us in that direction although I think it would be better if the out of line functions were the default and the inline version were special/private (ie only used by MMgc).

Re-requesting review.

Comment 14

[Tommy Reilly]

First patch:
tamarin-redux-argo: changeset:   3724:d634a1cff7ae, changeset:   3723:fcaffd5694c9

tamarin-redux: changeset:   3858:d634a1cff7ae, changeset:   3857:1c6bce438ca5

Comment 15

[Lars T Hansen]

Did you just push those without awaiting review?

Comment 16

[Tommy Reilly]

no, the first patch was pushed, attachment 427430 [details] [diff] [review]

Comment 17

[Tommy Reilly]

Second patch isn't critical, can wait till Lar's is back.

Comment 18

[Lars T Hansen]

(In reply to comment #12)
> (In reply to comment #10)
> > The restructuring as large REALLY_INLINE functions is OK but those inline
> > functions should not be in the header file; they're used only in the cpp file
> > so put them there.  Ditto the hook functions, then.
> 
> Actually FixedAllocSafe::Alloc/Free are used externally and since they use the
> the hook funcs and the SansHook functions I think everything needs to go in the
> inlines header.  At least that's what gcc is telling me.

Then that's a symptom of a breakdown in the design somewhere.  SR- again, and let's take it to IM or telephone.

Comment 19

[Tommy Reilly]

holding the lock across the AllocHook/FreeHook makes me nervous but is not known to be a problem moving to 10.1.1.

Comment 20

[Dan Smith]

Moving back to 10.1 after discussion with Tom.

Comment 22

[Tommy Reilly]

Attachment: makes FixedAllocSafe private to tighten things up

Comment 23

[Lars T Hansen]

Comment on attachment 433395 [details] [diff] [review]
makes FixedAllocSafe private to tighten things up

Technically speaking the change to GCRoot::new is not right because the size is always going to be known at compile time, and most of the inlined code will boil away to a call to the correct underlying operator.  But this is not a big deal and it's fine the way you've changed it.

More worrisome, don't the updates to m_totalAskSize in AllocHook and FreeHook, when called from FixedAllocSafe, need to be protected by a mutex?

Stylistically it would probably be better to rename AllocHook, AllocSansHook, etc as InlineAllocHook, InlineAllocSansHook, etc, to make the purpose clear and throw up warnings, but I'm not going to insist (at least not now).

Holding the review until the issue of the locking has been resolved.

Comment 24

[Tommy Reilly]

Attachment: Makes m_totalAskSize updates happen in locked Alloc/FreeSansHook and updates names to include "Inline" prefix

Kinda messy, wonder if there's a way to use templates to clean up the compile time ifdef's.

Comment 25

[Lars T Hansen]

(In reply to comment #24)
> Created an attachment (id=433555) [details]
> Kinda messy, wonder if there's a way to use templates to clean up the compile
> time ifdef's.

I sometimes use this pattern:

  #ifdef BLAH
  #define MAYBEARG(x) , x
  #else
  #define MAYBEARG(x)
  #endif

and then

  int foo(int x MAYBEARG(int y)) { ... }

and then

  foo(10 MAYBEARG(20))

which at least stops my eyes from bleeding and with a good name for MAYBEARG is pretty easy to understand when reading.

Comment 26

[Lars T Hansen]

eg, MEMPROFARG might be an adequate name in this circumstance.

Comment 27

[Lars T Hansen]

Comment on attachment 433555 [details] [diff] [review]
Makes m_totalAskSize updates happen in locked Alloc/FreeSansHook and updates names to include "Inline" prefix

It's a bit absurd for AllocSansHook to have an #ifdef MMGC_HOOKS in it, but let's get on with it.

Comment 28

[Tommy Reilly]

tamarin-redux-argo: changeset:   3842:cfcaebb5c0c8
tamarin-redux: changeset:   4101:cfcaebb5c0c8

Comment 29

[Dan Schaffer]

how can this bug be regression testing for marking verified?  Would it be enough to run several instance of the player with memory profiling enabled?

Comment 30

[Lars T Hansen]

(In reply to comment #29)
> how can this bug be regression testing for marking verified?  Would it be
> enough to run several instance of the player with memory profiling enabled?

At least that addresses the original report, though I can't speak to whether that would be enough to provoke the original bug, which is what you'd need to verify in such a test case, of course.

Comment 31

[Dan Schaffer]

ran the oom testsuite with plugin and memory profiling enabled with build 10,1,53,57.  tests worked correctly.  when oom condition was reached a refresh reloaded the player correctly. oom testsuite is here:  http://ats.macromedia.com/Players/ATS/OTS/OTS10/OOM/asteam/alltests.html

marking bug as verified.