Skip to content
This repository has been archived by the owner on Nov 1, 2020. It is now read-only.

Wasm: munmap possible problem #8330

Open
yowl opened this issue Sep 15, 2020 · 18 comments
Open

Wasm: munmap possible problem #8330

yowl opened this issue Sep 15, 2020 · 18 comments

Comments

@yowl
Copy link
Contributor

yowl commented Sep 15, 2020

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

if (startPadding != 0)
{
int ret = munmap(pRetVal, startPadding);
assert(ret == 0);
}
size_t endPadding = alignedSize - (startPadding + size);
if (endPadding != 0)
{
int ret = munmap((void *)((size_t)pAlignedRetVal + size), endPadding);
assert(ret == 0);
}
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
Copy link
Member

jkotas commented Sep 15, 2020

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

@RalfKornmannEnvision
Copy link
Contributor

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
Copy link
Contributor Author

yowl commented Sep 15, 2020

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
Copy link
Contributor Author

yowl commented Sep 15, 2020

"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
Copy link
Contributor

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
Copy link
Contributor Author

yowl commented Sep 15, 2020

That looks like its worth a try, thanks

@yowl
Copy link
Contributor Author

yowl commented Sep 15, 2020

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
Copy link
Contributor

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
Copy link
Contributor Author

yowl commented Sep 15, 2020

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
Copy link
Contributor Author

yowl commented Sep 15, 2020

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
Copy link
Contributor

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
Copy link
Contributor Author

yowl commented Sep 15, 2020

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
Copy link
Contributor

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

@yowl
Copy link
Contributor Author

yowl commented Sep 15, 2020

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
Copy link
Contributor Author

yowl commented Sep 15, 2020

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

The page size for this system is 4096 bytes.

@Suchiman
Copy link
Contributor

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
Copy link
Member

jkotas commented Sep 16, 2020

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

@RalfKornmannEnvision
Copy link
Contributor

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants