Stream: git-wasmtime

Topic: wasmtime / PR #5078 Perform I-Cache Maintenance on RISC-V.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 19 2022 at 22:26):

yuyang-ok opened PR #5078 from risc-fencei to main:

This is trying to fix https://github.com/bytecodealliance/wasmtime/issues/5033.
Thanks @afonso360, I think he is done all of the investigation,I didn't do a lot of work.
But I am still have question about the flag parameter.
look like SYS_RISCV_FLUSH_ICACHE_LOCAL and SYS_RISCV_FLUSH_ICACHE_LOCAL have the same value.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 19 2022 at 22:46):

yuyang-ok updated PR #5078 from risc-fencei to main.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 20 2022 at 08:07):

yuyang-ok updated PR #5078 from risc-fencei to main.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 20 2022 at 10:32):

yuyang-ok edited PR #5078 from risc-fencei to main:

This is trying to fix https://github.com/bytecodealliance/wasmtime/issues/5033.
Thanks @afonso360, I think he is done all of the investigation,I didn't do a lot of work.
But I am still have question about the flag parameter.
look like SYS_RISCV_FLUSH_ICACHE_ALL and SYS_RISCV_FLUSH_ICACHE_LOCAL have the same value.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 20 2022 at 11:26):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 20 2022 at 11:26):

bjorn3 created PR review comment:

num_cpus::get() does multiple syscalls and as such may be more expensive than the SYS_RISCV_FLUSH_ICACHE_ALL syscall it is meant to avoid.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 20 2022 at 11:30):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 20 2022 at 11:30):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 20 2022 at 11:30):

afonso360 created PR review comment:

Although it's not currently use we should really provide these values. These represent the range of addresses for which we should flush the cache (as far as I understand it). We can get these if we move this operation to the clear_cache function below which is a more appropriate place for it.

There we have the start address and the length which we can easily convert into a start and end address.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 20 2022 at 11:30):

afonso360 created PR review comment:

We might want to call this something along the lines of SYS_RISCV_FLUSH_ICACHE so that it provides a bit more context as to what this is doing.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 20 2022 at 11:30):

afonso360 created PR review comment:

I'm also a bit confused as to why the kernel decide to alias both of these settings, but it seems as it was just to represent the state of things at the time. I think we should be OK just referencing SYS_RISCV_FLUSH_ICACHE_ALL which is the operation that we want but adding a comment on the current situation.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 20 2022 at 11:30):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 20 2022 at 13:01):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 22 2022 at 05:21):

yuyang-ok submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 22 2022 at 05:21):

yuyang-ok created PR review comment:

Ok,My main consideration is that detected at runtime make it is more easy to use. use this library need no configuration.Maybe I will add a feature one-core. Because of multi-core is the most case.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2022 at 08:33):

yuyang-ok created PR review comment:

yeah.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2022 at 08:33):

yuyang-ok submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2022 at 10:16):

yuyang-ok submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2022 at 10:16):

yuyang-ok created PR review comment:

ok.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2022 at 10:16):

yuyang-ok updated PR #5078 from risc-fencei to main.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2022 at 05:55):

yuyang-ok updated PR #5078 from risc-fencei to main.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2022 at 06:03):

yuyang-ok updated PR #5078 from risc-fencei to main.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 28 2022 at 08:30):

afonso360 created PR review comment:

    riscv_flush_icache(ptr as u64, (ptr as u64) + (len as u64))?;

view this post on Zulip Wasmtime GitHub notifications bot (Oct 28 2022 at 08:30):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 28 2022 at 08:30):

afonso360 created PR review comment:

I don't think we need to do this here. With the way the API for this crate is set up, we should always have a clear_cache operation preceding this. So we are effectively doing the cache flushing twice!

view this post on Zulip Wasmtime GitHub notifications bot (Oct 28 2022 at 08:30):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2022 at 01:25):

yuyang-ok submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2022 at 01:25):

yuyang-ok created PR review comment:

ok.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2022 at 01:27):

yuyang-ok created PR review comment:

ok,thanks.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2022 at 01:27):

yuyang-ok submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2022 at 01:29):

yuyang-ok updated PR #5078 from risc-fencei to main.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 30 2022 at 22:51):

yuyang-ok has marked PR #5078 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2022 at 16:55):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2022 at 16:55):

cfallin created PR review comment:

Writing/grammar fix: let's reword this as

# Most modern CPUs are SMP (multicore). However, when only one core is present,
# some aspects of coherence are much cheaper. For example, RISC-V can use
# one instruction `fence.i` rather than a syscall that invokes all other cores.
# This feature enables such optimizations, but the resulting program will *only*
# be safe to run on one-core systems.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2022 at 16:55):

cfallin created PR review comment:

                        // The syscall isn't defined in `libc`, so we definfe the syscall number here.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2022 at 16:55):

cfallin created PR review comment:

Missing space before else

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2022 at 16:55):

cfallin created PR review comment:

return isn't necessary here, I think: the whole if is an expression that will return the last value in the block.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2022 at 16:55):

cfallin created PR review comment:

Missing newline after this line

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2022 at 16:55):

cfallin created PR review comment:

LIkewise for these two lines: return isn't needed in either case.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2022 at 16:55):

cfallin created PR review comment:

                    // Currently these parameters are not used, but they are still defined.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2022 at 16:55):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2022 at 17:10):

alexcrichton created PR review comment:

I may have missed some discussion on this, but how come this is a crate feature? It seems like this would be better detected by the crate dynamically since many folks compiling code aren't certain what the target system looks like.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2022 at 17:10):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2022 at 18:05):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2022 at 18:05):

afonso360 created PR review comment:

There a comment about detecting the number of cpu's at runtime: https://github.com/bytecodealliance/wasmtime/pull/5078#discussion_r1000502204

I'd prefer if we could call the vDSO implementation, since that would also do the exact same thing for non SMP kernels, and avoid the syscalls.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2022 at 18:05):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2022 at 18:25):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2022 at 18:25):

alexcrichton created PR review comment:

Could the result of num_cpus::get be stored in a static? That would incur syscalls on the first hit but none subsequently.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2022 at 18:26):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2022 at 02:09):

yuyang-ok submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2022 at 02:09):

yuyang-ok created PR review comment:

@alexcrichton That is a good idea save num_cpus::get in static.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2022 at 02:09):

yuyang-ok created PR review comment:

ok.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2022 at 02:09):

yuyang-ok submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2022 at 05:54):

yuyang-ok updated PR #5078 from risc-fencei to main.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2022 at 05:54):

yuyang-ok updated PR #5078 from risc-fencei to main.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2022 at 08:09):

yuyang-ok updated PR #5078 from risc-fencei to main.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2022 at 10:04):

yuyang-ok updated PR #5078 from risc-fencei to main.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2022 at 10:38):

bjorn3 created PR review comment:

I don't think num_cpus::get() is safe. Linux supports hotplugging entire cpus and enabling/disabling cpu cores at runtime. In addition cgroups can be used to limit and unlimit cpu usage. num_cpus considers a cgroup limit to 1 or less cpu worth of time as there being only a single core even if it can migrate to a different cpu.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2022 at 10:38):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2022 at 22:18):

yuyang-ok updated PR #5078 from risc-fencei to main.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2022 at 07:59):

yuyang-ok updated PR #5078 from risc-fencei to main.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2022 at 08:13):

yuyang-ok updated PR #5078 from risc-fencei to main.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2022 at 08:46):

yuyang-ok submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2022 at 08:46):

yuyang-ok created PR review comment:

@bjorn3 I have rollback to features implementation since not safe,safety come first.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2022 at 08:50):

yuyang-ok edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2022 at 08:51):

yuyang-ok edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 01 2023 at 06:40):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 02 2023 at 04:05):

yuyang-ok updated PR #5078 from risc-fencei to main.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 02 2023 at 23:23):

yuyang-ok closed without merge PR #5078.

Last updated: Nov 22 2024 at 16:03 UTC