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.

Sign up for GitHub

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

Jump to bottom

bpf: policy: cleanups to reduce program size #27369

Merged
merged 3 commits into from Aug 14, 2023
Merged

bpf: policy: cleanups to reduce program size #27369

merged 3 commits into from Aug 14, 2023

Conversation

julianwiedmann
Copy link
Member

@julianwiedmann julianwiedmann commented Aug 9, 2023 •
edited

This brings a bit of innocent fine-tuning for the BPF policy code.

For the example of bpf_host's tail_rev_nodeport_lb6() (containing HostFW policy code), it shaves off around 100 instructions.

@julianwiedmann julianwiedmann added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. release-note/misc This PR makes changes that have no direct user impact. labels Aug 9, 2023
@julianwiedmann
Copy link
Member Author

/ci-verifier

1 similar comment
@julianwiedmann
Copy link
Member Author

/ci-verifier

@julianwiedmann
bpf: policy: add policy_can_ingress*() wrappers
5075572 
We already have protocol-specific helpers for egress policy. Add the
matching helpers for ingress policy, and make the naming consistent.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann julianwiedmann added the kind/complexity-issue Relates to BPF complexity or program size issues label Aug 9, 2023
@julianwiedmann
bpf: policy: fine-tune ICMP logic
75e346a 
We're currently including the IPPROTO_ICMP handling into the IPv6 path, and
the IPPROTO_ICMPV6 handling into the IPv4 path.

Pass a static ethertype from the policy helpers, so that the compiler can
be a bit smarter and only includes the relevant code.

Also load the ICMP header just once, and share it amongst the code that
handles ALLOW_ICMP_FRAG_NEEDED and ENABLE_ICMP_RULE.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: policy: fine-tune logic in __policy_can_access()
b7c4a57 
Use goto statements to consolidate the calls to __account_and_check().

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann changed the title 1.15 bpf policy cleanups bpf: policy: cleanups to reduce program size Aug 9, 2023
@julianwiedmann julianwiedmann marked this pull request as ready for review August 9, 2023 09:19
@julianwiedmann julianwiedmann requested a review from a team as a code owner August 9, 2023 09:19
gentoo-root
gentoo-root approved these changes Aug 11, 2023
View reviewed changes
Copy link
Contributor

@gentoo-root gentoo-root left a comment •
edited by julianwiedmann

Choose a reason for hiding this comment

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

I'm not sure I fully understand the usefulness of the last patch.. Otherwise looks good!

Is the compiler not smart enough to consolidate these calls itself? What is the benefit of reducing the text size if we are only going through one of the branches?

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 11, 2023
@julianwiedmann
Copy link
Member Author

I'm not sure I fully understand the usefulness of the last patch.. Otherwise looks good!
Is the compiler not smart enough to consolidate these calls itself?

Nope - I was a bit surprised too :).

The baseline is
tail_rev_nodeport_lb4: processed 32822 insns (limit 131072), stack depth 216 (2029 unverified insns)

With the patch applied this becomes
tail_rev_nodeport_lb4: processed 30557 insns (limit 131072), stack depth 216 (1973 unverified insns)

That's not much of course. But I'll take wins where I can get them, until the 4.19 kernel is gone.

@julianwiedmann julianwiedmann merged commit f439a14 into cilium:main Aug 14, 2023
59 checks passed
@julianwiedmann julianwiedmann deleted the 1.15-bpf-policy-cleanups branch August 14, 2023 07:45
joestringer
joestringer reviewed Mar 7, 2024
View reviewed changes
bpf/lib/policy.h
Comment on lines +70 to +76
/* Convert from unsigned char to unsigned short
* considering byte order(little-endian).
* In the little-endian case, for example, 2byte data "AB"
* convert to "BA".
* Therefore, the "icmp_type" should be shifted not just casting.
*/
key.dport = (__u16)(icmphdr.type << 8);
Copy link
Member

Choose a reason for hiding this comment

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

How can this be correct on all architectures without using some form of ntohs()?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, looks like this was originally introduced by @chez-shanpu in #16516 .

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, probably this line should use bpf_htons() instead of using shift.
I'll fix it and submit a PR.

@chez-shanpu chez-shanpu mentioned this pull request Mar 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer left review comments

@chez-shanpu chez-shanpu chez-shanpu left review comments

@gentoo-root gentoo-root gentoo-root approved these changes

Assignees
No one assigned
Labels
kind/complexity-issue Relates to BPF complexity or program size issues ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@julianwiedmann @gentoo-root @joestringer @chez-shanpu