smitebot: add bench command to measure Nyx execution speed - #160
smitebot: add bench command to measure Nyx execution speed#160erickcestari wants to merge 3 commits into
Conversation
5170540 to
528ffd7
Compare
Chand-ra
left a comment
There was a problem hiding this comment.
Looks like a great addition overall. Following are some improvement suggestions.
15cf725 to
c2c557f
Compare
|
Thanks for the review @Chand-ra ! I've added the coverage metric, which is a nice feature to have. Specially when we start using the |
Ashish-Kumar-Dash
left a comment
There was a problem hiding this comment.
I had a few thoughts/nits, otherwise its a solid PR! The only real concern I have is the order in which we merge status, bench and print-ir since one of us will have to rebase, but I think it'd be much better if bench,print-ir could get merged and then we land status. I don't think it will be an issue on your part, since I'd have to do the rebase then :)
| smitebot bench campaign.toml --input testcase.bin --iterations 5000 | ||
| ``` | ||
|
|
||
| - `--input`: Input file to execute repeatedly. Defaults to a single `0x00` byte. |
There was a problem hiding this comment.
I think --input should be required -- it should be rare that anyone wants to benchmark an empty program.
There was a problem hiding this comment.
It's useful for quickly checking performance improvements. I've used it to quickly test JVM optimization because the executor checks if the target is live, which does a ping-pong.
There was a problem hiding this comment.
I think it is worth mentioning in the documentation then: the default input can be used to quickly check performance improvements due to factors like target optimization.
| /// Start of the `result` section within the buffer. | ||
| const RESULT: usize = 896; | ||
| /// `result.dirty_pages` (`u32`): pages restored for the last exec. | ||
| pub const DIRTY_PAGES: usize = RESULT + 16; | ||
| /// `result.runtime_usec` (`u32`): microsecond part of guest payload runtime. | ||
| pub const RUNTIME_USEC: usize = RESULT + 28; | ||
| /// `result.runtime_sec` (`u32`): second part of guest payload runtime. | ||
| pub const RUNTIME_SEC: usize = RESULT + 32; |
There was a problem hiding this comment.
These offsets are all correct AFAICT. But it seems fragile -- it would be nice if Nyx provided a clearer interface.
d6d5659 to
931a8b2
Compare
|
Thanks @morehouse and @Ashish-Kumar-Dash for the review. |
Chand-ra
left a comment
There was a problem hiding this comment.
GitHub started acting up so I couldn't use the "suggest change" functionality🤦. Looks mostly good to me, following are a couple of suggestions.
| smitebot bench campaign.toml --input testcase.bin --iterations 5000 | ||
| ``` | ||
|
|
||
| - `--input`: Input file to execute repeatedly. Defaults to a single `0x00` byte. |
There was a problem hiding this comment.
I think it is worth mentioning in the documentation then: the default input can be used to quickly check performance improvements due to factors like target optimization.
bfb96ea to
470fce8
Compare
Chand-ra
left a comment
There was a problem hiding this comment.
Took a quick look and verified that all the pointers from the previous feedback were addressed, looks good to me now!
|
@erickcestari I pushed some recent commits, you might have to rebase |
c80f707 to
76314ac
Compare
Thanks! I've rebased it now. |
| /// Maximum input buffer size in bytes for the Nyx VM. Defaults to the input | ||
| /// size rounded up to a 4 KiB page; pass 1048576 to match AFL++'s 1 MiB buffer. | ||
| #[arg(long)] | ||
| max_input_size: Option<u32>, |
There was a problem hiding this comment.
Is there ever a reason to use this flag now that we automatically calculate the buffer size?
There was a problem hiding this comment.
Actually I'll remove the adaptive buffer size, since when running with AFL++ the value used is always the 1MiB MAX_FILE. So I believe it makes more sense to bench in the same way AFL++ runs. Also I'll keep an option to change the max buffer size to test if changing it makes any difference on performance.
76314ac to
01a9562
Compare
Move setup_nyx out of the start command and into utils so other commands can reuse it to prepare the Nyx sharedir. Pure refactor, no behavior change.
The goal is to tell whether a change concretely improved the target's throughput or not: run bench before and after and compare execs/sec. It runs a single input through the target's Nyx VM many times, timing the snapshot-restore-plus-target-run loop. Each exec is one snapshot restore plus one target run, so the numbers isolate VM/snapshot and target speed from AFL++'s mutation and scheduling overhead. Like start, it builds the image and sets up the sharedir by default (pass --no-build to reuse an existing one), and loads libnyx.so at runtime the same way afl-fuzz does. --repeat averages over several fresh VM boots so a real change stands out from boot/snapshot variance, and --timeout bounds a single execution (default 2s). Adds FFI bindings to libnyx (loaded via dlopen) and a small stats module for latency percentiles and mean/stddev aggregation.
01a9562 to
28fe6ec
Compare
|
Thanks @morehouse. It was an insightful review. It's ready for another one. |
| /// # Errors | ||
| /// Returns an error if `cpu` is out of range or is not an online CPU this | ||
| /// process is allowed to run on. | ||
| pub fn pin_to_cpu(cpu: usize) -> Result<(), String> { |
There was a problem hiding this comment.
This should not be added in the commit labeled "pure refactor". It also looks like it's introduced before we even add the needed libc dependency.
| // `size_of::<cpu_set_t>()` bytes from the pointer. A pid of 0 means the | ||
| // calling thread. | ||
| let failed = unsafe { | ||
| let mut set: libc::cpu_set_t = std::mem::zeroed(); |
There was a problem hiding this comment.
I think we should probably use CPU_ZERO rather than assuming a manual zero-init does the same.
| true | ||
| } | ||
|
|
||
| /// Pins the calling process, and every process it later spawns, to `cpu`. |
There was a problem hiding this comment.
| /// Pins the calling process, and every process it later spawns, to `cpu`. | |
| /// Pins the calling thread, and every process it later spawns, to `cpu`. |
| // Affinity is per-thread, and this runs on the test's own thread, so | ||
| // pinning here leaves the rest of the suite alone. Pin to a CPU the | ||
| // process is already allowed on so a restricted cpuset does not fail it. | ||
| let mut set: libc::cpu_set_t = unsafe { std::mem::zeroed() }; |
There was a problem hiding this comment.
Should also be using CPU_ZERO in this test.
| /// asserts `shared_payload_buffer_size % x86_64_PAGE_SIZE == 0` | ||
| /// (`nyx_mode/QEMU-Nyx/nyx/memory_access.c`), so any buffer size we pass must be | ||
| /// page-rounded or the VM aborts. AFL++ never trips this because its buffer is a | ||
| /// page-multiple 1 MiB; a `--max-input-size` that is not is rejected up front. |
There was a problem hiding this comment.
Not sure what the last part is about.
| /// page-multiple 1 MiB; a `--max-input-size` that is not is rejected up front. | |
| /// page-multiple 1 MiB. |
| /// Returns `false` (rather than erroring) if not: benchmarking can still run, | ||
| /// only the target/overhead split is unreliable. |
There was a problem hiding this comment.
This seems like it would be confusing to the user. If we're reading garbage data, we should fail loudly rather than expecting the user to figure out that the data is garbage.
| /// Defaults to AFL++'s 1 MiB, the buffer a real campaign runs with. Lower it | ||
| /// to measure what a smaller buffer buys: the guest allocates the whole | ||
| /// buffer on every execution and the snapshot resets every page of it | ||
| /// dirtied, so buffer size shows up directly in the Nyx overhead and dirty | ||
| /// pages per exec. |
There was a problem hiding this comment.
I don't think this logic checks out. Assume the input size is X pages and the buffer size is Y > X pages. Then regardless of what Y is, only X pages ever get dirtied and need to be reset.
Have you actually observed differences in performance with varying buffer sizes?
| // A zero timeout reaches Nyx as "no timeout" rather than "expire | ||
| // immediately", which silently turns off the timing the benchmark exists | ||
| // to measure. | ||
| if args.timeout_secs == 0 { | ||
| log::error!("--timeout must be at least 1 second; Nyx reads zero as no timeout"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This is confusing to the user, who may very well want "no timeout" (a.k.a. super high timeout). The actual issue is that we lose benchmark timings .
| // A zero timeout reaches Nyx as "no timeout" rather than "expire | |
| // immediately", which silently turns off the timing the benchmark exists | |
| // to measure. | |
| if args.timeout_secs == 0 { | |
| log::error!("--timeout must be at least 1 second; Nyx reads zero as no timeout"); | |
| return false; | |
| } | |
| // A zero timeout causes Nyx to silently turn off the timing the | |
| // benchmark exists to measure. | |
| if args.timeout_secs == 0 { | |
| log::error!("Nyx requires --timeout to be at least 1 second"); | |
| return false; | |
| } |
| // Pin before booting: libnyx spawns QEMU as a child, so the VM inherits | ||
| // this affinity mask. Doing it here also pins the timing loop itself. | ||
| let cpu = args.cpu.unwrap_or(args.worker_id as usize); | ||
| if let Err(e) = pin_to_cpu(cpu) { | ||
| log::error!("{e}"); | ||
| return false; | ||
| } | ||
| log::info!("pinned to CPU {cpu}"); |
There was a problem hiding this comment.
Perhaps we should pin closer to where we actually start benchmarking. Currently we still load and validate a whole bunch of stuff after pinning.
| steady-state (snapshot restore + target run): | ||
| execs/sec: 1234.5 | ||
| exec time: 810.20 ms | ||
| latency: min 0.7 µs mean 0.8 µs median 0.8 µs p99 1.2 µs max 3.4 ms | ||
| input execution: mean 0.5 µs median 0.5 µs (guest runtime) | ||
| nyx overhead: mean 0.3 µs median 0.3 µs (restore + reset + ipc; 42 dirty pages/exec) | ||
| failed iterations: 0 / 1000 | ||
| coverage determinism: 98.4% stable (12 / 750 edges fluctuated) |
There was a problem hiding this comment.
These stats are inconsistent. exec time 810 ms implies 810 us latency per iteration.
Let's regenerate with data from a real run.
The goal is to tell whether a change concretely improved the target's throughput or not: run bench before and after and compare execs/sec.
It runs a single input through the target's Nyx VM many times, timing the snapshot-restore-plus-target-run loop. Each exec is one snapshot restore plus one target run, so the numbers isolate VM/snapshot and target speed from AFL++'s mutation and scheduling overhead.
Like start, it builds the image and sets up the sharedir by default (pass --no-build to reuse an existing one), and loads libnyx.so at runtime the same way afl-fuzz does. --repeat averages over several fresh VM boots so a real change stands out from boot/snapshot variance, and --timeout bounds a single execution (default 2s).