From ae3a575f32f272d02d9597d9909cc37bc31da505 Mon Sep 17 00:00:00 2001 From: Konrad Kokosa Date: Thu, 16 Jul 2026 17:25:07 +0200 Subject: [PATCH 1/6] Fix GC.GetTotalMemory returning negative under regions GC GCHeap::ApproxTotalBytesInUse computes gen0's live size as gen0_size - gen0_frag using unsigned arithmetic. gen0_frag (free_list_space + free_obj_space) is a per-generation total spanning every gen0 region, but gen0_size only summed region spans up to the ephemeral region, dropping any gen0 region linked after it (for example a pinned region swept in place). When the dropped regions' fragmentation exceeded their omitted span, gen0_frag exceeded gen0_size and the unsigned subtraction underflowed, surfacing as a negative value from GC.GetTotalMemory. Walk every gen0 region so the counted span matches the fragmentation total. Also reorder the free-list discard bookkeeping in a_fit_free_list_p so a lock-free reader observes a harmless under-count instead of a transient over-count. Segments are unaffected: the single contiguous ephemeral segment already bounds all gen0 fragmentation within the counted span. Adds a regression test that drives concurrent allocation with a large, continuously-refreshed pinned-object ring while probing GC.GetTotalMemory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 975e6875-e7d4-4172-8d1f-5ed514017a4d --- src/coreclr/gc/allocation.cpp | 7 +- src/coreclr/gc/interface.cpp | 15 ++- .../GC/API/GC/GetTotalMemoryConcurrent.cs | 125 ++++++++++++++++++ .../GC/API/GC/GetTotalMemoryConcurrent.csproj | 14 ++ 4 files changed, 154 insertions(+), 7 deletions(-) create mode 100644 src/tests/GC/API/GC/GetTotalMemoryConcurrent.cs create mode 100644 src/tests/GC/API/GC/GetTotalMemoryConcurrent.csproj diff --git a/src/coreclr/gc/allocation.cpp b/src/coreclr/gc/allocation.cpp index 06d5d231a9d0b6..897586d99346f4 100644 --- a/src/coreclr/gc/allocation.cpp +++ b/src/coreclr/gc/allocation.cpp @@ -2371,10 +2371,15 @@ BOOL gc_heap::a_fit_free_list_p (int gen_number, { assert (prev_free_item == 0); dprintf (3, ("couldn't use this free area, discarding")); - generation_free_obj_space (gen) += free_list_size; + // Update ordering for lock-free readers (e.g. GCHeap::ApproxTotalBytesInUse): + // remove the bytes from free_list_space before adding them to free_obj_space so + // the transient a concurrent reader can observe is a harmless under-count rather + // than an over-count that would make gen0 fragmentation (free_list_space + + // free_obj_space) appear larger than the gen0 span and underflow (issue #106712). gen_allocator->unlink_item (a_l_idx, free_list, prev_free_item, FALSE); generation_free_list_space (gen) -= free_list_size; + generation_free_obj_space (gen) += free_list_size; assert ((ptrdiff_t)generation_free_list_space (gen) >= 0); } else diff --git a/src/coreclr/gc/interface.cpp b/src/coreclr/gc/interface.cpp index 00d5f77f1a5e3f..0a260a50638507 100644 --- a/src/coreclr/gc/interface.cpp +++ b/src/coreclr/gc/interface.cpp @@ -2073,9 +2073,16 @@ size_t GCHeap::ApproxTotalBytesInUse(BOOL small_heap_only) generation* gen = pGenGCHeap->generation_of (0); size_t gen0_frag = generation_free_list_space (gen) + generation_free_obj_space (gen); uint8_t* current_alloc_allocated = pGenGCHeap->alloc_allocated; - heap_segment* current_eph_seg = pGenGCHeap->ephemeral_heap_segment; size_t gen0_size = 0; #ifdef USE_REGIONS + // Walk every gen0 region. free_list_space/free_obj_space (gen0_frag) are per-generation + // totals accumulated across all gen0 regions, so gen0_size must likewise span all of them. + // The ephemeral region is the one that holds alloc_allocated and is capped there; any other + // gen0 region (which can be linked before or after the ephemeral one, e.g. a pinned region + // that survived in place) is counted up to its heap_segment_allocated. Stopping at the + // ephemeral region here used to drop those regions' span while still subtracting their + // fragmentation, making gen0_frag exceed gen0_size and underflowing the subtraction below + // (issue #106712). heap_segment* gen0_seg = generation_start_segment (gen); while (gen0_seg) { @@ -2083,15 +2090,11 @@ size_t GCHeap::ApproxTotalBytesInUse(BOOL small_heap_only) current_alloc_allocated : heap_segment_allocated (gen0_seg); gen0_size += end - heap_segment_mem (gen0_seg); - if (gen0_seg == current_eph_seg) - { - break; - } - gen0_seg = heap_segment_next (gen0_seg); } #else //USE_REGIONS // For segments ephemeral seg does not change. + heap_segment* current_eph_seg = pGenGCHeap->ephemeral_heap_segment; gen0_size = current_alloc_allocated - heap_segment_mem (current_eph_seg); #endif //USE_REGIONS diff --git a/src/tests/GC/API/GC/GetTotalMemoryConcurrent.cs b/src/tests/GC/API/GC/GetTotalMemoryConcurrent.cs new file mode 100644 index 00000000000000..de657c211a9578 --- /dev/null +++ b/src/tests/GC/API/GC/GetTotalMemoryConcurrent.cs @@ -0,0 +1,125 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// GC.GetTotalMemory(false) must never return a negative value. +// +// The value bottoms out in ApproxTotalBytesInUse, which for gen0 computes gen0_size - gen0_frag +// as an unsigned quantity. gen0 fragmentation (free-list + free-object space) is a per-generation +// total that spans every gen0 region, so gen0_size must span every gen0 region as well. When gen0 +// retains a region past the ephemeral one (for example a region held in place by a pinned object), +// its span has to be included; otherwise fragmentation exceeds the counted span, the subtraction +// underflows, and the managed API surfaces the wrapped value as a negative long. +// +// This test drives concurrent allocation with a large, continuously-refreshed set of pinned +// objects (to force such retained gen0 regions) while repeatedly probing GetTotalMemory on another +// thread. Without the accounting fix it returns a negative value in well under a second; with the +// fix the value stays non-negative. + +using System; +using System.Diagnostics; +using System.Runtime.InteropServices; +using System.Threading; +using Xunit; + +public class GetTotalMemoryConcurrent +{ + private static volatile bool s_stop; + + [Fact] + public static void TestEntryPoint() + { + // A few seconds is far more than enough: the unfixed runtime fails almost immediately. + // Even a single allocating thread is sufficient to build the region layout that triggers + // the miscount, so this does not depend on the number of processors. + const int DurationSeconds = 8; + int workers = Math.Max(2, Math.Min(8, Environment.ProcessorCount)); + + var threads = new Thread[workers]; + for (int i = 0; i < workers; i++) + { + threads[i] = new Thread(PinChurn) { IsBackground = true }; + threads[i].Start(); + } + + long probes = 0; + long minObserved = long.MaxValue; + long negative = 0; + var sw = Stopwatch.StartNew(); + try + { + while (sw.Elapsed.TotalSeconds < DurationSeconds) + { + long total = GC.GetTotalMemory(false); + probes++; + if (total < minObserved) + { + minObserved = total; + } + + if (total < 0) + { + negative = total; + break; + } + } + } + finally + { + s_stop = true; + foreach (Thread t in threads) + { + t.Join(TimeSpan.FromSeconds(5)); + } + } + + Console.WriteLine($"probes={probes}, min observed={minObserved}, negative={negative}"); + Assert.True(negative >= 0, $"GC.GetTotalMemory(false) returned a negative value: {negative}"); + } + + // Keeps a large ring of pinned tiny objects alive, refreshing the oldest one each iteration, and + // floods gen0 with throwaway garbage in between. The pins prevent their gen0 regions from being + // compacted, so those regions are swept and retained with large free lists while the live set + // stays small - exactly the shape that makes gen0 fragmentation exceed the counted gen0 span. + private static void PinChurn() + { + var rng = new Random(Environment.CurrentManagedThreadId); + const int RingSize = 4096; + var ring = new GCHandle[RingSize]; + int slot = 0; + long sink = 0; + + try + { + while (!s_stop) + { + if (ring[slot].IsAllocated) + { + ring[slot].Free(); + } + ring[slot] = GCHandle.Alloc(new byte[24], GCHandleType.Pinned); + slot = (slot + 1) % RingSize; + + for (int j = 0; j < 96; j++) + { + byte[] junk = new byte[rng.Next(8, 256)]; + sink += junk.Length; + } + } + } + finally + { + for (int i = 0; i < RingSize; i++) + { + if (ring[i].IsAllocated) + { + ring[i].Free(); + } + } + } + + if (sink == long.MaxValue) + { + Console.WriteLine(sink); + } + } +} diff --git a/src/tests/GC/API/GC/GetTotalMemoryConcurrent.csproj b/src/tests/GC/API/GC/GetTotalMemoryConcurrent.csproj new file mode 100644 index 00000000000000..f0115d9f2dd851 --- /dev/null +++ b/src/tests/GC/API/GC/GetTotalMemoryConcurrent.csproj @@ -0,0 +1,14 @@ + + + + true + 1 + + + PdbOnly + true + + + + + From 7ce70ab2ec7747a620650c64afd650a6cbfdd348 Mon Sep 17 00:00:00 2001 From: Konrad Kokosa Date: Fri, 17 Jul 2026 11:47:21 +0200 Subject: [PATCH 2/6] Small test improvements --- src/tests/GC/API/GC/GetTotalMemoryConcurrent.cs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/tests/GC/API/GC/GetTotalMemoryConcurrent.cs b/src/tests/GC/API/GC/GetTotalMemoryConcurrent.cs index de657c211a9578..5da21d8f25c7e7 100644 --- a/src/tests/GC/API/GC/GetTotalMemoryConcurrent.cs +++ b/src/tests/GC/API/GC/GetTotalMemoryConcurrent.cs @@ -28,12 +28,17 @@ public class GetTotalMemoryConcurrent [Fact] public static void TestEntryPoint() { - // A few seconds is far more than enough: the unfixed runtime fails almost immediately. - // Even a single allocating thread is sufficient to build the region layout that triggers - // the miscount, so this does not depend on the number of processors. - const int DurationSeconds = 8; + // A couple of seconds is far more than enough: the unfixed runtime fails almost immediately + // (typically within the first ~100K probes, well under a second). Even a single allocating + // thread is sufficient to build the region layout that triggers the miscount, so this does + // not depend on the number of processors. + const int DurationSeconds = 2; int workers = Math.Max(2, Math.Min(8, Environment.ProcessorCount)); + // Reset in case this ever runs more than once in the same process: the worker threads exit + // as soon as s_stop is set, so a stale 'true' would leave the heap idle and mask the bug. + s_stop = false; + var threads = new Thread[workers]; for (int i = 0; i < workers; i++) { From a22ce85aa9edd8bcf0e8685f8687f391161b8a66 Mon Sep 17 00:00:00 2001 From: Konrad Kokosa Date: Mon, 20 Jul 2026 12:40:08 +0200 Subject: [PATCH 3/6] Address review: drop allocation.cpp reordering and trim interface.cpp comment Per reviewer feedback (jkotas, janvorli, mangod9): the discard-path counter reordering in allocation.cpp only reduces the frequency of the transient without proper memory barriers, and the accounting is best-effort by design, so revert it to the original ordering. Also remove the verbose gen0-region comment in interface.cpp. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2da4f5bf-503e-4a71-a1df-f73e8aedbfe2 --- src/coreclr/gc/allocation.cpp | 7 +------ src/coreclr/gc/interface.cpp | 8 -------- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/src/coreclr/gc/allocation.cpp b/src/coreclr/gc/allocation.cpp index 897586d99346f4..06d5d231a9d0b6 100644 --- a/src/coreclr/gc/allocation.cpp +++ b/src/coreclr/gc/allocation.cpp @@ -2371,15 +2371,10 @@ BOOL gc_heap::a_fit_free_list_p (int gen_number, { assert (prev_free_item == 0); dprintf (3, ("couldn't use this free area, discarding")); + generation_free_obj_space (gen) += free_list_size; - // Update ordering for lock-free readers (e.g. GCHeap::ApproxTotalBytesInUse): - // remove the bytes from free_list_space before adding them to free_obj_space so - // the transient a concurrent reader can observe is a harmless under-count rather - // than an over-count that would make gen0 fragmentation (free_list_space + - // free_obj_space) appear larger than the gen0 span and underflow (issue #106712). gen_allocator->unlink_item (a_l_idx, free_list, prev_free_item, FALSE); generation_free_list_space (gen) -= free_list_size; - generation_free_obj_space (gen) += free_list_size; assert ((ptrdiff_t)generation_free_list_space (gen) >= 0); } else diff --git a/src/coreclr/gc/interface.cpp b/src/coreclr/gc/interface.cpp index 0a260a50638507..9d393e001d3204 100644 --- a/src/coreclr/gc/interface.cpp +++ b/src/coreclr/gc/interface.cpp @@ -2075,14 +2075,6 @@ size_t GCHeap::ApproxTotalBytesInUse(BOOL small_heap_only) uint8_t* current_alloc_allocated = pGenGCHeap->alloc_allocated; size_t gen0_size = 0; #ifdef USE_REGIONS - // Walk every gen0 region. free_list_space/free_obj_space (gen0_frag) are per-generation - // totals accumulated across all gen0 regions, so gen0_size must likewise span all of them. - // The ephemeral region is the one that holds alloc_allocated and is capped there; any other - // gen0 region (which can be linked before or after the ephemeral one, e.g. a pinned region - // that survived in place) is counted up to its heap_segment_allocated. Stopping at the - // ephemeral region here used to drop those regions' span while still subtracting their - // fragmentation, making gen0_frag exceed gen0_size and underflowing the subtraction below - // (issue #106712). heap_segment* gen0_seg = generation_start_segment (gen); while (gen0_seg) { From 024b3d67478cbfd7735a0f1749565d4182152099 Mon Sep 17 00:00:00 2001 From: Konrad Kokosa Date: Fri, 24 Jul 2026 15:08:14 +0200 Subject: [PATCH 4/6] Clamp ApproxTotalBytesInUse gen0 subtraction as defense-in-depth The region-walk fix already removes the structural underflow at its source. Add a defensive clamp so the residual lock-free transient (gen0_frag updated by the allocator under more_space_lock_soh, not the gc_lock held by the reader) can never surface as a negative from GC.GetTotalMemory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ad1ce451-e247-42ec-accc-c4a688fcd555 --- src/coreclr/gc/interface.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/gc/interface.cpp b/src/coreclr/gc/interface.cpp index 9d393e001d3204..68237eb71a4814 100644 --- a/src/coreclr/gc/interface.cpp +++ b/src/coreclr/gc/interface.cpp @@ -2090,7 +2090,8 @@ size_t GCHeap::ApproxTotalBytesInUse(BOOL small_heap_only) gen0_size = current_alloc_allocated - heap_segment_mem (current_eph_seg); #endif //USE_REGIONS - totsize = gen0_size - gen0_frag; + // Defense-in-depth clamp: gen0_frag is updated by the allocator under a different lock, so a lock-free read must never underflow to a negative. + totsize = (gen0_size > gen0_frag) ? (gen0_size - gen0_frag) : 0; int stop_gen_index = max_generation; From 1e8f2bce9ddde649a154b15744618bb48640dfc3 Mon Sep 17 00:00:00 2001 From: Konrad Kokosa Date: Fri, 24 Jul 2026 15:18:05 +0200 Subject: [PATCH 5/6] Small comment improvement Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/coreclr/gc/interface.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/gc/interface.cpp b/src/coreclr/gc/interface.cpp index 68237eb71a4814..2d1f31b52dbc79 100644 --- a/src/coreclr/gc/interface.cpp +++ b/src/coreclr/gc/interface.cpp @@ -2090,7 +2090,8 @@ size_t GCHeap::ApproxTotalBytesInUse(BOOL small_heap_only) gen0_size = current_alloc_allocated - heap_segment_mem (current_eph_seg); #endif //USE_REGIONS - // Defense-in-depth clamp: gen0_frag is updated by the allocator under a different lock, so a lock-free read must never underflow to a negative. + // Defense-in-depth clamp: gen0 frag counters are updated by the allocator under a different lock. + // This read can observe a transiently inconsistent snapshot; avoid underflow. totsize = (gen0_size > gen0_frag) ? (gen0_size - gen0_frag) : 0; int stop_gen_index = max_generation; From d2b921971d5e7b3cef0f5f611d42f7becd6fe39f Mon Sep 17 00:00:00 2001 From: Konrad Kokosa Date: Fri, 24 Jul 2026 15:30:03 +0200 Subject: [PATCH 6/6] Assert worker threads stop within join timeout in regression test Thread.Join(TimeSpan) returns false on timeout; the result was ignored, so a worker that never observed s_stop could be silently skipped and the test could pass while masking a cleanup/liveness issue. Capture the join result and assert all workers stopped, after the primary non-negative assertion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ad1ce451-e247-42ec-accc-c4a688fcd555 --- src/tests/GC/API/GC/GetTotalMemoryConcurrent.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/tests/GC/API/GC/GetTotalMemoryConcurrent.cs b/src/tests/GC/API/GC/GetTotalMemoryConcurrent.cs index 5da21d8f25c7e7..933269e842967c 100644 --- a/src/tests/GC/API/GC/GetTotalMemoryConcurrent.cs +++ b/src/tests/GC/API/GC/GetTotalMemoryConcurrent.cs @@ -49,6 +49,7 @@ public static void TestEntryPoint() long probes = 0; long minObserved = long.MaxValue; long negative = 0; + bool allStopped = true; var sw = Stopwatch.StartNew(); try { @@ -73,12 +74,16 @@ public static void TestEntryPoint() s_stop = true; foreach (Thread t in threads) { - t.Join(TimeSpan.FromSeconds(5)); + if (!t.Join(TimeSpan.FromSeconds(5))) + { + allStopped = false; + } } } Console.WriteLine($"probes={probes}, min observed={minObserved}, negative={negative}"); Assert.True(negative >= 0, $"GC.GetTotalMemory(false) returned a negative value: {negative}"); + Assert.True(allStopped, "A worker thread did not stop within the join timeout."); } // Keeps a large ring of pinned tiny objects alive, refreshing the oldest one each iteration, and