Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove duplication, avoid handwritten assembly #1165

Merged
merged 4 commits into from
Feb 5, 2025
Merged

Conversation

tamird
Copy link
Member

@tamird tamird commented Feb 3, 2025

See individual commits.


This change is Reviewable

Copy link

netlify bot commented Feb 3, 2025

Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 8c4ce0c
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/67a29f0fb3efc5000899f153
😎 Deploy Preview https://deploy-preview-1165--aya-rs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@tyrone-wu
Copy link
Contributor

tyrone-wu commented Feb 4, 2025

I cleaned up some of the is_info_map_ids_supported() and is_info_gpl_compatible_supported() bloat in 294b950 which is open for review. This was somewhat inspired from #1043.

@tamird tamird changed the title Remove duplication Remove duplication, avoid handwritten assembly Feb 4, 2025
@mergify mergify bot added the aya-obj Relating to the aya-obj crate label Feb 4, 2025
@tamird
Copy link
Member Author

tamird commented Feb 4, 2025

cc @Billy99 I believe the last commit fixes a bug on big-endian. PTAL.

@tamird
Copy link
Member Author

tamird commented Feb 4, 2025

I was incorrect, the FD always goes in the first instruction, so there was no bug here.

Copy link
Member

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is much better than what we had before. Encoding those bpf_insns must have take a while 😓 A few small things to consider but otherwise looks good.


let gpl = b"GPL\0";
let mov64_imm = (BPF_ALU64 | BPF_MOV | BPF_K).try_into().unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given we re-use this same program in many places, is there any benefit to making it const?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe? But the constants are u32 and the field is u8 - without an as cast there's no way to make it const IIUC.

u.insn_cnt = insns.len() as u32;
u.insns = insns.as_ptr() as u64;
u.prog_type = bpf_prog_type::BPF_PROG_TYPE_SOCKET_FILTER as u32;
u.prog_type = bpf_prog_type::BPF_PROG_TYPE_TRACEPOINT as u32;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a motivation to change the program type?
IIRC SOCKET_FILTER is the oldest of the BPF program types so should always be available.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's in the commit message.

Use BPF_PROG_TYPE_TRACEPOINT instead of BPF_PROG_TYPE_SOCKET_FILTER
as the former seems to work with more feature detection functions.

is_perf_link_supported didn't work when this was SOCKET_FILTER.


let gpl = b"GPL\0";
u.license = gpl.as_ptr() as u64;
// TODO(tamird): remove when these are generated on the userspace side.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since #1166 has merged, please let me know if you need me to press the button on the codegen workflow. You should be able to trigger it from Github Actions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, working on that in #1168 since the constants I wanted weren't all generated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I was being blind.

u.map_name[..name_len]
.copy_from_slice(unsafe { slice::from_raw_parts(name.as_ptr(), name_len) });
let name_bytes = name.to_bytes_with_nul();
let len = cmp::min(name_bytes.len(), u.map_name.len());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this account for the nul byte required to terminate the string?
The string has to be 15 chars + 1 nul byte.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, note I'm using to_bytes_with_nul.

The safety requirements of this transmutation are simpler.
Use `BPF_PROG_TYPE_TRACEPOINT` instead of `BPF_PROG_TYPE_SOCKET_FILTER`
as the former seems to work with more feature detection functions.
@tamird tamird merged commit 942ea51 into aya-rs:main Feb 5, 2025
31 checks passed
@tamird tamird deleted the dedupe branch February 5, 2025 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aya This is about aya (userspace) aya-obj Relating to the aya-obj crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants