Stream: git-wasmtime

Topic: wasmtime / issue #4997 Flush Icache on AArch64 Windows

Wasmtime GitHub notifications bot (Oct 03 2022 at 10:50):

bjorn3 commented on issue #4997:

BTW the purpose of the membarrier() call is not cache maintenance, but rather flushing the processor pipeline (refer to the discussion in https://github.com/bytecodealliance/wasmtime/pull/3426 for more details), so this means that there is still a gap in the implementation on Windows. Looking at Microsoft's documentation, FlushProcessWriteBuffers() seems to have the necessary semantics.

The documentation for FlushInstructionCache says:

https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-flushinstructioncache

Applications should call FlushInstructionCache if they generate or modify code in memory. The CPU cannot detect the change, and may execute the old code it cached.

So it seems that FlushInstructionCache must always be called no matter if FlushProcessWriteBuffers is called or not.

Wasmtime GitHub notifications bot (Oct 03 2022 at 10:58):

akirilov-arm commented on issue #4997:

Yes, I am not claiming that FlushProcessWriteBuffers() replaces FlushInstructionCache() - my point is that it (or something else with the necessary semantics) should be used after the latter, unless FlushInstructionCache() also performs an implicit broadcast ISB operation in all running threads of the process (which the documentation doesn't suggest, to me at least). Similarly, as I mentioned on Linux membarrier() does not flush instruction caches.

Wasmtime GitHub notifications bot (Oct 03 2022 at 11:01):

akirilov-arm edited a comment on issue #4997:

Yes, I am not claiming that FlushProcessWriteBuffers() replaces FlushInstructionCache() - my point is that it (or something else with the necessary semantics) should be used after the latter, unless FlushInstructionCache() also performs an implicit broadcast ISB operation in all running threads of the process (which the documentation doesn't suggest, to me at least). Similarly, as I mentioned on Linux membarrier() does not flush instruction caches.

P.S. The same considerations about single-threaded applications apply to FlushProcessWriteBuffers() as well, of course.

Wasmtime GitHub notifications bot (Oct 03 2022 at 11:01):

akirilov-arm edited a comment on issue #4997:

Yes, I am not claiming that FlushProcessWriteBuffers() replaces FlushInstructionCache() - my point is that it (or something else with the necessary semantics) should be used after the latter, unless FlushInstructionCache() also performs an implicit broadcast ISB operation to all running threads of the process (which the documentation doesn't suggest, to me at least). Similarly, as I mentioned on Linux membarrier() does not flush instruction caches.

P.S. The same considerations about single-threaded applications apply to FlushProcessWriteBuffers() as well, of course.

Wasmtime GitHub notifications bot (Oct 03 2022 at 11:10):

afonso360 commented on issue #4997:

Thanks for reviewing this!

BTW the purpose of the membarrier() call is not cache maintenance, but rather flushing the processor pipeline (refer to the discussion in https://github.com/bytecodealliance/wasmtime/pull/3426 for more details), so this means that there is still a gap in the implementation on Windows. Looking at Microsoft's documentation, FlushProcessWriteBuffers() seems to have the necessary semantics.

So, if I understand https://github.com/bytecodealliance/wasmtime/issues/3310 correctly we need to both flush the icache (clear_cache / FlushInstructionCache) and then subsequently also flush the pipeline (membarrier / FlushProcessWriteBuffers). The flush only needs to happen in multi threaded environments but we cant easily detect that.

Is that right? If that is the case I might as well just clean up the terminology and add a clear_cache call for linux and that also fixes #3310 right?

Wasmtime GitHub notifications bot (Oct 03 2022 at 11:10):

afonso360 edited a comment on issue #4997:

Thanks for reviewing this! :heart:️

BTW the purpose of the membarrier() call is not cache maintenance, but rather flushing the processor pipeline (refer to the discussion in https://github.com/bytecodealliance/wasmtime/pull/3426 for more details), so this means that there is still a gap in the implementation on Windows. Looking at Microsoft's documentation, FlushProcessWriteBuffers() seems to have the necessary semantics.

So, if I understand https://github.com/bytecodealliance/wasmtime/issues/3310 correctly we need to both flush the icache (clear_cache / FlushInstructionCache) and then subsequently also flush the pipeline (membarrier / FlushProcessWriteBuffers). The flush only needs to happen in multi threaded environments but we cant easily detect that.

Is that right? If that is the case I might as well just clean up the terminology and add a clear_cache call for linux and that also fixes #3310 right?

Wasmtime GitHub notifications bot (Oct 03 2022 at 11:11):

afonso360 edited a comment on issue #4997:

Thanks for reviewing this! :heart:️

BTW the purpose of the membarrier() call is not cache maintenance, but rather flushing the processor pipeline (refer to the discussion in https://github.com/bytecodealliance/wasmtime/pull/3426 for more details), so this means that there is still a gap in the implementation on Windows. Looking at Microsoft's documentation, FlushProcessWriteBuffers() seems to have the necessary semantics.

So, if I understand https://github.com/bytecodealliance/wasmtime/issues/3310 correctly we need to both flush the icache (clear_cache / FlushInstructionCache) and then subsequently also flush the pipeline (membarrier / FlushProcessWriteBuffers). The pipeline flush only needs to happen in multi threaded environments but we cant easily detect that.

Is that right? If that is the case I might as well just clean up the terminology and add a clear_cache call for linux and that also fixes #3310 right?

Wasmtime GitHub notifications bot (Oct 03 2022 at 12:19):

akirilov-arm commented on issue #4997:

Is that right?

Yes, your understanding is correct. Technically the pipeline flush is always mandatory, but all implementations of __builtin___clear_cache() that I am aware of end with an ISB instruction (and I'd guess that FlushInstructionCache() does too) and furthermore a system call (e.g. the subsequent change of the memory page permissions to executable, since we don't use RWX mappings) is a context synchronization operation as well.

The ideal solution, which I had in mind when I opened #3310, would be to have a generic clear_cache() function (probably in the compiler-builtins crate to mirror what GCC and LLVM are doing; I am not aware of anything like that existing, but I would be happy to be proven wrong). Then we would call that function, followed by membarrier()/FlushProcessWriteBuffers(), finishing with making the memory executable. The order of the last 2 operations does not matter (as long as no one tries to run the newly executable code without a pipeline flush before that), but I think that this sequence looks nicer.

Note that it might appear that on Linux we don't flush instruction caches, so there might be a correctness issue. Actually we are relying on an implementation detail (as discussed in #3426), but I'd rather see an explicit operation, hence I have kept #3310 open. However, I would advise against implementing the cache flushing instruction sequence inside Cranelift and/or Wasmtime because it is a bit involved and a solution that is easily accessible to all Rust users on AArch64 is a much better option. In the meantime we can continue relying on an implementation detail; any other cleanup would be appreciated, of course.

Wasmtime GitHub notifications bot (Oct 03 2022 at 12:32):

bjorn3 commented on issue #4997:

probably in the compiler-builtins crate to mirror what GCC and LLVM are doing

It should rather be in libcore as compiler-builtins is an implementation detail of rustc to be used by the compiler backend and never directly called by the user.

Wasmtime GitHub notifications bot (Oct 03 2022 at 15:11):

afonso360 commented on issue #4997:

Alright I think I understand this a little bit more! Thank you for your patience dealing with this.

However, I would advise against implementing the cache flushing instruction sequence inside Cranelift and/or Wasmtime because it is a bit involved and a solution that is easily accessible to all Rust users on AArch64 is a much better option. In the meantime we can continue relying on an implementation detail; any other cleanup would be appreciated, of course.

Yeah I also doubt I could write anything like that correctly :laughing: So I'm not very inclined to go that route.

What I think we could do is something along the lines of what @cfallin suggested on zulip create a jit-icache-coherence crate with the interface that we want, use that on both wasmtime-jit and cranelift-jit. This at least shares the windows implementation between both crates, we still need to duplicate the membarriers between rsix and libc since we want to keep those separate. But at least it is a little bit more organized.

For __builtin___clear_cache() can we for now link it in from a C file as you suggested? And later on use a version from libcore if one gets upstreamed. Or if we don't want to do that on wasmtime (due to not using rsix, not sure if that is a requirement), we can ignore it for now and have essentially the same implementation but shared with cranelift.

What do you guys think about this?

Note that it might appear that on Linux we don't flush instruction caches, so there might be a correctness issue. Actually we are relying on an implementation detail (as discussed in https://github.com/bytecodealliance/wasmtime/pull/3426), but I'd rather see an explicit operation, hence I have kept https://github.com/bytecodealliance/wasmtime/issues/3310 open.

Yeah that confused me for a while! This whole thing is not easy to understand. I'd really like to get this in a central place and write all of these details in comments around this code.

Wasmtime GitHub notifications bot (Oct 03 2022 at 16:31):

akirilov-arm commented on issue #4997:

For __builtin___clear_cache() can we for now link it in from a C file as you suggested?

That is certainly an option, but my impression is that we are trying to move away from relying on C files as much as possible (refer to the refactoring that has been done in the wasmtime-fiber crate, for instance), and given that on Linux we are kind of OK at the moment, IMHO it might not be the best course of action.

As for creating a crate to be used by both cranelift-jit and wasmtime-jit, since the issue was that cranelift-jit should not depend on rustix, I am not sure how much sharing would be possible, though we do have the option of just implementing the libc variant (and avoiding rustix completely). That would also help with consolidating the locations that @cfallin needs to change in #4987.

Wasmtime GitHub notifications bot (Oct 04 2022 at 18:29):

afonso360 commented on issue #4997:

I've implemented a new crate jit-icache-coherency, that merges all of this code together so that we don't have to update stuff in multiple places, it also provides a common interface that both wasmtime-jit and cranelift-jit can use.

It does change our call's to SYNC_CORE membarriers, using the new method suggested by @akirilov-arm. We now execute SYNC_CORE and if it fails we retry after registering it. This should only happen once per process, so I don't think its too much overhead.

It also includes the changes made by @cfallin in #4987 since I thought it was just easier to do it here instead of having to deal with the ensuing merge conflicts.

I've tried to explain the whole cache coherency thing as best as I could in the documentation, hopefully I didn't miss anything!

Wasmtime GitHub notifications bot (Oct 05 2022 at 15:23):

akirilov-arm commented on issue #4997:

Forgot to post a general comment with my review - the overall code structure looks great and is definitely cleaner than what we had before; my remarks are mostly about improving the code comments. Also, as I have said before I am fine with just implementing the libc code path (and omitting the rustix one).

Wasmtime GitHub notifications bot (Oct 05 2022 at 17:01):

afonso360 commented on issue #4997:

I think I got all of them, let me know if I missed any of the changes that you requested. And thanks for you patience in dealing with this! I'm still very much learning about all of this stuff.

Also, as I have said before I am fine with just implementing the libc code path (and omitting the rustix one).

I'm okay with libc too, but would like an ack from someone on the wasmtime side if we want to do that.

Wasmtime GitHub notifications bot (Oct 11 2022 at 12:05):

afonso360 commented on issue #4997:

I had to add a flags arg to membarrier on libc since on my aarch64 machine it wasn't being properly added, somehow cranelift never triggered this. I've confirmed with strace and the membarrier is now being properly executed, and the register on fail trick is working.

[pid 615515] membarrier(MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE, 0) = -1 EPERM (Operation not permitted)
[pid 615515] membarrier(MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE, 0) = 0
[pid 615515] membarrier(MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE, 0) = 0

Wasmtime GitHub notifications bot (Oct 11 2022 at 12:06):

afonso360 edited a comment on issue #4997:

I had to add a flags arg to membarrier on libc since on my aarch64 machine it wasn't being properly added, somehow cranelift never triggered this. I've confirmed with strace and the membarrier is now being properly executed, and the register on fail trick is working.

[pid 615515] membarrier(MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE, 0) = -1 EPERM (Operation not permitted)
[pid 615515] membarrier(MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE, 0) = 0
[pid 615515] membarrier(MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE, 0) = 0

Is there a way to test the GLOBAL path on modern kernels?

Wasmtime GitHub notifications bot (Oct 11 2022 at 12:06):

afonso360 edited a comment on issue #4997:

I had to add a flags arg to membarrier on libc since on my aarch64 machine it wasn't being properly added, somehow cranelift never triggered this. I've confirmed with strace and the membarrier is now being properly executed, and the register on fail trick is working.

[pid 615515] membarrier(MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE, 0) = -1 EPERM (Operation not permitted)
[pid 615515] membarrier(MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE, 0) = 0
[pid 615515] membarrier(MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE, 0) = 0

Is there a way to test the GLOBAL path on modern kernels? i.e. fail MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE with EINVAL?

Wasmtime GitHub notifications bot (Oct 11 2022 at 12:07):

afonso360 edited a comment on issue #4997:

I had to add a flags arg to membarrier on libc since on my aarch64 machine it wasn't being properly added, somehow cranelift never triggered this. I've confirmed with strace and the membarrier is now being properly executed, and the register on fail trick is working.

[pid 615515] membarrier(MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE, 0) = -1 EPERM (Operation not permitted)
[pid 615515] membarrier(MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE, 0) = 0
[pid 615515] membarrier(MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE, 0) = 0

Is there a way to test the GLOBAL path on modern kernels? i.e. fail MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE with EINVAL?

Wasmtime GitHub notifications bot (Oct 11 2022 at 12:45):

afonso360 commented on issue #4997:

Right, I just wanted to check if there was an easy way to add a test that forces us to use GLOBAL so that we at least have some CI coverage on that branch. But I tested that manually and it worked, so I guess that's ok.

Wasmtime GitHub notifications bot (Oct 12 2022 at 16:07):

afonso360 commented on issue #4997:

Hey, @alexcrichton @sunfishcode we discussed this in the cranelift meeting today, and @cfallin mentioned that you might be interested in this PR as (in its current form) changes wasmtime to use libc on this particular membarrier call.

As a summary: We got to this situation when I merged this particular piece of code from wasmtime-jit and cranelift-jit, both those crates have to do the exact same thing when it comes to icache mainetenance, after merging we had two backends which do exactly the same thing, and to avoid having duplicated code we need to decide on one.

wasmtime already has libc on its dependency tree, but cranelift does not have rustix yet, so to avoid adding more stuff to the compilation path, I decided to keep libc.

I'd like to know if this is okay with you guys.

Wasmtime GitHub notifications bot (Oct 12 2022 at 16:08):

afonso360 edited a comment on issue #4997:

Hey, @alexcrichton @sunfishcode we discussed this in the cranelift meeting today, and @cfallin mentioned that you might be interested in this PR as (in its current form) it changes wasmtime to use libc on this particular membarrier call.

As a summary: We got to this situation when I merged this particular piece of code from wasmtime-jit and cranelift-jit, both those crates have to do the exact same thing when it comes to icache mainetenance, after merging we had two backends which do exactly the same thing, and to avoid having duplicated code we need to decide on one.

wasmtime already has libc on its dependency tree, but cranelift does not have rustix yet, so to avoid adding more stuff to the compilation path, I decided to keep libc.

I'd like to know if this is okay with you guys.

Wasmtime GitHub notifications bot (Oct 12 2022 at 16:08):

afonso360 edited a comment on issue #4997:

Hey, @alexcrichton @sunfishcode we discussed this in the cranelift meeting today, and @cfallin mentioned that you might be interested in this PR as (in its current form) it changes wasmtime to use libc on this particular membarrier call.

As a summary: We got to this situation when I merged this particular piece of code from wasmtime-jit and cranelift-jit, both of those crates have to do the exact same thing when it comes to icache mainetenance, after merging we had two backends which do exactly the same thing, and to avoid having duplicated code we need to decide on one.

wasmtime already has libc on its dependency tree, but cranelift does not have rustix yet, so to avoid adding more stuff to the compilation path, I decided to keep libc.

I'd like to know if this is okay with you guys.

Wasmtime GitHub notifications bot (Oct 12 2022 at 16:08):

sunfishcode commented on issue #4997:

Yes, that makes sense!

Wasmtime GitHub notifications bot (Oct 12 2022 at 16:08):

afonso360 edited a comment on issue #4997:

Hey, @alexcrichton @sunfishcode we discussed this in the cranelift meeting today, and @cfallin mentioned that you might be interested in this PR as (in its current form) it changes wasmtime to use libc on this particular membarrier call.

As a summary: We got to this situation when I merged this particular piece of code from wasmtime-jit and cranelift-jit, both of those crates have to do the exact same thing when it comes to icache maintenance. After merging we had two backends which do exactly the same thing, and to avoid having duplicated code we need to decide on one.

wasmtime already has libc on its dependency tree, but cranelift does not have rustix yet, so to avoid adding more stuff to the compilation path, I decided to keep libc.

I'd like to know if this is okay with you guys.

Wasmtime GitHub notifications bot (Oct 12 2022 at 16:22):

alexcrichton commented on issue #4997:

Indeed seems reasonable to me!

Last updated: Nov 22 2024 at 16:03 UTC