Skip to content

Fix GC page size assumptions - #134059

Open
kkokosa wants to merge 1 commit into
dotnet:mainfrom
kkokosa:fix-issue-133743-invalid-page-size-assum
Open

kkokosa wants to merge 1 commit into
dotnet:mainfrom
kkokosa:fix-issue-133743-invalid-page-size-assum

Conversation

@kkokosa

@kkokosa kkokosa commented Sep 16, 2026

Copy link
Copy Markdown
Member

Fixes #133743.

On systems with OS pages larger than 4 KiB, GC regions could have mark bitmaps smaller than one OS page, causing regions to share committed bitmap pages and corrupt commitment accounting. The pre-OOM path also compared region commitment against the logical 4 KiB GC_PAGE_SIZE instead of the platform initial commit size.

This change:

  • derives the minimum safe region size from OS_PAGE_SIZE and mark-word coverage
  • rejects explicitly configured region sizes below that minimum
  • uses SEGMENT_INITIAL_COMMIT for pre-OOM reclamation
  • adds a background-GC regression test

Validated on ARM64 Linux with both 64 KiB and 4 KiB kernels. The regression fails before and passes after the fix on 64 KiB pages, while regular 4 KiB region sizing remains unchanged.

Ensure region mark bitmaps are independently page aligned and use the platform initial commit size when reclaiming regions before OOM.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings September 16, 2026 12:30
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @anicka-net, @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Existing ARM64 configurations may fail with 64-KiB pages, and the sizing comment remains inaccurate.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes GC region sizing and commitment accounting on systems with OS pages larger than 4 KiB.

Changes:

  • Enforces a page-size-aware minimum GC region size.
  • Uses SEGMENT_INITIAL_COMMIT for pre-OOM reclamation.
  • Adds regression coverage and updates GC error resources.
File summaries
File Summary
src/tests/GC/Regressions/Github/Runtime_133743/Runtime_133743.csproj Configures the regression test.
src/tests/GC/Regressions/Github/Runtime_133743/Runtime_133743.cs Adds background-GC regression coverage.
src/coreclr/pal/prebuilt/corerror/mscorurt.rc Updates the corresponding error resource.
src/coreclr/inc/corerror.xml Updates GC region-size error text.
src/coreclr/gc/regions_segments.cpp Corrects initial-commit comparison.
src/coreclr/gc/interface.cpp Validates minimum region size. Moderate issue (3 votes): existing ARM64 configurations may fail on 64-KiB-page systems; nit (1 vote): the sizing comment no longer matches behavior.
src/coreclr/gc/gcpriv.h Documents logical GC page granularity.
Review details

Suppressed comments (1)

src/coreclr/gc/interface.cpp:451

  • This revised comment still says automatic sizing chooses only 4, 2, or 1 MiB, but the max below can now select the platform-dependent minimum (8 MiB on 64-KiB pages). Please document the base-size selection and the subsequent minimum-size adjustment so the comment matches the new behavior.
    // except for the smallest case.
  • Files reviewed: 7/7 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment on lines +472 to +476
else if (gc_region_size < min_gc_region_size)
{
log_init_error_to_host ("The GC RegionSize config is set to %zd bytes, it needs to be >= %zd bytes for the OS page size",
gc_region_size, min_gc_region_size);
return CLR_E_GC_BAD_REGION_SIZE;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use something like this, fe. for Runtime_126043, to exclude those tests on non-4 KiB machines?

<CLRTestBashPreCommands><![CDATA[
  $(CLRTestBashPreCommands)
  page_size=`getconf PAGESIZE`
  if [ "$page_size" -gt 32768 ]; then
    echo "Test requires 4 MiB GC regions, which are unsupported with ${page_size}-byte OS pages."
    exit 0
  fi
  export DOTNET_gcServer=1
  export DOTNET_GCHeapCount=1
  export DOTNET_GCRegionSize=400000
]]></CLRTestBashPreCommands>

@kkokosa
kkokosa requested a review from janvorli September 16, 2026 12:50
@jkotas

jkotas commented Sep 16, 2026

Copy link
Copy Markdown
Member

cc @tmds

<Message>"GC Region Size must be less than 2GB."</Message>
<Comment>During a GC initialization, GC Region Size must be less than 2GB.</Comment>
<Message>"GC Region Size configuration is invalid."</Message>
<Comment>During a GC initialization, the GC Region Size configuration is invalid.</Comment>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather say:

Suggested change
<Comment>During a GC initialization, the GC Region Size configuration is invalid.</Comment>
<Comment>During a GC initialization, the GC Region Size configuration must be valid.</Comment>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Invalid page size assumptions in GC

4 participants