Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RISC-V][GC] Use 128gb for regions_range on RISCV64 #84797

Merged
merged 2 commits into from
May 5, 2023

Conversation

t-mustafin
Copy link
Contributor

Use 128gb for regions_range cause RISCV64 SV39 memory layout allows to use only 256gb of virtual memory: https://docs.kernel.org/riscv/vm-layout.html#risc-v-linux-kernel-sv39.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 13, 2023
@ghost
Copy link

ghost commented Apr 13, 2023

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

Issue Details

Use 128gb for regions_range cause RISCV64 SV39 memory layout allows to use only 256gb of virtual memory: https://docs.kernel.org/riscv/vm-layout.html#risc-v-linux-kernel-sv39.

Author: t-mustafin
Assignees: -
Labels:

area-GC-coreclr, community-contribution

Milestone: -

src/coreclr/gc/gc.cpp Outdated Show resolved Hide resolved
src/coreclr/gc/gc.cpp Outdated Show resolved Hide resolved
@jkotas jkotas added the arch-riscv Related to the RISC-V architecture label Apr 13, 2023
@t-mustafin t-mustafin marked this pull request as draft April 15, 2023 09:43
@t-mustafin t-mustafin marked this pull request as ready for review April 15, 2023 10:14
@t-mustafin t-mustafin changed the title Use 128gb for regions_range on RISCV64 [RISC-V][GC] Use 128gb for regions_range on RISCV64 Apr 15, 2023
Copy link
Member

@mangod9 mangod9 left a comment

Choose a reason for hiding this comment

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

Nit: might even be easier to make it REGIONS_RESERVATION_SIZE_GB to reduce the 1024 * duplication.

@cshung
Copy link
Member

cshung commented Apr 15, 2023

@janvorli, is that 256GB limit something we can deduce from the operating system so that we don't need to hardcode it?

@janvorli
Copy link
Member

@cshung unfortunately there is no API to get virtual memory size limit on Linux. However, it seems it would be better to use the value returned by the GCToOSInterface::GetVirtualMemoryLimit derived value as a limit and modify the Unix implementation of the method. Currently it looks like this:

size_t GCToOSInterface::GetVirtualMemoryLimit()
{
#ifdef HOST_64BIT
// There is no API to get the total virtual address space size on
// Unix, so we use a constant value representing 128TB, which is
// the approximate size of total user virtual address space on
// the currently supported Unix systems.
static const uint64_t _128TB = (1ull << 47);
return _128TB;
#else
return (size_t)-1;
#endif
}

@t-mustafin we should also modify the GlobalMemoryStatusEx implementation in PAL here:
// There is no API to get the total virtual address space size on
// Unix, so we use a constant value representing 128TB, which is
// the approximate size of total user virtual address space on
// the currently supported Unix systems.
static const UINT64 _128TB = (1ull << 47);
lpBuffer->ullTotalVirtual = _128TB;

@t-mustafin
Copy link
Contributor Author

@janvorli GCToOSInterface::GetVirtualMemoryLimit() returns 128TB for non-RISCV archetictures instead of 256GB used by gc_heap::regions_range. To avoid magic constants in code I propose to use half of GCToOSInterface::GetVirtualMemoryLimit() to regions mapping.

And I also added RISCV ifdef for GlobalMemoryStatusEx.

src/coreclr/gc/gc.cpp Show resolved Hide resolved
src/coreclr/gc/gc.cpp Show resolved Hide resolved
Use 128gb for regions_range cause RISCV64 SV39 memory layout allows to use only 256gb of virtual memory: https://docs.kernel.org/riscv/vm-layout.html#risc-v-linux-kernel-sv39.
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@janvorli janvorli merged commit 82640bd into dotnet:main May 5, 2023
Copy link
Member

@gbalykov gbalykov left a comment

Choose a reason for hiding this comment

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

LGTM

@t-mustafin t-mustafin deleted the riscv_oom_fix branch May 31, 2023 20:18
@ghost ghost locked as resolved and limited conversation to collaborators Jul 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-riscv Related to the RISC-V architecture area-GC-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants