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: L3 cleanups #23876

Merged
merged 3 commits into from Feb 21, 2023
Merged

bpf: L3 cleanups #23876

merged 3 commits into from Feb 21, 2023

Conversation

julianwiedmann
Copy link
Member

@julianwiedmann julianwiedmann commented Feb 20, 2023 •
edited

Some L3-related cleanups that have accumulated. The csum helpers follow the usage outlined in the l3_csum_replace() man page.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 20, 2023
@julianwiedmann julianwiedmann added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact. labels Feb 20, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Feb 20, 2023
@julianwiedmann julianwiedmann changed the title bpf l3 cleanups WIP bpf l3 cleanups Feb 20, 2023
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann
Copy link
Member Author

/test-1.16-4.19

1 similar comment
@julianwiedmann
Copy link
Member Author

/test-1.16-4.19

@julianwiedmann julianwiedmann changed the title WIP bpf l3 cleanups bpf: L3 cleanups Feb 20, 2023
@julianwiedmann julianwiedmann marked this pull request as ready for review February 20, 2023 16:27
@julianwiedmann julianwiedmann requested a review from a team as a code owner February 20, 2023 16:27
gentoo-root
gentoo-root requested changes Feb 20, 2023
View reviewed changes
Copy link
Contributor

@gentoo-root gentoo-root left a comment

Choose a reason for hiding this comment

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

Nice cleanups! One tiny comment, and the rest looks good to me.

bpf/lib/l3.h Outdated Show resolved Hide resolved
@julianwiedmann
bpf: l3: de-duplicate ipv_local_delivery()
5eb31b8 
Extract a helper for the shared logic between IPv4 and IPv6.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: ipv4: add L3 csum wrappers
8070d00 
l3_csum_replace() can be used in two ways:
a) updating the packet's checksum after modifying a specific header field
   to a new value.
b) updating the packet's checksum with an opaque diff, eg. after updating
   multiple fields.

Add corresponding wrappers, so we don't have to worry about setting up all
the parameter correctly.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: combine L3 csum changes in IP_POOLS path
eefff62 
This code path modifies the .saddr and .daddr fields. Calculate & apply
the corresponding csum diff in one go, thus saving one call to
l3_csum_update() and avoiding an intermittent L3 header revalidation.

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

Removed some trivial __maybe_unused annotations. CI was all green before, so think we're still good to go unless the build checks flag something unexpected.

dylandreimerink
dylandreimerink approved these changes Feb 21, 2023
View reviewed changes
Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

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

Nice cleanup! Small nit, not blocking, just in case you need to rebase or make other changes anyway

bpf/lib/ipv4.h
@@ -48,7 +63,7 @@ static __always_inline int ipv4_dec_ttl(struct __ctx_buff *ctx, int off,

new_ttl = ttl - 1;
/* l3_csum_replace() takes at min 2 bytes, zero extended. */
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This comment doesn't make sense anymore with the new function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I would say it still makes sense, but refers to an implementation detail of ipv4_csum_update_by_value, which might be not obvious to a reader.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I was unsure what to do with this, so left it for now. Maybe turn it into proper documentation for ipv4_csum_update_by_value().

@julianwiedmann
Copy link
Member Author

Removed some trivial __maybe_unused annotations. CI was all green before, so think we're still good to go unless the build checks flag something unexpected.

-> marking ready-to-merge

@julianwiedmann julianwiedmann added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 21, 2023
@pchaigno pchaigno merged commit 47de737 into cilium:master Feb 21, 2023
@julianwiedmann julianwiedmann deleted the 1.14-bpf-l3-cleanups branch February 21, 2023 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@dylandreimerink dylandreimerink dylandreimerink approved these changes

Assignees
No one assigned
Labels
kind/cleanup This includes no functional changes. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@julianwiedmann @gentoo-root @dylandreimerink @pchaigno