Skip to content

Conversation

@mkroening
Copy link
Member

Although it should usually be avoided, the current system permits blocking on async tasks from an interrupt handler. This restores this behavior after it got broken in b735657.

I am not very sure about the logic, so please review very thoroughly. :)

Extracted from #1910.

@mkroening mkroening self-assigned this Sep 2, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmark Results

Benchmark Current: fc0c5c3 Previous: 083dacf Performance Ratio
startup_benchmark Build Time 123.70 s 124.04 s 1.00
startup_benchmark File Size 0.91 MB 0.91 MB 1.00
Startup Time - 1 core 0.97 s (±0.02 s) 0.93 s (±0.02 s) 1.03
Startup Time - 2 cores 0.94 s (±0.02 s) 0.93 s (±0.02 s) 1.00
Startup Time - 4 cores 0.95 s (±0.03 s) 0.95 s (±0.02 s) 1.01
multithreaded_benchmark Build Time 127.24 s 122.53 s 1.04
multithreaded_benchmark File Size 0.97 MB 0.97 MB 1.00
Multithreaded Pi Efficiency - 2 Threads 3.15 % (±15.13 %) 2.99 % (±14.36 %) 1.05
Multithreaded Pi Efficiency - 4 Threads 1.62 % (±7.78 %) 1.79 % (±8.58 %) 0.91
Multithreaded Pi Efficiency - 8 Threads 0.88 % (±4.21 %) 0.86 % (±4.12 %) 1.02
micro_benchmarks Build Time 125.56 s 120.73 s 1.04
micro_benchmarks File Size 0.97 MB 0.97 MB 1.00
Scheduling time - 1 thread 2.47 ticks (±11.85 ticks) 2.49 ticks (±11.93 ticks) 0.99
Scheduling time - 2 threads 1.87 ticks (±8.96 ticks) 1.36 ticks (±6.51 ticks) 1.38
Micro - Time for syscall (getpid) 0.14 ticks (±0.66 ticks) 0.13 ticks (±0.60 ticks) 1.10
Memcpy speed - (built_in) block size 4096 1531.86 MByte/s (±7352.94 MByte/s) 1250.00 MByte/s (±6000.00 MByte/s) 1.23
Memcpy speed - (built_in) block size 1048576 747.64 MByte/s (±3588.65 MByte/s) 844.45 MByte/s (±4053.37 MByte/s) 0.89
Memcpy speed - (built_in) block size 16777216 212.80 MByte/s (±1021.42 MByte/s) 220.90 MByte/s (±1060.32 MByte/s) 0.96
Memset speed - (built_in) block size 4096 1290.32 MByte/s (±6193.55 MByte/s) 1818.18 MByte/s (±8727.27 MByte/s) 0.71
Memset speed - (built_in) block size 1048576 958.70 MByte/s (±4601.78 MByte/s) 995.72 MByte/s (±4779.45 MByte/s) 0.96
Memset speed - (built_in) block size 16777216 902.18 MByte/s (±4330.44 MByte/s) 918.43 MByte/s (±4408.46 MByte/s) 0.98
Memcpy speed - (rust) block size 4096 1379.31 MByte/s (±6620.69 MByte/s) 1176.47 MByte/s (±5647.06 MByte/s) 1.17
Memcpy speed - (rust) block size 1048576 676.88 MByte/s (±3249.01 MByte/s) 768.98 MByte/s (±3691.10 MByte/s) 0.88
Memcpy speed - (rust) block size 16777216 215.18 MByte/s (±1032.87 MByte/s) 218.60 MByte/s (±1049.28 MByte/s) 0.98
Memset speed - (rust) block size 4096 1395.35 MByte/s (±6697.67 MByte/s) 1538.46 MByte/s (±7384.62 MByte/s) 0.91
Memset speed - (rust) block size 1048576 981.98 MByte/s (±4713.51 MByte/s) 1011.15 MByte/s (±4853.51 MByte/s) 0.97
Memset speed - (rust) block size 16777216 905.62 MByte/s (±4346.96 MByte/s) 913.73 MByte/s (±4385.90 MByte/s) 0.99
alloc_benchmarks Build Time 119.64 s 117.69 s 1.02
alloc_benchmarks File Size 0.98 MB 0.98 MB 1.00
Allocations - Allocation success 2.00 % (±13.86 %) 2.00 % (±13.86 %) 1
Allocations - Deallocation success 1.40 % (±9.72 %) 1.40 % (±9.67 %) 1.01
Allocations - Pre-fail Allocations 2.00 % (±13.86 %) 2.00 % (±13.86 %) 1
Allocations - Average Allocation time 231.94 Ticks (±1607.27 Ticks) 232.75 Ticks (±1612.89 Ticks) 1.00
Allocations - Average Allocation time (no fail) 231.94 Ticks (±1607.27 Ticks) 232.75 Ticks (±1612.89 Ticks) 1.00
Allocations - Average Deallocation time 15.66 Ticks (±108.49 Ticks) 15.72 Ticks (±108.93 Ticks) 1.00
mutex_benchmark Build Time 118.87 s 120.56 s 0.99
mutex_benchmark File Size 0.97 MB 0.97 MB 1.00
Mutex Stress Test Average Time per Iteration - 1 Threads 0.28 ns (±1.94 ns) 0.26 ns (±1.80 ns) 1.08
Mutex Stress Test Average Time per Iteration - 2 Threads 0.30 ns (±2.08 ns) 0.30 ns (±2.08 ns) 1

This comment was automatically generated by workflow using github-action-benchmark.

device.set_polling_mode(true);
}
crate::executor::network::NIC
.lock()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this lock not an issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and that is also the part I am unsure about. I have pushed an alternative. 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not so sure what to do about this one. I think the way it is now is definitely wrong, but I'm unsure what the proper way of working around this would be. We can probably just avoid re-enabling the polling mode (i.e. drop this entirely) because it currently isn't done anywhere, so it can't be worse than it already is?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you think that this code is wrong?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unwrapping the try_lock will panic if the lock is being held by a different core, which may be extremely unlikely, but could in theory happen

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, your are talking from line 243. Yes, the unwrapping should be removed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this, then? This might be bad since it opens up the possibility of a deadlock in the unlikely case of blocking in an interrupt handler, but that's better than panicking in regular usage, right?

@mkroening mkroening force-pushed the block_on-deadlock branch 3 times, most recently from 459f3de to c0acf00 Compare September 2, 2025 15:52
@mkroening mkroening added this pull request to the merge queue Sep 9, 2025
Merged via the queue into main with commit 56a2a8a Sep 9, 2025
15 checks passed
@mkroening mkroening deleted the block_on-deadlock branch October 16, 2025 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants