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

[NativeAOT] Support variable page size on Linux Arm64 #88710

Merged
merged 8 commits into from
Jul 14, 2023
Merged

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Jul 12, 2023

OS Page size can be configured on some platforms. (i.e. Linux arm64)

Fixes: #75737

@ghost
Copy link

ghost commented Jul 12, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

OS Page size can be configured on some platforms. (i.e. Linux arm64)

Fixes: #75737

Author: VSadov
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@@ -56,7 +57,7 @@ void EncodeThumb2Mov32(uint16_t * pCode, uint32_t value, uint8_t rDestination)

COOP_PINVOKE_HELPER(int, RhpGetNumThunkBlocksPerMapping, ())
{
static_assert((THUNKS_MAP_SIZE % OS_PAGE_SIZE) == 0, "Thunks map size should be in multiples of pages");
ASSERT_MSG((THUNKS_MAP_SIZE % OS_PAGE_SIZE) == 0, "Thunks map size should be in multiples of pages");
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, 64kB is a common page size on Linux Arm64. It is going to assert there.

Ask @janvorli if you need help getting Linux arm64 machine configured like that for testing.

Copy link
Member Author

@VSadov VSadov Jul 12, 2023

Choose a reason for hiding this comment

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

with #define THUNKS_MAP_SIZE (max(0x8000, OS_PAGE_SIZE)) would it assert?

It would still be interesting to run this on an actual system with 64K pages.

Copy link
Member

Choose a reason for hiding this comment

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

You are right. I got it backwards.

FORCEINLINE int32_t PalOsPageSize()
{
#ifdef HOST_APPLE
return 0x4000;
Copy link
Member

Choose a reason for hiding this comment

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

This is true for arm64. Is it true on x64 machines as well? Or are we pessimizing it here for simplicity?

Copy link
Member

Choose a reason for hiding this comment

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

Is it true on x64 machines as well?

No, it is not. macOS x64 still uses 4Kb pages.

Copy link
Member Author

Choose a reason for hiding this comment

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

I forgot that OSX on x64 is still possible :-). Yes, on x64 everything uses 4K as OS page size.

Copy link
Member

Choose a reason for hiding this comment

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

Is it the case even with x86 emulator on OSX arm64?

The OS can emulate larger page size than the hardware supports, but doing it the other way around is hard.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it the case even with x86 emulator on OSX arm64?

good question. arm64 should support 4K pages. However, what Rosetta does when running on M1 - not sure. I'd guess 16K pages on x64 could be a breaking change. There is one way to find out.

Copy link
Member

Choose a reason for hiding this comment

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

Is it the case even with x86 emulator on OSX arm64?

Yes, is it (with the exception of the first ARM Dev Kits that are no longer in circulation).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, is it (with the exception of the first ARM Dev Kits that are no longer in circulation).

I assume that is the "yes" for 4K pages? :-)

I wonder if there are any docs, but emulation could be too much of a corner case and a temporary solution, to document such details.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, always 4K pages even in Rosetta emulation.

if (saved_pagesize)
return saved_pagesize;

// Prefer sysconf () as it's signal safe.
Copy link
Member

Choose a reason for hiding this comment

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

Do we care about signal safety here? If we do, we can initialize this during PalInit like it is done in CoreCLR.

Copy link
Member Author

Choose a reason for hiding this comment

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

The same pattern is used in mono in multiple paces. I've just copied it, together with the comment. I guess when either one can be used signal safety might be an advantage.
We do not call this from code that must be signal-safe right now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can drop the comment about signal safety - not very relevant to us, but keep the pattern - use sysconf, if can, then use getpagesize.

#if defined (HAVE_SYSCONF) && defined (_SC_PAGESIZE)
saved_pagesize = sysconf(_SC_PAGESIZE);
#else
saved_pagesize = getpagesize();
Copy link
Member

Choose a reason for hiding this comment

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

I have notice that CoreCLR PAL uses getpagesize, but GC uses sysconf(_SC_PAGESIZE). Is there difference between the two? Which one is better?

Copy link
Member Author

Choose a reason for hiding this comment

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

the documentation seems to direct to sysconf as more portable:

DESCRIPTION
       The function getpagesize() returns the number of bytes in a memory page, where "page" is a fixed-length block,
       the unit for memory allocation and file mapping performed by mmap(2).

CONFORMING TO
       SVr4, 4.4BSD, SUSv2.  In SUSv2 the getpagesize() call is labeled LEGACY,  and  in  POSIX.1-2001  it  has  been
       dropped; HP-UX does not have this call.

NOTES
       Portable applications should employ sysconf(_SC_PAGESIZE) instead of getpagesize():

Copy link
Member

Choose a reason for hiding this comment

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

So just call sysconf(_SC_PAGESIZE) everywhere without any ifdefs?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought so, but mono does ifdefs and has a fallback to getpagesize. Perhaps the call is not always present. I noticed that coreclr also uses sysconf conditionally. Maybe to handle some legacy cases where it is not there or for some other platforms.

My conclusion was - we can see one or another missing. If both are present, use sysconf. If neither - fail to build.

From the mono pattern it looks like it is also possible that sysconf is there, but _SC_PAGESIZE is missing.

Copy link
Member Author

@VSadov VSadov Jul 12, 2023

Choose a reason for hiding this comment

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

Is there sysconf on bionic or on some FreeBSD derived OS for consoles?
I think from portability point of view the pattern from mono has some advantages, while not a big burden on mainstream Linux.

Copy link
Member

Choose a reason for hiding this comment

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

GC calls sysconf unconditionally and it works fine for all platforms that we care about.

I would expect that this ifdef is only needed for some old systems that we do not care about.

Copy link
Member Author

@VSadov VSadov Jul 12, 2023

Choose a reason for hiding this comment

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

no point to be more portable than GC. I will switch to calling sysconf unconditionally then.

Co-authored-by: Filip Navara <filip.navara@gmail.com>
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

@jkotas jkotas merged commit 8bf03a6 into dotnet:main Jul 14, 2023
100 of 106 checks passed

void InitializeOsPageSize()
{
g_RhPageSize = (uint32_t)sysconf(_SC_PAGE_SIZE);
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps better:

#ifdef OS_PAGE_SIZE
    g_RhPageSize = (uint32_t)OS_PAGE_SIZE;
#else
    g_RhPageSize = (uint32_t)sysconf(_SC_PAGE_SIZE);
#endif

, delete PalOsPageSize and use PalGetOsPageSize everywhere?

nit: Os -> OS (two-letter abbreviation rule)

Copy link
Member Author

@VSadov VSadov Jul 14, 2023

Choose a reason for hiding this comment

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

delete PalOsPageSize and use PalGetOsPageSize everywhere?

I think we need one that inlines, and one that does not. (and one calls another, if needed)
I did not replace uses of OS_PAGE_SIZE, though, as that would be a lot of changes and keeping OS_PAGE_SIZE would match closer the similar pattern in GC code.

@VSadov VSadov deleted the pgSize branch July 14, 2023 15:33
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NativeAOT] Handle variable page size on Linux Arm64
4 participants