aya: lazily load custom BTF sources - #1644
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. |
90f2d1b to
72c4b28
Compare
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed 1 file and all commit messages, and made 5 comments.
Reviewable status: 1 of 3 files reviewed, 5 unresolved discussions (waiting on swananan).
aya/src/bpf.rs line 140 at r1 (raw file):
struct BtfSource { read: Box< dyn Fn() -> io::Result<Vec<u8>>
I think you could avoid the Vec if you made this a function that takes a function from &[u8] to parsed BTF
aya/src/bpf.rs line 159 at r1 (raw file):
impl BtfSource { fn read(&self) -> io::Result<Vec<u8>> { (self.read)()
destructure? :)
aya/src/bpf.rs line 260 at r1 (raw file):
+ std::panic::RefUnwindSafe + std::panic::UnwindSafe + 'static,
consider giving this huge type a local alias
Code quote:
F: Fn() -> io::Result<R>
+ Send
+ Sync
+ std::panic::RefUnwindSafe
+ std::panic::UnwindSafe
+ 'static,aya/src/bpf.rs line 140 at r1 (raw file):
struct BtfSource { read: Box< dyn Fn() -> io::Result<Vec<u8>>
does this need to be Fn or can it be FnOnce?
aya/src/bpf.rs line 608 at r1 (raw file):
TargetBtf::Parsed(btf) => Some(btf), _ => None, }
can we avoid this somehow and do it all in one match?
Code quote:
match btf {
TargetBtf::Parsed(btf) => Some(btf),
_ => None,
}72c4b28 to
816a2b7
Compare
swananan
left a comment
There was a problem hiding this comment.
@swananan made 5 comments.
Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on tamird).
aya/src/bpf.rs line 140 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
does this need to be Fn or can it be FnOnce?
Add a comment to explain why keep it Fn here.
aya/src/bpf.rs line 140 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
I think you could avoid the Vec if you made this a function that takes a function from &[u8] to parsed BTF
Done.
aya/src/bpf.rs line 159 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
destructure? :)
Sure, will keep it in mind (:
aya/src/bpf.rs line 260 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
consider giving this huge type a local alias
Done.
aya/src/bpf.rs line 608 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
can we avoid this somehow and do it all in one match?
Done.
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed 1 file and all commit messages, made 1 comment, and resolved 5 discussions.
Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on swananan).
aya/src/bpf.rs line 251 at r2 (raw file):
/// # Ok::<(), aya::EbpfError>(()) /// ``` pub fn btf_source<F, R>(&mut self, source: F) -> &mut Self
this signature still requires the function to return io::Read (e.g. a cursor) so it can't avoid an allocation. can this take a generic version of BtfSourceFn?
a887a2c to
3019af7
Compare
swananan
left a comment
There was a problem hiding this comment.
@swananan made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on tamird).
aya/src/bpf.rs line 251 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
this signature still requires the function to return io::Read (e.g. a cursor) so it can't avoid an allocation. can this take a generic version of BtfSourceFn?
Got it, the cursor used in the test is redundant, we should not allocate an intermediate Vec, since callers may already provide contiguous bytes.
I tried the suggested approach, but that requires callers to map source errors into EbpfError themselves.
pub fn btf_source<F>(&mut self, source: F) -> &mut Self
where
F: Fn(&BtfParser) -> Result<Btf, EbpfError>
+ Send
+ Sync
+ RefUnwindSafe
+ UnwindSafe
+ 'static,
{
self.btf = TargetBtf::Source(BtfSource(Box::new(source)));
self
}
EbpfLoader::new()
.btf_source(|parse| {
let data =
fs::read("/custom_btf_file").map_err(EbpfError::BtfSourceError)?;
parse(&data).map_err(EbpfError::BtfError)
})
.load_file("file.o")?;
So I switched to a different apporach in the latest update.
vadorovsky
left a comment
There was a problem hiding this comment.
No further comments apart from what @tamird said, I'll approve after addressing the last comment.
@vadorovsky made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on tamird).
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed all commit messages and made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on swananan and vadorovsky).
aya/src/bpf.rs line 251 at r2 (raw file):
Previously, swananan (swananan) wrote…
Got it, the cursor used in the test is redundant, we should not allocate an intermediate Vec, since callers may already provide contiguous bytes.
I tried the suggested approach, but that requires callers to map source errors into EbpfError themselves.
pub fn btf_source<F>(&mut self, source: F) -> &mut Self where F: Fn(&BtfParser) -> Result<Btf, EbpfError> + Send + Sync + RefUnwindSafe + UnwindSafe + 'static, { self.btf = TargetBtf::Source(BtfSource(Box::new(source))); self } EbpfLoader::new() .btf_source(|parse| { let data = fs::read("/custom_btf_file").map_err(EbpfError::BtfSourceError)?; parse(&data).map_err(EbpfError::BtfError) }) .load_file("file.o")?;So I switched to a different apporach in the latest update.
can you add a test that exercises the case where the function wants to do something a bit esoteric like mmap a file where the BTF is located in a subslice? I think this will not work for something like that.
3019af7 to
e453e4e
Compare
swananan
left a comment
There was a problem hiding this comment.
@swananan made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on tamird and vadorovsky).
aya/src/bpf.rs line 251 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
can you add a test that exercises the case where the function wants to do something a bit esoteric like mmap a file where the BTF is located in a subslice? I think this will not work for something like that.
I overlooked the subslice case. The current approach can not support it, unless we add an owning wrapper around the mmap, record the range and implement AsRef, but that feels too heavy for aya. So I switched back to the suggested approach and also added a UT for this case.
Custom BTF currently has to be read and parsed before configuring an EbpfLoader, even when the object does not require target BTF. Add btf_source(), which accepts a callback invoked only for objects with CO-RE relocations or typed kernel symbols. The callback receives a parser configured for the object endianness, allowing callers to parse borrowed data such as a subslice of an mmap without an intermediate allocation. Cache the first successfully parsed BTF for reuse by the loader, while keeping btf(&Btf) for callers that share one parsed value across multiple loaders.
e453e4e to
d6f3e5a
Compare
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed 3 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on vadorovsky).
tamird
left a comment
There was a problem hiding this comment.
Will let vad merge. Gonna kick off bot reviews.
@tamird made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on vadorovsky).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6f3e5a844
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR updates Aya’s EbpfLoader to support lazy loading of custom target BTF: instead of requiring target BTF to be read/parsed up-front, a caller-provided factory is invoked only when CO-RE relocations or typed kernel symbols require it, and the parsed result is cached for subsequent loads using the same loader.
Changes:
- Added
EbpfLoader::btf_sourceto supply a lazy target-BTF reader/parser callback and cache the successfully parsed BTF within the loader. - Refactored loader internals to represent target BTF configuration as
System | Parsed | Source, and to only consult target BTF when the object requires it. - Added integration/unit tests to validate that the source callback is cached after first use and not called when target BTF is not needed.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
aya/src/bpf.rs |
Introduces btf_source, caching behavior, and associated error/loader logic changes for lazy target BTF loading. |
test/integration-test/src/tests/btf_relocations.rs |
Adds integration tests covering caching and “not called when unneeded” behavior for btf_source. |
xtask/public-api/aya.txt |
Updates public API snapshot for the new EbpfLoader::btf_source method and EbpfError::BtfSourceError variant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
vadorovsky
left a comment
There was a problem hiding this comment.
@vadorovsky reviewed 3 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on swananan).
Custom BTF currently has to be read and parsed before configuring an EbpfLoader, even when the object does not require target BTF.
Add a reader factory that is invoked only for CO-RE relocations or typed kernel symbols, and cache the BTF after the first successful parse. Keep btf(&Btf) for callers that share one parsed value across multiple loaders.
Added/updated tests?
We strongly encourage you to add a test for your changes.
Checklist
cargo +nightly fmt.You can find failing lints with
cargo xtask clippy.cargo test.cargo xtask public-api --bless.(Optional) What GIF best describes this PR or how it makes you feel?
This change is