feat(aya): add support for map-of-maps (HashOfMaps, ArrayOfMaps)#1446
Conversation
✅ Deploy Preview for aya-rs-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive support for BPF map-of-maps (BPF_MAP_TYPE_HASH_OF_MAPS and BPF_MAP_TYPE_ARRAY_OF_MAPS) to the Aya framework, building upon the foundation from PR #70.
Changes:
- Added
innerattribute to#[map]macro for declaring map-of-maps templates in eBPF code - Implemented
HashMapOfMapsandArrayOfMapstypes withget(),iter(), and other helper methods - Added support for program array population via
EbpfLoader::set_prog_array_entry()andEbpf::populate_prog_arrays()
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| xtask/public-api/aya.txt | Updated public API surface with new map-of-maps types and test run functionality |
| xtask/public-api/aya-obj.txt | Added inner map bindings and map creation helpers to object parser API |
| xtask/public-api/aya-ebpf.txt | Introduced InnerMap trait and map-of-maps types for eBPF side |
| test/integration-test/src/tests/prog_array.rs | Added integration tests for program array population |
| test/integration-test/src/tests/map_of_maps.rs | Added integration tests for map-of-maps functionality |
| test/integration-ebpf/src/prog_array.rs | eBPF test program for tail calls using program arrays |
| test/integration-ebpf/src/map_of_maps.rs | eBPF test program demonstrating map-of-maps usage |
| ebpf/aya-ebpf/src/maps/*.rs | Implemented InnerMap trait across all compatible map types |
| aya/src/sys/bpf.rs | Added inner_map_fd parameter to map creation and test run functionality |
| aya/src/maps/of_maps/*.rs | Implemented HashMapOfMaps and ArrayOfMaps with iterators |
| aya/src/maps/mod.rs | Added map-of-maps variants to Map enum and error handling |
| aya/src/bpf.rs | Enhanced loader to handle map-of-maps creation and program array population |
| aya-obj/src/obj.rs | Added parsing for .maps.inner section and inner map bindings |
| aya-obj/src/maps.rs | Extended map definitions with inner map support and helper constructors |
| aya-ebpf-macros/src/map.rs | Implemented inner attribute processing in map macro |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2593b39d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let value: Option<u32> = | ||
| bpf_map_lookup_elem(fd, key, flags).map_err(|io_error| SyscallError { | ||
| call: "bpf_map_lookup_elem", | ||
| io_error, | ||
| })?; | ||
| if let Some(id) = value { | ||
| let inner_fd = bpf_map_get_fd_by_id(id)?; | ||
| let info = MapInfo::new_from_fd(inner_fd.as_fd())?; | ||
| let map_data = MapData::from_id(info.id())?; |
There was a problem hiding this comment.
Here get() treats the value returned by bpf_map_lookup_elem as a map ID and calls bpf_map_get_fd_by_id, but this API inserts raw map FDs (insert passes value.as_fd().as_raw_fd()), so the lookup is expected to return an FD in common map-in-map setups. In that case this path will fail (EINVAL/ENOENT) or open a different map whose ID happens to match the FD integer. Consider constructing MapData directly from the returned FD (or otherwise aligning with the stored value type) instead of resolving it as an ID.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This implementation is correct. For map-of-maps types, the kernel uses an asymmetric API:
- Update (
bpf_map_update_elem): expects the FD of the inner map - Lookup (
bpf_map_lookup_elem): returns the ID of the inner map
This is documented behavior in the Linux kernel: https://docs.kernel.org/bpf/map_of_maps.html
The lookup value must be converted to an FD using bpf_map_get_fd_by_id, which is exactly what this code does
There was a problem hiding this comment.
Perhaps this deserves some inline comments?
| let value: Option<u32> = | ||
| bpf_map_lookup_elem(fd, index, flags).map_err(|io_error| SyscallError { | ||
| call: "bpf_map_lookup_elem", | ||
| io_error, | ||
| })?; | ||
| if let Some(id) = value { | ||
| let inner_fd = bpf_map_get_fd_by_id(id)?; | ||
| let info = MapInfo::new_from_fd(inner_fd.as_fd())?; | ||
| let map_data = MapData::from_id(info.id())?; |
There was a problem hiding this comment.
This get() path interprets the lookup value as a map ID and calls bpf_map_get_fd_by_id, but set() stores raw map FDs in the outer array. If the kernel returns the stored FD (as it commonly does for map-in-map values), bpf_map_get_fd_by_id will fail or resolve the wrong map. Using MapData::from_fd on the returned value would keep the value interpretation consistent with set().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same as above - this is the correct behavior. The kernel's map-of-maps syscall API is asymmetric by design:
BPF_MAP_UPDATE_ELEMtakes an FDBPF_MAP_LOOKUP_ELEMreturns an ID
See: https://docs.ebpf.io/linux/map-type/BPF_MAP_TYPE_ARRAY_OF_MAPS/
Using bpf_map_get_fd_by_id(id) to convert the returned ID to an FD is the intended pattern.
There was a problem hiding this comment.
Perhaps this deserves some inline comments?
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
@tamird made 3 comments.
Reviewable status: 0 of 43 files reviewed, 3 unresolved discussions (waiting on @Brskt).
-- commits line 70 at r35:
The commits in this PR are mostly a mess, but e.g. this one looks useful on its own. Did you intend for the commits history to be preserved? If yes, we will need you to rewrite it into something coherent. If not, then this PR is 3k lines that have to be reviewed in one shot, which is quite difficult.
Code quote:
New commits in r8 on 1/17/2026 at 4:21 PM:
- d1f0cb8: feat(aya): Add prog_array population support for tail calls
Add EbpfLoader::set_prog_array_entry() to declaratively specify which
programs should be placed in program arrays at which indices.
Add Ebpf::populate_prog_arrays() to populate the declared entries with
loaded program file descriptors after programs are loaded.
This enables easier setup of tail call jump tables without manually
managing program array entries.
| let value: Option<u32> = | ||
| bpf_map_lookup_elem(fd, index, flags).map_err(|io_error| SyscallError { | ||
| call: "bpf_map_lookup_elem", | ||
| io_error, | ||
| })?; | ||
| if let Some(id) = value { | ||
| let inner_fd = bpf_map_get_fd_by_id(id)?; | ||
| let info = MapInfo::new_from_fd(inner_fd.as_fd())?; | ||
| let map_data = MapData::from_id(info.id())?; |
There was a problem hiding this comment.
Perhaps this deserves some inline comments?
| let value: Option<u32> = | ||
| bpf_map_lookup_elem(fd, key, flags).map_err(|io_error| SyscallError { | ||
| call: "bpf_map_lookup_elem", | ||
| io_error, | ||
| })?; | ||
| if let Some(id) = value { | ||
| let inner_fd = bpf_map_get_fd_by_id(id)?; | ||
| let info = MapInfo::new_from_fd(inner_fd.as_fd())?; | ||
| let map_data = MapData::from_id(info.id())?; |
There was a problem hiding this comment.
Perhaps this deserves some inline comments?
Brskt
force-pushed
the
hashmapofmaps-new
branch
from
f2593b3 to
333e272
Compare
Brskt
left a comment
Brskt
left a comment
There was a problem hiding this comment.
@Brskt made 3 comments and resolved 2 discussions.
Reviewable status: 0 of 43 files reviewed, 1 unresolved discussion (waiting on @tamird).
Previously, tamird (Tamir Duberstein) wrote…
The commits in this PR are mostly a mess, but e.g. this one looks useful on its own. Did you intend for the commits history to be preserved? If yes, we will need you to rewrite it into something coherent. If not, then this PR is 3k lines that have to be reviewed in one shot, which is quite difficult.
Yes, I've kept the commit history and rewritten it as requested:
- aya-ebpf: eBPF-side map-of-maps implementation
- aya-ebpf-macros: inner attribute for #[map] macro
- aya-obj: Map constructors and .maps.inner parsing
- aya: userspace map-of-maps support
- tests: integration and unit tests
- public API updates
Should be easier to review now.
| let value: Option<u32> = | ||
| bpf_map_lookup_elem(fd, index, flags).map_err(|io_error| SyscallError { | ||
| call: "bpf_map_lookup_elem", | ||
| io_error, | ||
| })?; | ||
| if let Some(id) = value { | ||
| let inner_fd = bpf_map_get_fd_by_id(id)?; | ||
| let info = MapInfo::new_from_fd(inner_fd.as_fd())?; | ||
| let map_data = MapData::from_id(info.id())?; |
| let value: Option<u32> = | ||
| bpf_map_lookup_elem(fd, key, flags).map_err(|io_error| SyscallError { | ||
| call: "bpf_map_lookup_elem", | ||
| io_error, | ||
| })?; | ||
| if let Some(id) = value { | ||
| let inner_fd = bpf_map_get_fd_by_id(id)?; | ||
| let info = MapInfo::new_from_fd(inner_fd.as_fd())?; | ||
| let map_data = MapData::from_id(info.id())?; |
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
@tamird partially reviewed 15 files and made 2 comments.
Reviewable status: 1 of 43 files reviewed, 2 unresolved discussions (waiting on @Brskt).
Previously, Brskt wrote…
Yes, I've kept the commit history and rewritten it as requested:
- aya-ebpf: eBPF-side map-of-maps implementation
- aya-ebpf-macros: inner attribute for #[map] macro
- aya-obj: Map constructors and .maps.inner parsing
- aya: userspace map-of-maps support
- tests: integration and unit tests
- public API updates
Should be easier to review now.
it's still just one big blob, right? the commits are now cut along which crates they touch, which is maybe easier for review but they need to be squashed on merge. do I understand correctly?
ebpf/aya-ebpf/src/maps/mod.rs line 47 at r36 (raw file):
/// /// Only implement this trait for map types that can be safely used as inner maps. pub unsafe trait InnerMap {}
🤔 does this need to be pub?
Brskt
left a comment
Brskt
left a comment
There was a problem hiding this comment.
@Brskt made 2 comments.
Reviewable status: 1 of 43 files reviewed, 2 unresolved discussions (waiting on @tamird).
Previously, tamird (Tamir Duberstein) wrote…
it's still just one big blob, right? the commits are now cut along which crates they touch, which is maybe easier for review but they need to be squashed on merge. do I understand correctly?
Yes, that's correct. Split for easier review, feel free to squash on merge.
ebpf/aya-ebpf/src/maps/mod.rs line 47 at r36 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
🤔 does this need to be pub?
I tested with pub(crate) and here's what happens:
Build results:
cargo build:⚠️ 3 warnings (private_bounds)cargo clippy -D warnings: ❌ 3 errors - fails CIcargo test: ✅ passintegration tests: ✅ 127 passedpublic-api check: ❌ fails (InnerMap removed from API)
Why it fails:
InnerMap is used as a trait bound on public types:
pub struct ArrayOfMaps<T: InnerMap> { ... }A private trait in a public bound triggers private_bounds, which becomes an error with -D warnings.
Conclusion:
pub is required to pass CI. The unsafe marker already discourages external implementations, and the kernel validates map types at load time anyway.
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
@tamird partially reviewed 42 files, made 8 comments, and resolved 1 discussion.
Reviewable status: 19 of 43 files reviewed, 8 unresolved discussions (waiting on @Brskt).
ebpf/aya-ebpf/src/maps/mod.rs line 47 at r36 (raw file):
Previously, Brskt wrote…
I tested with
pub(crate)and here's what happens:Build results:
cargo build:⚠️ 3 warnings (private_bounds)cargo clippy -D warnings: ❌ 3 errors - fails CIcargo test: ✅ passintegration tests: ✅ 127 passedpublic-api check: ❌ fails (InnerMap removed from API)Why it fails:
InnerMapis used as a trait bound on public types:pub struct ArrayOfMaps<T: InnerMap> { ... }A private trait in a public bound triggers private_bounds, which becomes an error with -D warnings.
Conclusion:
pub is required to pass CI. The unsafe marker already discourages external implementations, and the kernel validates map types at load time anyway.
I see. This should be a sealed trait then since we don't want external implementations.
-- commits line 25 at r38:
this commit is ...bad. it's adding a bunch of code that is unused, making review impossible.
ebpf/aya-ebpf/src/maps/hash_of_maps.rs line 14 at r36 (raw file):
pub struct HashOfMaps<K, V> { def: UnsafeCell<bpf_map_def>, _k: PhantomData<K>,
See #1447; use a single phantom plz
aya-ebpf-macros/src/map.rs line 20 at r37 (raw file):
let mut args = syn::parse2(attrs)?; let name = name_arg(&mut args).unwrap_or_else(|| item.ident.to_string()); let inner = pop_string_arg(&mut args, "inner");
while you're here, please add err_on_unknown_args(args)?; (see #1448)
aya-ebpf-macros/src/map.rs line 40 at r37 (raw file):
#[used] static #binding_ident: [u8; #binding_len] = [#(#binding_bytes),*]; }
are we following libbpf conventions here? needs citations
Code quote:
// Create a unique identifier for the binding
let binding_ident = format_ident!("__inner_map_binding_{}", name);
// Format: "outer_name\0inner_name\0" (null-terminated strings)
let binding_value = format!("{}\0{}\0", name, inner);
let binding_len = binding_value.len();
let binding_bytes = binding_value.as_bytes();
quote! {
#[unsafe(link_section = ".maps.inner")]
#[used]
static #binding_ident: [u8; #binding_len] = [#(#binding_bytes),*];
}aya-ebpf-macros/src/map.rs line 42 at r37 (raw file):
} } else { quote! {}
we can drop this b/c Options impls ToTokens
https://docs.rs/quote/latest/quote/trait.ToTokens.html#impl-ToTokens-for-Option%3CT%3E
aya-ebpf-macros/src/map.rs line 118 at r37 (raw file):
); // "OUTER\0INNER_TEMPLATE\0" = 21 bytes assert!(expanded_str.contains("21usize"), "expected 21 bytes");
these assertions are problematic because they emit no information on failure
Code quote:
assert!(
expanded_str.contains(".maps.inner"),
"expected .maps.inner section"
);
assert!(
expanded_str.contains("__inner_map_binding_OUTER"),
"expected binding identifier"
);
// "OUTER\0INNER_TEMPLATE\0" = 21 bytes
assert!(expanded_str.contains("21usize"), "expected 21 bytes");ebpf/aya-ebpf/src/maps/array_of_maps.rs line 19 at r36 (raw file):
unsafe impl<T: InnerMap> Sync for ArrayOfMaps<T> {} impl<T: InnerMap> ArrayOfMaps<T> {
let's reduce some of this boilerplate, see #1447
Brskt
force-pushed
the
hashmapofmaps-new
branch
from
333e272 to
60f6d7c
Compare
Brskt
left a comment
Brskt
left a comment
There was a problem hiding this comment.
@Brskt made 8 comments.
Reviewable status: 19 of 43 files reviewed, 8 unresolved discussions (waiting on @tamird).
Previously, tamird (Tamir Duberstein) wrote…
this commit is ...bad. it's adding a bunch of code that is unused, making review impossible.
Done, is this the way u wanted ?
aya-ebpf-macros/src/map.rs line 20 at r37 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
while you're here, please add
err_on_unknown_args(args)?;(see #1448)
Done.
aya-ebpf-macros/src/map.rs line 40 at r37 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
are we following libbpf conventions here? needs citations
No, this is not following libbpf conventions. Documentation has been added to clarify this.
libbpf uses BTF relocations within the .maps section for inner map bindings (see https://patchwork.ozlabs.org/comment/2418417/), where u declare .values = { [0] = &inner_map, ... } and libbpf processes the relocations.
The .maps.inner section is an aya-specific mechanism. This approach was chosen because:
- aya-ebpf doesn't require BTF for map definitions
- It provides a simpler mechanism that works with both legacy and BTF-style maps
The format is now documented in both aya-ebpf-macros/src/map.rs and aya-obj/src/obj.rs with references to the libbpf implementation.
aya-ebpf-macros/src/map.rs line 42 at r37 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
we can drop this b/c Options impls ToTokens
https://docs.rs/quote/latest/quote/trait.ToTokens.html#impl-ToTokens-for-Option%3CT%3E
Done.
aya-ebpf-macros/src/map.rs line 118 at r37 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
these assertions are problematic because they emit no information on failure
Done.
ebpf/aya-ebpf/src/maps/array_of_maps.rs line 19 at r36 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
let's reduce some of this boilerplate, see #1447
Acknowledged. This PR can be rebased on top of #1447 once it's merged to use the MapDef abstraction, which will eliminate the duplicated UnsafeCell<bpf_map_def> wrapper, unsafe impl Sync, and constructor boilerplate.
Should I wait for #1447 to land first, or would you prefer I implement a similar pattern in this PR?
ebpf/aya-ebpf/src/maps/hash_of_maps.rs line 14 at r36 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
See #1447; use a single phantom plz
Done.
ebpf/aya-ebpf/src/maps/mod.rs line 47 at r36 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
I see. This should be a sealed trait then since we don't want external implementations.
Done.
Each map type now implements Sealed (e.g., impl<T> Sealed for Array<T> {}), preventing external implementations while keeping InnerMap public to satisfy the trait bounds on ArrayOfMaps<T: InnerMap> and HashOfMaps<K, V: InnerMap>.
Brskt
force-pushed
the
hashmapofmaps-new
branch
from
60f6d7c to
581a00e
Compare
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
@tamird partially reviewed 26 files, made 3 comments, and resolved 3 discussions.
Reviewable status: 3 of 43 files reviewed, 7 unresolved discussions (waiting on @Brskt).
aya-ebpf-macros/src/map.rs line 40 at r37 (raw file):
Previously, Brskt wrote…
No, this is not following libbpf conventions. Documentation has been added to clarify this.
libbpf uses BTF relocations within the .maps section for inner map bindings (see https://patchwork.ozlabs.org/comment/2418417/), where u declare
.values = { [0] = &inner_map, ... }and libbpf processes the relocations.The .maps.inner section is an aya-specific mechanism. This approach was chosen because:
- aya-ebpf doesn't require BTF for map definitions
- It provides a simpler mechanism that works with both legacy and BTF-style maps
The format is now documented in both aya-ebpf-macros/src/map.rs and aya-obj/src/obj.rs with references to the libbpf implementation.
Doesn't this mean that libbpf can't load aya programs that use map-in-map, and vice versa? That's generally not the approach we have taken.
A better link: torvalds/linux@646f02ffdd49
aya-ebpf-macros/src/map.rs line 106 at r47 (raw file):
#[test] fn test_map_with_inner() {
the tests above check for the exact generated code, can we follow the same style? if not, please add a comment explaining why
aya-ebpf-macros/src/map.rs line 171 at r47 (raw file):
), ); assert!(result.is_err());
pretty weak assertion
Brskt
force-pushed
the
hashmapofmaps-new
branch
from
581a00e to
cf9be0a
Compare
Brskt
left a comment
Brskt
left a comment
There was a problem hiding this comment.
@Brskt made 3 comments.
Reviewable status: 2 of 48 files reviewed, 7 unresolved discussions (waiting on @tamird).
aya-ebpf-macros/src/map.rs line 40 at r37 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Doesn't this mean that libbpf can't load aya programs that use map-in-map, and vice versa? That's generally not the approach we have taken.
A better link: torvalds/linux@646f02ffdd49
Done. #[btf_map] with btf_maps::ArrayOfMaps/HashOfMaps now works with both aya and libbpf loaders (tested both). Uses [*const V; 0] for the values field per the BTF relocation format libbpf expects.
Legacy #[map(inner = "...")] remains aya-specific but is now documented as such.
Does this address your concern ?
aya-ebpf-macros/src/map.rs line 106 at r47 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
the tests above check for the exact generated code, can we follow the same style? if not, please add a comment explaining why
Done.
aya-ebpf-macros/src/map.rs line 171 at r47 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
pretty weak assertion
Done.
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
@tamird partially reviewed 41 files and made 2 comments.
Reviewable status: 2 of 48 files reviewed, 9 unresolved discussions (waiting on @Brskt).
ebpf/aya-ebpf/src/btf_maps/array.rs line 11 at r53 (raw file):
/// /// This map type stores elements of type `T` indexed by `u32` keys. /// The struct layout is designed to be compatible with both aya and libbpf loaders.
what does that mean?
ebpf/aya-ebpf/src/btf_maps/array.rs line 23 at r53 (raw file):
#[repr(C)] #[allow(dead_code)] pub struct Array<T, const M: usize, const F: usize = 0> {
why did we need to toss bpf_map_def!?
Brskt
force-pushed
the
hashmapofmaps-new
branch
from
cf9be0a to
fd9cb5b
Compare
Brskt
left a comment
Brskt
left a comment
There was a problem hiding this comment.
@Brskt made 2 comments.
Reviewable status: 2 of 48 files reviewed, 9 unresolved discussions (waiting on @tamird).
ebpf/aya-ebpf/src/btf_maps/array.rs line 11 at r53 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
what does that mean?
I've improved the comments. Is it clearer now ?
ebpf/aya-ebpf/src/btf_maps/array.rs line 23 at r53 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
why did we need to toss
bpf_map_def!?
The existing btf_maps that use btf_map_def! (RingBuf, SkStorage) aren't libbpf-compatible either - they only work with aya's loader. For this PR, you requested that map-of-maps be loadable by both aya and libbpf, so I used flat #[repr(C)] structs instead.
Brskt
force-pushed
the
hashmapofmaps-new
branch
from
fd9cb5b to
0e4c970
Compare
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
@tamird made 1 comment.
Reviewable status: 2 of 48 files reviewed, 9 unresolved discussions (waiting on @Brskt).
ebpf/aya-ebpf/src/btf_maps/array.rs line 23 at r53 (raw file):
Previously, Brskt wrote…
The existing btf_maps that use
btf_map_def!(RingBuf, SkStorage) aren't libbpf-compatible either - they only work with aya's loader. For this PR, you requested that map-of-maps be loadable by both aya and libbpf, so I used flat#[repr(C)]structs instead.
Ah, yeah this is also #1455. Would you be willing to send a separate PR to fix that for all the maps?
|
Previously, tamird (Tamir Duberstein) wrote…
Or at least a separate commit would be great. |
Brskt
left a comment
Brskt
left a comment
There was a problem hiding this comment.
@Brskt made 1 comment.
Reviewable status: 2 of 48 files reviewed, 9 unresolved discussions (waiting on @tamird and @vadorovsky).
ebpf/aya-ebpf/src/btf_maps/array.rs line 23 at r53 (raw file):
Previously, vadorovsky (Michal R) wrote…
Or at least a separate commit would be great.
Done in #1457
Brskt
force-pushed
the
hashmapofmaps-new
branch
5 times, most recently
from
c3d2e4a to
eef6947
Compare
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on alessandrod, Brskt, and vadorovsky).
| /// Missing inner map BTF definition for a map-of-maps. | ||
| #[error( | ||
| "map `{outer_name}` is a map-of-maps but has no inner map definition; \ | ||
| use #[btf_map] with a BTF-typed map-of-maps that includes an inner map type" |
There was a problem hiding this comment.
Interesting that copilot picked up on legacy maps at all. Where did it find that? Perhaps in the PR description?
| /// This is used to create inner maps for map-of-maps types. | ||
| /// | ||
| /// This trait is sealed and cannot be implemented outside of this crate. | ||
| pub trait CreatableMap: sealed::CreatableMap { |
There was a problem hiding this comment.
why do we need this trait? doesn't seem to be used as a bound anywhere and API
wise it's better if I can call create() without having to import a trait
There was a problem hiding this comment.
I don't know, @tamird said "The trait is better than giving each one an inherent method, I think.". Which way do I put ?
There was a problem hiding this comment.
I don't think that makes sense?
The trait still has to be implemented for each implementor, so code wise it's even more code. It's not used as a bound anywhere. And - and worst part - now users have to import the trait to call create.
The trait is effectively unused today I think we should remove it. If we ever need to pass a generic type based on the existence of create we can add it back.
There was a problem hiding this comment.
Can we use it as a bound? Traits at least give a clue that API should be implemented by all maps and give a name to the common surface. A macro can do this as well, it's just a little more fragile.
There was a problem hiding this comment.
I don't think it makes sense? Why are we leaking extra API to users that doesn't bring any benefits to them?
If we need to internally ensure we don't forget to implement something (and we probably should now that we support so much of the ebpf API!), we should figure out a way to do so without impacting the public API.
There was a problem hiding this comment.
Why do we need CreatableMap? What do we need to parametrize based on the existence of ::create?
There was a problem hiding this comment.
I don't understand your question as a reply to my comment. Can we just make all these traits one trait which represents a map that isn't of-maps?
There was a problem hiding this comment.
your comment does a bunch of impl_creatable_map and it's not clear to me why the CreatableMap trait needs to exist to begin with
there is no use for API users, it shouldn't be in the API
There was a problem hiding this comment.
from discord:
I still don't understand why we need the traits
to me FromMapData makes sense
it's an ugly but necessary public API because we have MapData
CreatableMap is an internal convenience thing. But by making it a trait we make it a public thing.
I undersand where you're coming from, but copypasta, macro, anything is better than leaking an implementation thing to the public API
the leak: I now have to use CreatableMap to call a method, but it makes no sense
so @Brskt let's either justify the existence of a trait or remove it
There was a problem hiding this comment.
Removed it, looks better.
| @@ -0,0 +1,72 @@ | |||
| use core::ptr::NonNull; | |||
There was a problem hiding this comment.
OCD nit: would be great if we could keep the module names consistent with
userspace?
userspace is aya::maps::of_maps::ArrayOfMaps
this one is aya_ebpf::btf_maps::array_of_maps::ArrayOfMaps
|
This also needs a rebase now I'm afraid |
|
I hope you don't mind, I took the liberty to rebase this at #1564 |
|
@OliverGavin The PR is still alive, just waiting of some reviewers which way to approach a point. |
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
keen to get this merged!
@tamird reviewed 28 files and all commit messages, made 4 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on alessandrod, Brskt, and vadorovsky).
| // The kernel requires an inner map fd when creating a map-of-maps. | ||
| let btf_inner_map; | ||
| let inner_map_fd = if is_map_of_maps { | ||
| let inner = map_obj.inner().ok_or_else(|| { | ||
| EbpfError::MapError(MapError::MissingInnerMapDefinition { | ||
| outer_name: name.clone(), | ||
| }) | ||
| })?; | ||
| btf_inner_map = MapData::create(inner, &format!("{name}.inner"), btf_fd)?; | ||
| Some(btf_inner_map.fd().as_fd()) | ||
| } else { |
| /// This is used to create inner maps for map-of-maps types. | ||
| /// | ||
| /// This trait is sealed and cannot be implemented outside of this crate. | ||
| pub trait CreatableMap: sealed::CreatableMap { |
There was a problem hiding this comment.
from discord:
I still don't understand why we need the traits
to me FromMapData makes sense
it's an ugly but necessary public API because we have MapData
CreatableMap is an internal convenience thing. But by making it a trait we make it a public thing.
I undersand where you're coming from, but copypasta, macro, anything is better than leaking an implementation thing to the public API
the leak: I now have to use CreatableMap to call a method, but it makes no sense
so @Brskt let's either justify the existence of a trait or remove it
| @@ -0,0 +1,72 @@ | |||
| use core::ptr::NonNull; | |||
Brskt
left a comment
Brskt
left a comment
There was a problem hiding this comment.
I also tweaked some files, it should be better than before.
@Brskt made 4 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on alessandrod, tamird, and vadorovsky).
| // The kernel requires an inner map fd when creating a map-of-maps. | ||
| let btf_inner_map; | ||
| let inner_map_fd = if is_map_of_maps { | ||
| let inner = map_obj.inner().ok_or_else(|| { | ||
| EbpfError::MapError(MapError::MissingInnerMapDefinition { | ||
| outer_name: name.clone(), | ||
| }) | ||
| })?; | ||
| btf_inner_map = MapData::create(inner, &format!("{name}.inner"), btf_fd)?; | ||
| Some(btf_inner_map.fd().as_fd()) | ||
| } else { |
| /// This is used to create inner maps for map-of-maps types. | ||
| /// | ||
| /// This trait is sealed and cannot be implemented outside of this crate. | ||
| pub trait CreatableMap: sealed::CreatableMap { |
There was a problem hiding this comment.
Removed it, looks better.
| @@ -0,0 +1,72 @@ | |||
| use core::ptr::NonNull; | |||
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
@codex review
@tamird partially reviewed 18 files and all commit messages, made 1 comment, resolved 3 discussions, and dismissed @alessandrod from 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on alessandrod and vadorovsky).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c548f9121
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// and inner `bpf_map_lookup_elem` calls in a single method. | ||
| /// | ||
| /// This trait is sealed and cannot be implemented outside this crate. | ||
| pub trait MapDef: private::MapDef {} |
There was a problem hiding this comment.
Only btf_maps::Array implements the sealed private::MapDef trait, while the new get_value/get_value_ptr_mut APIs require V: MapDef. In practice, a map-of-maps whose inner map is another supported BTF map with lookup semantics, such as PerCpuArray or LpmTrie, can be declared and loaded but cannot use the fused lookup APIs, and downstream crates cannot add the missing impls because the trait is sealed here. Please add the corresponding in-crate impls or narrow the API/docs to the actually supported inner map types.
Useful? React with 👍 / 👎.
| /// Key and value types of a BTF map definition. | ||
| /// | ||
| /// Used by map-of-maps types to perform fused lookups that combine the outer | ||
| /// and inner `bpf_map_lookup_elem` calls in a single method. | ||
| /// | ||
| /// This trait is sealed and cannot be implemented outside this crate. | ||
| pub trait MapDef: private::MapDef {} | ||
|
|
||
| impl<T: private::MapDef> MapDef for T {} |
There was a problem hiding this comment.
Duplicate of Codex, applied.
Add support for `BPF_MAP_TYPE_HASH_OF_MAPS` and
`BPF_MAP_TYPE_ARRAY_OF_MAPS`, kernel map types that store inner map
FDs as values.
aya-ebpf adds `btf_maps::of_maps::{ArrayOfMaps, HashOfMaps}` with an
inner map type parameter `V`. The `btf_map_def!` macro grows an
`inner_map: V` clause that emits the BTF `values: [*const V; 0]`
relocation field. The macro also generates a sealed `private::MapDef`
impl exposing each map's declared key and value types, so the fused
`get_value` / `get_value_ptr_mut` lookups compile for any BTF map as
inner, not only `Array`.
aya adds `maps::of_maps::{ArrayOfMaps, HashOfMaps}` with sealed
`InnerMap` and `FromMapData` bounds. `EbpfLoader::load()` populates
inner maps declared in BTF; pinned map-of-maps defer inner-map
creation until the outer map is not already pinned, mirroring
`BPF_F_INNER_MAP`. Each non-of-maps map type gains an inherent
`create(max_entries, flags)` method for building standalone inner
maps from userspace.
aya-obj tracks the inner map definition on `BtfMap` and `LegacyMap`
so the loader can resolve inner-map references during attach.
Cover `btf_maps::ArrayOfMaps` and `btf_maps::HashOfMaps` with uprobe tests parametrized over a `MapKind` enum via `#[test_case]`. The eBPF probes read from an inner array through the outer map and write the looked-up value into a result slot. Tests assert both the plain `get` path and the fused `get_value` / `get_value_ptr_mut` lookups reach the inner data, and that inserting an inner map into the outer does not consume the userspace handle.
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
@codex review
@tamird reviewed 7 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status:
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a571ec8297
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if arr.len != 0 { | ||
| return Err(BtfError::InvalidValuesSpec { |
There was a problem hiding this comment.
values arrays
This rejects valid libbpf-style map-in-map definitions that prepopulate the outer map with inner maps, where __array(values, struct inner_map) is initialized with slots such as .values = { [0] = &inner } and the BTF array length is therefore non-zero. In that scenario Aya returns InvalidValuesSpec while parsing the object, so programs that previously load with libbpf cannot be loaded even though the values field is the standard way to encode map-in-map templates and initial contents.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
False positive. __array expands to typeof(val) *name[], so clang always emits BTF nelems=0 regardless of .values initializers. libbpf's parse_btf_map_def does the same check.
|
Huge! |
|
Nice work, congrats! |
This PR is a continuation of #70.
Adds
aya_ebpf::btf_maps::of_maps::{ArrayOfMaps, HashOfMaps}with aninner map type parameter
V(anybtf_mapsmap). Thebtf_map_def!macro grows an
inner_map: Vclause that emits avalues: [*const V; 0]field so loaders can resolve the inner map template via BTF.
get_value/get_value_ptr_mutperform fused outer+inner lookups ina single method.
Userspace adds
aya::maps::of_maps::{ArrayOfMaps, HashOfMaps}withget,set/insert,keys, andpin, gated by sealedInnerMapand
FromMapDatabounds.EbpfLoader::load()populates inner mapsdeclared in BTF; pinned map-of-maps defer inner-map creation until the
outer map is not already pinned, mirroring
BPF_F_INNER_MAP. Eachnon-of-maps map type gains an inherent
create(max_entries, flags)method for building standalone inner maps from userspace.
aya_objtracks the inner map definition onBtfMapandLegacyMapso the loader can resolve inner-map references during attach.
Integration tests parametrize the basic
getand fusedget_value/get_value_ptr_mutpaths over aMapKindenum via#[test_case], andverify that inserting an inner map into the outer does not consume the
userspace handle.
Added/updated tests?
Checklist
cargo +nightly fmt.You can find failing lints with
cargo xtask clippy.cargo test.cargo xtask public-api --bless.This change is