Fixed malloc with concurrent executions of memory.grow #404

MoritzS · 2023-04-02T20:52:57Z

Other PRs have fixed issues with malloc not being the only code that may use the memory.grow instruction (#377, #394). This merge requests fixes some issues that can occur when running the WebAssembly module multi-threaded and other threads may call memory.grow concurrently while malloc is executed.

sunfishcode · 2023-04-03T20:33:11Z

Interesting. dlmalloc's documentation for MORECORE_CONTIGUOUS says that it should be correct in the presence of non-continuities, which I assumed would include non-continuities induced by other threads. But seeing code like this:

     br = (char*)(CALL_MORECORE(asize));
     end = (char*)(CALL_MORECORE(0));

It appears this is not the case. At an initial glance, this PR makes sense to me, though I'm not super familiar with dlmalloc internals.

yamt · 2023-04-14T06:44:43Z

dlmalloc/src/malloc.c

+#else
+    // In wasi-libc, this branch should never be taken as CALL_MORECORE(0) may
+    // lead to incorrect allocations. See try_init_allocator() for more
+    // information.


this is inside if (MORECORE_CONTIGUOUS && ... block.
if we are using MORECORE_CONTIGUOUS=1, it's the bug to fix i guess.

In general I would say setting MORECORE_CONTIGUOUS to 1 is correct. The comments in malloc.c say this:

MORECORE_CONTIGUOUS default: 1 (true) if HAVE_MORECORE If true, take advantage of fact that consecutive calls to MORECORE with positive arguments always return contiguous increasing addresses. This is true of unix sbrk. It does not hurt too much to set it true anyway, since malloc copes with non-contiguities. Setting it false when definitely non-contiguous saves time and possibly wasted space it would take to discover this though.

So, in general I think we definitely want to benefit from the fact that the WebAssembly memory is contiguous and only fall back to the non-contiguous malloc if some other allocator uses memory.grow in the same program, as well.

ok. it makes sense.
i vaguely concern that discontiguous fallback logic will never be tested well though.

yamt · 2023-04-27T05:42:38Z

dlmalloc/src/malloc.c

@@ -4139,13 +4147,23 @@ static void* sys_alloc(mstate m, size_t nb) {
            ssize < nb + SYS_ALLOC_PADDING) {
          size_t esize = granularity_align(nb + SYS_ALLOC_PADDING - ssize);
          if (esize < HALF_MAX_SIZE_T) {
+#ifdef __wasilibc_unmodified_upstream
            char* end = (char*)CALL_MORECORE(esize);
            if (end != CMFAIL)
              ssize += esize;


it seems like an upstream bug to assume contiguity.
does dlmalloc still have a maintained upstream?

dmalloc upstream has not been updated since 2012.

The comments at the top of dlmalloc say that it should handle the case of non-contiguous memory, though it will opportunistically detect contiguous memory and make use of it. If it has a bug, it'd be good to have a comment here mentioning that we're fixing an upstream bug.

I don't think this is a bug, as it works correctly if dlmalloc is the only part of the program that uses sbrk (or MORECORE). That's just not necessarily the case in wasm where we want to allow other functions to grow the memory.

That's just not necessarily the case in wasm where we want to allow other functions to grow the memory.

Can you remind me where is this requirement coming from? Would it not simplify thing considerably if were to declare it undefined behavior the call sbkr() or memory.grow on a memory that was owned/used/controlled by malloc? i.e. can we not consider malloc as owning the entire region of memory from __heap_base to the end of the memory?

I agree with giving malloc all the memory between __heap_base and __heap_end (which is not the end of the entire memory!), but I don't think we should prohibit other functions from growing the memory.

The use-case for me is to allow programs compiled with wasi-libc to have their own memory allocators in addition to malloc. When an application has very specific memory allocation patterns, such as all allocations having the same size, it is often more efficient to write a specialized allocator for a specific part of the program. On a regular linux target, you would then use mmap to allocate entire pages, so you don't need to mess with sbrk. However, in WebAssembly, sbrk/memory.grow is the only way to allocate new memory, so custom allocators need to able to use it without breaking malloc.

In any case, all memory pages allocated by malloc using memory.grow will continue to be used exclusively by malloc.

I do wonder if its worth it (or possible) to support that kind of thing.

In that first example wouldn't make more sense if mypage was just statically allocated in the assembly file? e.g:

.section .data.mypage,"",@ mypage: .size mypage, 65536

no. because the linear memory is shared with another module.

Then the linker could place it and it would no runtime cost to allocate.

I'm not sure what the second example is used for, but I wonder if it couldn't depend on the malloc symbol existing? Or at least weakly depend on it so its used when it exists?

it's basically same as the first example.

i don't think it's possible unless malloc is exported by the module.

a. do not support it (memory.grow outside of this malloc) at all
b. support it only if it's done at the very beginning of the module lifetime (eg. around wasm start function)
c. support it only if it's done before the first call of malloc
d. support it only if it's serialized with normal malloc/free calls
e. support it anytime, even from another thread

@MoritzS
can't you make your use case backed by malloc as @sbc100 suggested? why not?
if you can't, which of the above do you need for your use case?

@alexcrichton @sunfishcode
which does the preview1 adapter need?

I do wonder if its worth it (or possible) to support that kind of thing.
In that first example wouldn't make more sense if mypage was just statically allocated in the assembly file? e.g:

.section .data.mypage,"",@ mypage: .size mypage, 65536

no. because the linear memory is shared with another module.

If you have two modules sharing the same memory, isn't its reasonable to assume that one of them will export a malloc function and the other one will use it. The need to coordinate somehow, and having them both call memory.grow and while assume that other can just deal with it seems.. fragile. How do you know the allocator in other other module is ok with having a non-contiguous heap? Isn't that fairly big assumption? Why not just mandate that the other module export an allocator for you to use?

I do wonder if its worth it (or possible) to support that kind of thing.
In that first example wouldn't make more sense if mypage was just statically allocated in the assembly file? e.g:

.section .data.mypage,"",@ mypage: .size mypage, 65536

no. because the linear memory is shared with another module.

If you have two modules sharing the same memory, isn't its reasonable to assume that one of them will export a malloc function and the other one will use it. The need to coordinate somehow, and having them both call memory.grow and while assume that other can just deal with it seems.. fragile. How do you know the allocator in other other module is ok with having a non-contiguous heap? Isn't that fairly big assumption? Why not just mandate that the other module export an allocator for you to use?

it isn't possible to assume proper export of malloc functions when you want to deal with the existing wasi modules.
i totally agree it's a fragile hack.

which does the preview1 adapter need?

Sorry I haven't been keeping up with the rest of the context here, but the preview1 adapter won't ever be used with threads so it's ok if something odd goes wrong. Ideally wasi-libc would allow other modules to use memory.grow instructions themselves, however, and continue to work with that.

yamt · 2023-04-27T06:15:53Z

dlmalloc/src/malloc.c

+    // In wasi-libc, this branch should never be taken as CALL_MORECORE(0) may
+    // lead to incorrect allocations. See try_init_allocator() for more
+    // information.
+    assert(ss != 0);


but ss can be 0 here if there was not enough space in the initial heap, can't it?

@MoritzS Would you be able to comment on this question?

That is true, however we still cannot take the if-branch because using CALL_MORECORE(0) will almost always be incorrect (and that's the whole point of why try_init_allocator() exists). So, maybe we should make sure that try_init_allocator() either allocates at least one page if the initial heap is too small, or maybe even directly abort?

what's wrong with assuming the discontiguous case if ss == 0?

You are right. I initially assumed that ss can only be 0 in cases where the memory allocation failed, but that is not the case. I will change the code so that it falls back to the non-contiguous case if ss is 0.

Fixed malloc with concurrent executions of memory.grow

0f3800e

wenyongh mentioned this pull request Apr 3, 2023

Upgrade to wasi-sdk 20 bytecodealliance/wasm-micro-runtime#2095

Merged

yamt suggested changes Apr 14, 2023

View reviewed changes

yamt reviewed Apr 27, 2023

View reviewed changes

abrown assigned sbc100 Jul 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed malloc with concurrent executions of memory.grow #404

Fixed malloc with concurrent executions of memory.grow #404

MoritzS commented Apr 2, 2023

sunfishcode commented Apr 3, 2023

yamt Apr 14, 2023

MoritzS Apr 19, 2023

yamt Apr 27, 2023

yamt Apr 27, 2023

sunfishcode Jun 2, 2023

MoritzS Jun 26, 2023

sbc100 Jun 26, 2023

MoritzS Jun 27, 2023

yamt Jun 30, 2023

yamt Jun 30, 2023

sbc100 Jun 30, 2023

yamt Jun 30, 2023

alexcrichton Jun 30, 2023

yamt Apr 27, 2023

sunfishcode Jun 2, 2023

MoritzS Jun 4, 2023

yamt Jun 16, 2023

MoritzS Jun 26, 2023

Fixed malloc with concurrent executions of memory.grow #404

Are you sure you want to change the base?

Fixed malloc with concurrent executions of memory.grow #404

Conversation

MoritzS commented Apr 2, 2023

sunfishcode commented Apr 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment