Free the tls memory on thread termination #95362

kunalspathak · 2023-11-28T22:10:12Z

The TLS memory that we used to store the TLS data per thread was never freed on thread termination. This results in memory leak.

src/coreclr/vm/threads.cpp

kunalspathak · 2023-11-29T22:14:29Z

@jkotas

AndyAyersMS

Changes look ok to me. I think @AaronRobinsonMSFT is also taking a look.

mangod9 · 2023-11-29T23:53:59Z

src/coreclr/vm/threads.cpp

@@ -7702,6 +7708,15 @@ void Thread::DeleteThreadStaticData()
    CONTRACTL_END;

    m_ThreadLocalBlock.FreeTable();
+
+    delete[] t_ThreadStatics.NonGCThreadStaticBlocks;


so these were thought to be implicitly deleted but that isnt the case?

carlossanlop · 2023-11-29T23:57:11Z

If we intend to backport this fix to 8.0 so it gets included in the January Release, please target the release/8.0 branch directly, not staging, so we can flow it to internal quickly. Use:

/backport to release/8.0

AndyAyersMS · 2023-11-30T01:38:19Z

@kunalspathak no symbolization on the stack traces so I can only guess what's behind the failures -- maybe the cleanup needs to be conditional on the TLS not being nullptr? Linux may be more tolerant of calling delete on nulls.

kunalspathak · 2023-11-30T01:49:07Z

maybe the cleanup needs to be conditional on the TLS not being nullptr

Here is what I found for 1 repro, but it doesn't always stay like that. Notice the JIT_GetSharedGCThreadStaticBaseOptimized at the end, so my first guess was it has to do with CONSISTENCY_CHECK(staticBlock != NULL);, but adding printf doesn't help as it is not consistent.

CORECLR! CHECK::Trigger + 0x1E6 (0x00007ffe`907a7356)
CORECLR! CLRVectoredExceptionHandlerPhase3 + 0x4B5 (0x00007ffe`900d7135)
CORECLR! CLRVectoredExceptionHandlerPhase2 + 0x91 (0x00007ffe`900d6a11)
CORECLR! CLRVectoredExceptionHandler + 0xDFD (0x00007ffe`900d696d)
CORECLR! CLRVectoredExceptionHandlerShim + 0x1B4 (0x00007ffe`900d73b4)
NTDLL! RtlGetLengthWithoutLastFullDosOrNtPathElement + 0x3DA (0x00007fff`c6e67a8a)
NTDLL! RtlFindCharInUnicodeString + 0x872 (0x00007fff`c6e0e242)
NTDLL! KiUserExceptionDispatcher + 0x2E (0x00007fff`c6e9340e)
CORECLR! JIT_GetSharedGCThreadStaticBaseOptimized + 0x4CE (0x00007ffe`903f8e9e)
<no module>! <no symbol> + 0x0 (0x00007ffe`375eb155)
    File: D:\git\runtime2\src\coreclr\vm\excep.cpp Line: 7177
    Image: E:\helix\95362_2\correlation-payload\test-runtime-net9.0-windows-Release-x64\dotnet.exe

Under windbg, I always hit this kind of AV, which I think is coming because staticBlock is nullptr? The AV in windbg is consistent but I don't understand why I don't see if windbg is not attached:

