Adapts to lower kernels, no BPF_STATS_RUN_TIME, but has "/proc/sys/kernel/bpf_stats_enabled" #21
Conversation
src/main.rs
Outdated
| @@ -97,6 +103,10 @@ fn main() -> Result<()> { | |||
| app.start_background_thread(); | |||
| let res = run_draw_loop(&mut terminal, app); | |||
|
|
|||
| if fd < 0 { | |||
| fs::write(BPF_STATS_ENABLED, b"0")?; | |||
There was a problem hiding this comment.
If the app panics, this line is not guaranteed to run and stats will remain enabled. This is not an issue with the _owned_fd approach since it will auto close when the app exits. We need another approach that guarantees we disable stats if the app panics.
src/main.rs
Outdated
| @@ -78,11 +81,14 @@ fn main() -> Result<()> { | |||
| // Try to enable BPF stats | |||
| let fd = unsafe { bpf_enable_stats(libbpf_sys::BPF_STATS_RUN_TIME) }; | |||
| if fd < 0 { | |||
| return Err(anyhow!("Failed to enable BPF stats")); | |||
| if let Err(err) = fs::write(BPF_STATS_ENABLED, b"1") { | |||
There was a problem hiding this comment.
My preferred approach would be to find a way to detect that the Kernel supports the BPF_STATS_RUN_TIME bpf syscall, like checking the Kernel version.
Falling back to the BPF_STATS_ENABLED path if the syscall fails feels brittle. The syscall may have failed for a different reason and the user should know. We should only do the fallback way if we know the user is on an older kernel.
There was a problem hiding this comment.
@zf1575192187 I pushed a change to this branch to only enable stats via procfs if the Kernel version is below 5.8 6ef956d
There was a problem hiding this comment.
Could you pull that change into your branch and push it to this PR?
There was a problem hiding this comment.
Could you pull that change into your branch and push it to this PR?
OK, and do you have any good suggestions for app panics?
There was a problem hiding this comment.
I don't right now. But we can address that in a follow-up PR since there is already a lot of changes here. It may require refactoring the main func.
The kernel version is lower than 5.8, there is no unsupported BPF_STATS_RUN_TIME, set "/proc/sys/kernel/bpf_stats_enabled". Signed-off-by: Feng Zhou <[email protected]>
Adapts to lower kernels, no BPF_STATS_RUN_TIME, but has "/proc/sys/kernel/bpf_stats_enabled"