dotnet / corert

This repo contains CoreRT, an experimental .NET Core runtime optimized for AOT (ahead of time compilation) scenarios, with the accompanying compiler toolchain.
http://dot.net
MIT License
2.91k stars 508 forks source link

Wasm: munmap possible problem #8330

Open yowl opened 4 years ago

yowl commented 4 years ago

Emscripten basically maps mmap/munmap to malloc/free. The second parameter to munmap is effectively unused https://github.com/emscripten-core/emscripten/blob/3abb4a6993ef97d652eae673cd79ffcdf97732dd/src/library_syscall.js#L275 I think this means that https://github.com/dotnet/corert/blob/a892bbad6795125a460453125489d2cbad325fd2/src/Native/gc/unix/gcenv.unix.cpp#L624-L635 is going to be a problem. startPadding never seems to be non zero, but endPadding often is.

If the alignment == OS_PAGE_SIZE for Wasm that might be a workaround, or maybe VirtualReserveInner and VirtualRelease that just went to malloc/free ?

jkotas commented 4 years ago

You may need to do manual tracking of allocate blocks to solve this. Similar to what @RalfKornmannEnvision had to do for Switch. https://github.com/dotnet/runtime/issues/41675 has the details.

RalfKornmannEnvision commented 4 years ago

I think this is a somewhat different problem. In my case it was about committing the same page twice. And it only happens when the background GC activates. Based on what I have read so far the issue here is that the memory is not released when the GC doesn't need it anymore.

Based on the comment in the emscripten code the mmap/unmap emulation is not fully implemented. This might be part of the issue.

My sugestion here would be going with your implmention for VirtualReserve,VirtualRelease, VirtualCommit and VirtualDecommit.

Commit and Decommit doesn't need to do anything.  Reserve ist a aligned_alloc + meset and release just a free. As far as I have seen VirtualRelease never release less than VirtualReserve have asked for. So it should be fine that free doesn't get the size.

yowl commented 4 years ago

Thanks, I think I'll still need to track allocations due to the alignment requirement. The address returned out of VirtualReserve is not the same as the address from malloc but when VirtualReleased is called it needs to track if all the malloc space is released and only then call free (ignoring any space discarded due to padding for the alignment.

yowl commented 4 years ago

"VirtualRelease never release less than VirtualReserve" That helps in the tracking for space, but still need to go from the aligned address to the malloc block

RalfKornmannEnvision commented 4 years ago

Not sure if this works but could you use memalign instead of malloc?

The mmap code uses it, too

https://github.com/emscripten-core/emscripten/blob/3abb4a6993ef97d652eae673cd79ffcdf97732dd/src/library_syscall.js#L241

yowl commented 4 years ago

That looks like its worth a try, thanks

yowl commented 4 years ago

Something I don't understand is that

size_t alignedSize = size + (alignment - OS_PAGE_SIZE);

If alignment < OS_PAGE_SIZE then you get less space than you asked for, but the caller doesn't know, and this seems to happen, so how does the caller know how much space it gets?

RalfKornmannEnvision commented 4 years ago

I think it just have never happened as the GC always seems to ask for 8K and it seems every regular unix OS uses 4K page sizes.

If the memalign works there would be no need to increase the size

yowl commented 4 years ago

Wasm page size seems to be 16k, and some requests are for 4k alignment, so a request for 0x1000000 gets 0xffd000. As you say, if I switch it to memalign then it shouldn't matter.

yowl commented 4 years ago

Seems like it should be 4 but at runtime is 0x4000, strange

#elif defined(HOST_WASM)

#define DATA_ALIGNMENT  4
#ifndef OS_PAGE_SIZE
#define OS_PAGE_SIZE    0x4
#endif
RalfKornmannEnvision commented 4 years ago

OS_PAGE_SIZE is normaly set to GCToOSInterface::GetPageSize()

#define OS_PAGE_SIZE GCToOSInterface::GetPageSize()

this returns the value of g_pageSizeUnixInl

and g_pageSizeUnixInl is set in GCToOSInterface::Initialize()

int pageSize = sysconf( _SC_PAGE_SIZE );

 g_pageSizeUnixInl = uint32_t((pageSize > 0) ? pageSize : 0x1000);

If it is 16K the sysconf call reports it this way. Not sure why the go for 16K as 4K is a more common value for page size.

yowl commented 4 years ago

WebAssembly natural page size is 64KB, that being the Windows page size and the lowest common denominator. 16K is a bit of a hangover and I think they may just go to 64KB in the future. I think I've got enough for a PR now so thanks for the help.

Suchiman commented 4 years ago

IIRC Windows has 4KB (0x1000) page size.

yowl commented 4 years ago

ok, I shamelessly quoted without checking from discord

It was chosen as the LCM of the page sizes on all major platforms (and windows has 64k pages).
yowl commented 4 years ago

So now I need to go back there as my Win10 reports

The page size for this system is 4096 bytes.
Suchiman commented 4 years ago

Windows supports page sizes other than 4KB as well (e.g. https://docs.microsoft.com/en-us/windows-hardware/drivers/display/support-for-64kb-pages ) but this is like the only resource i can find on it, there are also "large" pages in the megabyte range for which the GC has support as well (although it's not wired up in CoreRT AFAIK). Downside of large pages are, is that they're not pageable (badum ts).

jkotas commented 4 years ago

Windows has 4kB page size and 64kB allocation granularity (ie the OS gives you the VM space in 64kB chunks).

RalfKornmannEnvision commented 4 years ago

Windows supports page sizes other than 4KB as well (e.g. https://docs.microsoft.com/en-us/windows-hardware/drivers/display/support-for-64kb-pages ) but this is like the only resource i can find on it, there are also "large" pages in the megabyte range for which the GC has support as well (although it's not wired up in CoreRT AFAIK). Downside of large pages are, is that they're not pageable (badum ts).

The large pages in CoreRT work. At least together with a hard limit. This combination give me a big performances improvment on the Switch/PlayStation