Conversation
NoNickeD
force-pushed
the
add-bloom-filter-support
branch
from
937ed79 to
f079fc5
Compare
dylandreimerink
left a comment
dylandreimerink
left a comment
There was a problem hiding this comment.
Thanks for the PR! However, I have some changes to request.
I think the change to parse the MapExtra from ELF makes sense. The addition of the constructor does not.
All of the test logic added tests this constructor. I suggest you remove it, and instead extend TestLoadCollectionSpec to test parsing of map extra from ELF. You would do so my adding a new map definition to testdata/loader.c and then adding a new expected MapSpec.
Per review feedback, removing the NewBloomFilter constructor as it adds unnecessary API surface area. MapSpec fields are public and can be used directly to create bloom filter maps. The MapExtra field parsing from ELF is already implemented and working in the ELF loader, so bloom filters can be loaded from BPF programs. Fixes cilium#1856
Per review feedback, removing the NewBloomFilter constructor as it adds unnecessary API surface area. MapSpec fields are public and can be used directly to create bloom filter maps. The MapExtra field parsing from ELF is already implemented and working in the ELF loader, so bloom filters can be loaded from BPF programs. Fixes cilium#1856 Signed-off-by: nikos.nikolakakis <[email protected]>
NoNickeD
force-pushed
the
add-bloom-filter-support
branch
from
a847beb to
aff2768
Compare
|
Hello @dylandreimerink, Thanks for the feedback! I have removed the NewBloomFilter constructor and all associated tests as requested. The MapExtra parsing from ELF was already implemented in the existing code, so bloom filters can still be created by setting the MapSpec fields directly. The changes have been pushed and the PR is ready for another review. |
Yea, passing from MapSpec was already there, but this addition is needed to make ELF -> MapSpec work properly. Thank you for the changes. Please don't forget about extending the tests to cover this new behavior https://github.com/cilium/ebpf/blob/main/elf_reader_test.go#L48 + https://github.com/cilium/ebpf/blob/main/testdata/loader.c That way we can assert this stays working if we ever have to make modifications in the future. |
NoNickeD
force-pushed
the
add-bloom-filter-support
branch
from
7a34fb0 to
e3207a9
Compare
ti-mo
force-pushed
the
add-bloom-filter-support
branch
from
3a8f119 to
1c30f3e
Compare
ti-mo
changed the title
Bloom filter maps were introduced in Linux 5.16 and use the map_extra field to specify the number of hash functions (lower 4 bits, 1-15 range). Parse this field from the ELF and pass it through to the kernel. Fixes cilium#669 Signed-off-by: nikos.nikolakakis <[email protected]> Co-authored-by: Timo Beckers <[email protected]>
This commit teaches ebpf-go to understand the __ulong BTF map definition macro and uses it to specify a 64-bit value for map_extra in an arena map, defining the start of the mmap region for the Collection's arena. Signed-off-by: Timo Beckers <[email protected]>
ti-mo
force-pushed
the
add-bloom-filter-support
branch
from
1c30f3e to
6c09c28
Compare
|
@NoNickeD Thanks for picking this up! I ended up moving things around a bit and added separate testdata for arena maps since loader.c needs to work on all kernels in the matrix. I'm planning on doing more arena-related work, so it made sense long-term. Arenas also need the |
|
@ti-mo Thanks for merging and improving this! The separate arena testdata and __ulong macro support for 64-bit map_extra make sense. Learned a lot from your restructuring. Thanks for the guidance! |
3 participants
Summary
This PR adds support for the MapExtra field and bloom filter maps, addressing issue #669.
Changes Made
Notes