fix(virtio/pci): map BARs only once - #2572
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Benchmark Results
Details
| Benchmark | Current: f4bf447 | Previous: 2e23902 | Performance Ratio |
|---|---|---|---|
| startup_benchmark Build Time | 82.71 s |
80.34 s |
1.03 ❗ |
| startup_benchmark File Size | 0.79 MB |
0.80 MB |
1.00 ❗ |
| Startup Time - 1 core | 0.75 s (±0.02 s) |
0.75 s (±0.02 s) |
1.00 |
| Startup Time - 2 cores | 0.76 s (±0.02 s) |
0.74 s (±0.02 s) |
1.03 |
| Startup Time - 4 cores | 0.77 s (±0.03 s) |
0.74 s (±0.02 s) |
1.04 |
| multithreaded_benchmark Build Time | 85.33 s |
82.11 s |
1.04 ❗ |
| multithreaded_benchmark File Size | 0.90 MB |
0.86 MB |
1.05 ❗ |
| Multithreaded Pi Efficiency - 2 Threads | 85.71 % (±7.07 %) |
85.89 % (±6.61 %) |
1.00 |
| Multithreaded Pi Efficiency - 4 Threads | 43.52 % (±2.12 %) |
43.43 % (±2.56 %) |
1.00 |
| Multithreaded Pi Efficiency - 8 Threads | 25.07 % (±1.71 %) |
25.76 % (±1.53 %) |
0.97 |
| micro_benchmarks Build Time | 85.08 s |
80.40 s |
1.06 ❗ |
| micro_benchmarks File Size | 0.90 MB |
0.86 MB |
1.05 ❗ |
| Scheduling time - 1 thread | 66.56 ticks (±3.66 ticks) |
62.65 ticks (±4.06 ticks) |
1.06 |
| Scheduling time - 2 threads | 35.76 ticks (±3.30 ticks) |
34.08 ticks (±4.10 ticks) |
1.05 |
| Micro - Time for syscall (getpid) | 4.14 ticks (±0.59 ticks) |
3.45 ticks (±0.58 ticks) |
1.20 |
| Memcpy speed - (built_in) block size 4096 | 82675.68 MByte/s (±57144.17 MByte/s) |
82448.38 MByte/s (±56997.13 MByte/s) |
1.00 |
| Memcpy speed - (built_in) block size 1048576 | 30269.24 MByte/s (±24475.78 MByte/s) |
30585.98 MByte/s (±24707.84 MByte/s) |
0.99 |
| Memcpy speed - (built_in) block size 16777216 | 26463.51 MByte/s (±21820.32 MByte/s) |
26340.06 MByte/s (±21720.96 MByte/s) |
1.00 |
| Memset speed - (built_in) block size 4096 | 82690.56 MByte/s (±57155.46 MByte/s) |
82292.76 MByte/s (±56891.50 MByte/s) |
1.00 |
| Memset speed - (built_in) block size 1048576 | 31020.09 MByte/s (±24914.08 MByte/s) |
31323.85 MByte/s (±25145.86 MByte/s) |
0.99 |
| Memset speed - (built_in) block size 16777216 | 27233.65 MByte/s (±22312.23 MByte/s) |
27104.68 MByte/s (±22209.94 MByte/s) |
1.00 |
| Memcpy speed - (rust) block size 4096 | 75402.71 MByte/s (±52376.81 MByte/s) |
74097.96 MByte/s (±51811.44 MByte/s) |
1.02 |
| Memcpy speed - (rust) block size 1048576 | 30319.22 MByte/s (±24519.97 MByte/s) |
30361.60 MByte/s (±24602.37 MByte/s) |
1.00 |
| Memcpy speed - (rust) block size 16777216 | 27337.15 MByte/s (±22570.80 MByte/s) |
27625.34 MByte/s (±22806.88 MByte/s) |
0.99 |
| Memset speed - (rust) block size 4096 | 75825.54 MByte/s (±52675.82 MByte/s) |
74373.47 MByte/s (±51976.48 MByte/s) |
1.02 |
| Memset speed - (rust) block size 1048576 | 31062.45 MByte/s (±24951.21 MByte/s) |
31110.89 MByte/s (±25033.24 MByte/s) |
1.00 |
| Memset speed - (rust) block size 16777216 | 28077.95 MByte/s (±23015.34 MByte/s) |
28386.93 MByte/s (±23265.03 MByte/s) |
0.99 |
| alloc_benchmarks Build Time | 78.12 s |
74.76 s |
1.04 ❗ |
| alloc_benchmarks File Size | 0.87 MB |
0.87 MB |
1.00 ❗ |
| Allocations - Allocation success | 91.31 % |
91.31 % |
1 |
| Allocations - Deallocation success | 100.00 % |
100.00 % |
1 |
| Allocations - Pre-fail Allocations | 61.44 % |
61.44 % |
1 |
| Allocations - Average Allocation time | 4191.09 Ticks (±131.70 Ticks) |
5860.58 Ticks (±98.43 Ticks) |
0.72 ❗ |
| Allocations - Average Allocation time (no fail) | 4966.01 Ticks (±133.76 Ticks) |
6554.81 Ticks (±92.86 Ticks) |
0.76 ❗ |
| Allocations - Average Deallocation time | 859.81 Ticks (±135.24 Ticks) |
1805.01 Ticks (±250.35 Ticks) |
0.48 ❗ |
| mutex_benchmark Build Time | 81.62 s |
79.82 s |
1.02 ❗ |
| mutex_benchmark File Size | 0.90 MB |
0.86 MB |
1.05 ❗ |
| Mutex Stress Test Average Time per Iteration - 1 Threads | 12.22 ns (±0.88 ns) |
12.10 ns (±0.41 ns) |
1.01 |
| Mutex Stress Test Average Time per Iteration - 2 Threads | 41.08 ns (±2.15 ns) |
40.26 ns (±1.68 ns) |
1.02 |
This comment was automatically generated by workflow using github-action-benchmark.
|
@cagatay-y @mkroening @jounathaen Sorry, this PR was my test if an LLM is able to support reviews. I am not sure, if this helps... |
I replied to the comments in case it is useful for the evaluation of the test :) |
79de297 to
3c7afcb
Compare
6ae9bfe to
e159351
Compare
There was a problem hiding this comment.
The CI failure does not seem to be spurious. I am wondering what goes wrong exactly. Maybe we need to go back to lazy cells after all. What kind of issue does this PR resolve? Is there a bug, or is this just to save resources?
By the way: where does the number six come from?
The issue is that if a base address is 64 bits, it occupies two slots, so the slot after it is invalid as a BAR by itself. When we blindly try to map the slot that is occupied by the second part of the previous BAR, it returns values that cannot be interpreted as a BAR. There does not seem to be a way to tell that it is not a BAR by itself without reading the previous one and skipping the current one accordingly.
We can either switch back to lazy cells or solve the issue by iterating over the slot numbers and skipping one if the previous slot was a 64-bit one. That would require changing the current
I did not encounter an issue caused by the current behavior but mapping the same address twice seemed like an accident to me.
Per PCIe spec. sec. 7.5.1.2.1 "A Type 0 Configuration Space Header has six DWORD locations allocated for Base Address registers starting at offset 10h in Configuration Space." I should introduce a constant with a descriptive name for that. By the way, do you think it would make sense to have the virtio-console test later amongst the virtio tests? For the common virtio issues it could be nice to have the tests where the console output is not affected earlier, so that the CI output is more helpful. |
It is possible for multiple capabilities to be accessed through the same base address, so create the mappings for the BAR slots at the beginning and use saved virtual addresses.
e159351 to
f4bf447
Compare
| .into_iter() | ||
| .find_map(|bar| { | ||
| bar.map(|(addr, _size)| unsafe { | ||
| VolatileRef::new(NonNull::new(addr.as_mut_ptr()).unwrap()) |
There was a problem hiding this comment.
The behaviour is slightly different:
- We now accept 64-bit BARs.
32-bit BAR choice was made in 7246792, maybe @Gelbpunkt can chime in on if 64-bit BARs are also okay. - We use the first BAR that we can map.
The driver originally used the first IO BAR it could find and was changed to use the last one with 75013ce#diff-81e90c26a83946cda84acea9ea30c793496f20651faf921f080cba1347c3c2e6R438-R444. There is not information in the commit message to indicate if that was intentional. We can reverse the iterator to achieve the same effect, if necessary.
Let's go lazy, then. ^^
I did originally put them to the non-virtio |
I've realized that rather than returning the BAR type information to the caller, we can move the array construction into the mapping function. The latest commit does that. Should I revert to lazy cells or is the current version good? I've also left a review comment about a change in the RTL8139 driver's behavior if we end up going with the eager approach. |
3 participants
It is possible for multiple capabilities to be accessed through the same base address, so only create a new mapping if one does not already exist for the BAR slot.