n-acd: runtime eBPF support detection #11
Conversation
jvaclav-rh
force-pushed
the
runtime-ebpf
branch
from
fc0c9bd to
5f157b3
Compare
|
Hi, thanks for the suggestion! Can you explain who would use it? Distros know their kernel versions, so they can easily require a kernel with eBPF. Who would build n-acd without knowing their kernel version? |
It is not so much about whether the kernel supports eBPF but also about if the application has capabilities to use it at runtime. For example it would be very useful for NetworkManager if it is e.g. running in a container. edit: I also just noticed an older pull request (#9) that had some discussion related to this. |
|
@dvdhrm Is there anything I can explain or amend to help move this forward? |
See also: #9 (comment) . If n-acd runs as root in a network namespace where the current unprivileged user is mapped as root, eBPF doesn't work unless |
| @@ -22,6 +22,4 @@ dep_crbtree = sub_crbtree.get_variable('libcrbtree_dep') | |||
| dep_csiphash = sub_csiphash.get_variable('libcsiphash_dep') | |||
| dep_cstdaux = sub_cstdaux.get_variable('libcstdaux_dep') | |||
|
|
|||
| use_ebpf = get_option('ebpf') | |||
There was a problem hiding this comment.
After removing the option, README.md must be updated.
src/n-acd-probe.c
Outdated
| */ | ||
| if (n_acd_probe_is_unique(probe)) { | ||
| if (probe->acd->fd_bpf_map != -1 && n_acd_probe_is_unique(probe)) { |
There was a problem hiding this comment.
This probably should be changed as suggested in #9 (comment)
There was a problem hiding this comment.
I am not sure I understand the original comment -- it talks about that if we manage to successfully create the BPF map, all of the IPs would be instantly added. But if we move this ... != 1 condition up, and return early, wouldn't that mean the IP is silently dropped, not added to the RBtree and thus not included in the later created bpf map? Would that be the desired behavior?
There was a problem hiding this comment.
I think the suggestion was more to remove this check and let n_acd_bpf_map_add() return success when the map is negative. In this way, we keep tracking the probe->acd->n_bpf_map value correctly; then, if the map can be created later, it will be re-initialized with the right size in n_acd_ensure_bpf_map_space() . @dvdhrm ?
jvaclav-rh
force-pushed
the
runtime-ebpf
branch
3 times, most recently
from
fe48f3e to
f8a8384
Compare
|
@dvdhrm I am going to try and continue working on this, but before doing anything I wanted to ask which approach makes more sense to you. If we support switching to eBPF at runtime, we have two options:
What do you think is better/makes more sense? |
|
I would prefer to create the map once, and only ever re-create it if it already exists. Hence, we never end up using ebpf later on, if it failed initially. However, I still think it is wise to keep any counters up to date, even if we do not end up using them. It is much less confusing to see correct counters in debug-dumps, than random values. And it makes it a lot less likely to trip if we use those counters for other purposes later on. |
Currently, eBPF support is toggled by a compile-time flag based on whether we expect the system to support it or not. Make it so that n-acd always attempts to use eBPF, and gracefully handles failure if it isn't supported (e.g. due to lacking capabilities).
Currently, eBPF support is toggled by a compile-time flag based on whether we expect the system to support it or not.
This PR makes it so that n-acd always attempts to use eBPF, and gracefully handles failure if it isn't supported (e.g. due to lacking capabilities).
This change is desirable for programs that would like to use n-acd with eBPF as often as possible, but expect that eBPF setup may fail (and are OK with falling back).