0007ffe`4a379c9e c5fe7f65e0      vmovdqu ymmword ptr [rbp-20h],ymm4
00007ffe`4a379ca3 48894d10        mov     qword ptr [rbp+10h],rcx
00007ffe`4a379ca7 488b4d10        mov     rcx,qword ptr [rbp+10h]
00007ffe`4a379cab 488b4908        mov     rcx,qword ptr [rcx+8]
00007ffe`4a379caf 48894df8        mov     qword ptr [rbp-8],rcx
00007ffe`4a379cb3 488b4df8        mov     rcx,qword ptr [rbp-8]
00007ffe`4a379cb7 48baf8597e4afe7f0000 mov rdx,7FFE4A7E59F8h
00007ffe`4a379cc1 e89a14435c      call    coreclr!JIT_ClassProfile32 (00007ffe`a67ab160)
00007ffe`4a379cc6 488b4df8        mov     rcx,qword ptr [rbp-8]
00007ffe`4a379cca 48894de8        mov     qword ptr [rbp-18h],rcx
00007ffe`4a379cce 488b4de8        mov     rcx,qword ptr [rbp-18h]
00007ffe`4a379cd2 488b45e8        mov     rax,qword ptr [rbp-18h]
00007ffe`4a379cd6 488b00          mov     rax,qword ptr [rax] ds:00000000`00000000=???????????????? ; <----
00007ffe`4a379cd9 488b4060        mov     rax,qword ptr [rax+60h]
00007ffe`4a379cdd ff5018          call    qword ptr [rax+18h]
00007ffe`4a379ce0 85c0            test    eax,eax

kunalspathak · 2023-11-30T02:07:05Z

I just pushed the conditional checking as well as reset the other two variables.

kunalspathak · 2023-11-30T02:11:31Z

I just pushed the conditional checking as well as reset the other two variables.

I verified locally and MSVC doesn't complain if using delete[] on nullptr.

kunalspathak · 2023-11-30T02:15:28Z

alright, verified the AV in jit code that I shared earlier and I can see it repros even on my local build from main, so perhaps it is from the test itself.

jkotas · 2023-11-30T02:22:42Z

Is the DeleteThreadStaticData method guaranteed to be called on the terminating thread? Can it be called on some other thread?

kunalspathak · 2023-11-30T02:52:31Z

Is the DeleteThreadStaticData method guaranteed to be called on the terminating thread? Can it be called on some other thread?

Looking at the comments about OnThreadTerminate, it seems to be the case:

runtime/src/coreclr/vm/threads.cpp

Lines 2353 to 2359 in 8cb3c61

    
           // One way OnThreadTerminate() is called is when the thread finishes doing useful 
        
           // work.  This case always happens on the correct thread. 
        
           // 
        
           // The other way OnThreadTerminate()  is called is during product shutdown.  We do 
        
           // a "best effort" to eliminate all threads except the Main thread before shutdown 
        
           // happens.  But there may be some background threads or external threads still 
        
           // running.

)

One of the caller DestroyThread of OnThreadTerminate has this assert:

runtime/src/coreclr/vm/threads.cpp

Line 894 in 8cb3c61

_ASSERTE (th == GetThread());

but the fact that inside OnThreadTerminate() has this code just few lines after DeleteThreadStaticData() makes me wonder if the terminating thread is the one that calls DeleteThreadStaticData:

runtime/src/coreclr/vm/threads.cpp

Lines 2962 to 2964 in 8cb3c61

    
           // Guaranteed to NOT be a shutdown case, because we tear down the heap before 
        
           // we tear down any threads during shutdown. 
        
           if (ThisThreadID == CurrentThreadID)

I don't want to disturb the CI, but once it is done, I can try adding this check inside DeleteThreadStaticData

JulieLeeMSFT · 2023-11-30T03:06:03Z

@mangod9 for his input:

but the fact that inside OnThreadTerminate() has this code just few lines after DeleteThreadStaticData() makes me wonder if the terminating thread is the one that calls DeleteThreadStaticData:

kunalspathak · 2023-11-30T03:08:19Z

but the fact that inside OnThreadTerminate() has this code just few lines after DeleteThreadStaticData() makes me wonder if the terminating thread is the one that calls DeleteThreadStaticData:

alright, I added an assert locally and I hit that assert.

     m_ThreadLocalBlock.FreeTable();

+    Thread *pCurrentThread = GetThreadNULLOk();
+    DWORD CurrentThreadID = pCurrentThread?pCurrentThread->GetThreadId():0;
+    DWORD ThisThreadID = GetThreadId();
+
+    _ASSERTE(CurrentThreadID == ThisThreadID);

Assert failure(PID 84504 [0x00014a18], Thread: 99544 [0x184d8]): CurrentThreadID == ThisThreadID

CORECLR! Thread::DeleteThreadStaticData + 0x13F (0x00007ffe`ad4f7b6f)
CORECLR! Thread::OnThreadTerminate + 0x22A (0x00007ffe`ad500f6a)
CORECLR! Thread::CleanupDetachedThreads + 0x219 (0x00007ffe`ad4f5429)
CORECLR! Thread::DoExtraWorkForFinalizer + 0x1A7 (0x00007ffe`ad4f9227)
CORECLR! FinalizerThread::FinalizerThreadWorker + 0x8C2 (0x00007ffe`ad610952)
CORECLR! ManagedThreadBase_DispatchInner + 0xB4 (0x00007ffe`ad4ffd84)
CORECLR! ManagedThreadBase_DispatchMiddle + 0xB6 (0x00007ffe`ad4ffe86)
CORECLR! ``ManagedThreadBase_DispatchOuter'::`11'::__Body::Run'::`5'::__Body::Run + 0x7B (0x00007ffe`ad50265b)
CORECLR! `ManagedThreadBase_DispatchOuter'::`11'::__Body::Run + 0x93 (0x00007ffe`ad502953)
CORECLR! ManagedThreadBase_DispatchOuter + 0xDD (0x00007ffe`ad4fff9d)
    File: D:\git\runtime2\src\coreclr\vm\threads.cpp Line: 7716
    Image: E:\helix\95362_2\correlation-payload\test-runtime-net9.0-windows-Release-x64\dotnet.exe

So given that I can guard the deletion only if we are on current thread, where can we delete the memory for detached threads? Is there code that detached threads execute before termination?

kunalspathak · 2023-11-30T03:30:53Z

Is there code that detached threads execute before termination?

Seems DetachThread could be the right method.

runtime/src/coreclr/vm/threads.cpp

Line 983 in 8cb3c61

_ASSERTE (this == GetThread());

JulieLeeMSFT · 2023-11-30T03:47:17Z

@kouvel, please see Kunal's question. We need to backport this PR tonight.

kunalspathak · 2023-11-30T04:31:32Z

/backport to release/8.0

github-actions · 2023-11-30T04:31:46Z

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/7042036275

AaronRobinsonMSFT · 2023-11-30T04:38:02Z

src/coreclr/vm/ceemain.cpp

+    if (t_ThreadStatics.NonGCThreadStaticBlocks != nullptr)
+    {
+        delete[] t_ThreadStatics.NonGCThreadStaticBlocks;
+        t_ThreadStatics.NonGCThreadStaticBlocks = nullptr;
+    }
+    if (t_ThreadStatics.GCThreadStaticBlocks != nullptr)
+    {
+        delete[] t_ThreadStatics.GCThreadStaticBlocks;
+        t_ThreadStatics.GCThreadStaticBlocks = nullptr;
+    }


Nit: None of these null checks are needed.

Free the tls memory

3502147

ghost assigned kunalspathak Nov 28, 2023

dotnet-issue-labeler bot added the area-VM-coreclr label Nov 28, 2023

kunalspathak changed the title ~~Free the tls memory~~ Free the tls memory on thread termination Nov 28, 2023

kunalspathak added 4 commits November 28, 2023 14:22

zero the size variables

adbad79

fix linux build errors

9db0d85

Move ThreadStaticBlockInfo definition in jitinterface.h

18fd697

cleanup and consolidate TLS variables

42ccce9

build-analysis bot mentioned this pull request Nov 29, 2023

Test_EventSource_EtwManifestGeneration* tests failing in CI #48798

Closed

AaronRobinsonMSFT reviewed Nov 29, 2023

View reviewed changes

src/coreclr/vm/threads.cpp Outdated Show resolved Hide resolved

kunalspathak added 2 commits November 29, 2023 14:07

Address review feedback

6d008d3

Merge remote-tracking branch 'origin/main' into free-tls

9dcb923

kunalspathak marked this pull request as ready for review November 29, 2023 22:14

AaronRobinsonMSFT approved these changes Nov 29, 2023

View reviewed changes

AndyAyersMS approved these changes Nov 29, 2023

View reviewed changes

kunalspathak requested a review from mangod9 November 29, 2023 23:52

mangod9 reviewed Nov 29, 2023

View reviewed changes

mangod9 approved these changes Nov 29, 2023

View reviewed changes

reset t_NonGCThreadStaticBlocksSize and conditional check the nullptr

787145a

kunalspathak added 2 commits November 29, 2023 20:11

Delete tls memory from detach thread

c3e5823

Call DeleteThreadLocalMemory() from TlsDestructionMonitor

80cf17f

jkotas approved these changes Nov 30, 2023

View reviewed changes

github-actions bot mentioned this pull request Nov 30, 2023

[release/8.0] Free the tls memory on thread termination #95439

Merged

AaronRobinsonMSFT reviewed Nov 30, 2023

View reviewed changes

AaronRobinsonMSFT approved these changes Nov 30, 2023

View reviewed changes

kunalspathak merged commit 60be20c into dotnet:main Nov 30, 2023
111 checks passed

kunalspathak deleted the free-tls branch November 30, 2023 06:27

This was referenced Nov 30, 2023

Workingset constantly growing, possible unmanaged memory leak #92490

Open

Clean up the thread local memory regardless of managed thread's presence #95715

Merged

kunalspathak mentioned this pull request Dec 7, 2023

[release/8.0-staging] Clean up the thread local memory regardless of managed thread's presence #95722

Merged

mangod9 mentioned this pull request Dec 12, 2023

Large unmanaged memory growth (leak?) when upgrading from .NET 6 to 8 #95922

Closed

github-actions bot locked and limited conversation to collaborators Dec 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Free the tls memory on thread termination #95362

Free the tls memory on thread termination #95362

kunalspathak commented Nov 28, 2023

kunalspathak commented Nov 29, 2023

AndyAyersMS left a comment

mangod9 Nov 29, 2023

kunalspathak Nov 30, 2023

carlossanlop commented Nov 29, 2023

AndyAyersMS commented Nov 30, 2023

kunalspathak commented Nov 30, 2023

kunalspathak commented Nov 30, 2023

kunalspathak commented Nov 30, 2023

kunalspathak commented Nov 30, 2023

jkotas commented Nov 30, 2023

kunalspathak commented Nov 30, 2023

JulieLeeMSFT commented Nov 30, 2023

kunalspathak commented Nov 30, 2023 •

edited

Loading

kunalspathak commented Nov 30, 2023

JulieLeeMSFT commented Nov 30, 2023 •

edited

Loading

kunalspathak commented Nov 30, 2023

github-actions bot commented Nov 30, 2023

AaronRobinsonMSFT Nov 30, 2023

Free the tls memory on thread termination #95362

Free the tls memory on thread termination #95362

Conversation

kunalspathak commented Nov 28, 2023

kunalspathak commented Nov 29, 2023

AndyAyersMS left a comment

Choose a reason for hiding this comment

mangod9 Nov 29, 2023

Choose a reason for hiding this comment

kunalspathak Nov 30, 2023

Choose a reason for hiding this comment

carlossanlop commented Nov 29, 2023

AndyAyersMS commented Nov 30, 2023

kunalspathak commented Nov 30, 2023

kunalspathak commented Nov 30, 2023

kunalspathak commented Nov 30, 2023

kunalspathak commented Nov 30, 2023

jkotas commented Nov 30, 2023

kunalspathak commented Nov 30, 2023

JulieLeeMSFT commented Nov 30, 2023

kunalspathak commented Nov 30, 2023 • edited Loading

kunalspathak commented Nov 30, 2023

JulieLeeMSFT commented Nov 30, 2023 • edited Loading

kunalspathak commented Nov 30, 2023

github-actions bot commented Nov 30, 2023

AaronRobinsonMSFT Nov 30, 2023

Choose a reason for hiding this comment

kunalspathak commented Nov 30, 2023 •

edited

Loading

JulieLeeMSFT commented Nov 30, 2023 •

edited

Loading