Add MapExtra support and bloom filter maps #1856
Conversation
- Add MapExtra field to MapSpec struct for map-specific configuration
- Update map creation to pass MapExtra to kernel syscall
- Handle map_extra field in BTF map definitions (elf_reader.go)
- Add NewBloomFilter helper function for creating bloom filter maps
- Add comprehensive tests for bloom filter functionality
Bloom filter maps were introduced in Linux 5.16 and use the MapExtra
field to specify the number of hash functions (lower 4 bits, 1-15 range).
The default is 5 hash functions if not specified.
This enables creating bloom filters with custom hash function counts:
spec, err := NewBloomFilter("my_bloom", 4, 1000, 3)
Fixes cilium#669
Signed-off-by: nikos.nikolakakis <[email protected]>
NoNickeD
force-pushed
the
add-bloom-filter-support
branch
from
937ed79 to
f079fc5
Compare
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. |
Per reviewer feedback, add test coverage for parsing bloom filter maps from ELF. This ensures MapExtra field is properly parsed from ELF files and can be used to configure the number of hash functions for bloom filter maps. - Add BPF_MAP_TYPE_BLOOM_FILTER definition to testdata/common.h - Add bloom_filter map with map_extra=7 to testdata/loader.c - Update TestLoadCollectionSpec to expect bloom_filter map in results - Rebuild testdata/loader-clang-14-*.elf files using Ubuntu VM with proper build tools The test now verifies that bloom filter maps can be loaded from compiled BPF programs and the MapExtra field is correctly preserved. Signed-off-by: nikos.nikolakakis <[email protected]>
Remove extra blank line at end of file to fix CI formatting check. Signed-off-by: nikos.nikolakakis <[email protected]>
Adjust comment alignment to match clang-format expectations from CI. Signed-off-by: nikos.nikolakakis <[email protected]>
Include bloom filter map definition in loader test data as requested in PR review. The bloom filter map includes MapExtra field set to 7 for testing ELF parsing of map_extra. Signed-off-by: nikos.nikolakakis <[email protected]>
NoNickeD
force-pushed
the
add-bloom-filter-support
branch
from
7a34fb0 to
e3207a9
Compare
Rebuild all testdata ELF files (clang-14, clang-17, clang-20) with bloom filter map definition included. This ensures the ELF parsing tests correctly validate bloom filter parsing from binary files. Signed-off-by: nikos.nikolakakis <[email protected]>
Summary
This PR adds support for the MapExtra field and bloom filter maps, addressing issue #669.
Changes Made
Notes