Skip to content

Conversation

julianwiedmann
Copy link
Member

@julianwiedmann julianwiedmann commented May 12, 2025

#38308 helped to simplify the BPF-redirect logic, as we can now assume support for HAVE_FIB_IFINDEX. Therefore let's lift the remaining processing of the FIB result from the low-level redirect helper, and make it the caller's responsibility to select the egress interface.

While at it also free up some related space in the CT entry thanks to #38308, and clarify the use of redirect_neigh() in XDP context.

We expect all supported kernels to offer HAVE_FIB_INDEX, and there's no
code that would still use the `ifindex` field. Reclaim these two bytes
for the future.

Note that in particular there's no downgrade concerns here - even if we
go back to v1.17, the underlying kernel is still the same and provides
HAVE_FIB_INDEX. And thus Cilium v1.17 doesn't expect us to provide
CT entries with a valid `ifindex`.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@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 May 12, 2025
@julianwiedmann julianwiedmann added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels May 12, 2025
@julianwiedmann
Copy link
Member Author

/test

Move the selection of the egress interface into fib_do_redirect()'s
callers, and provide it through a single `oif` parameter (which now becomes
read-only). The callers know much better what information is available
(FIB lookup result, DIRECT_ROUTING_DEV_IFINDEX, ...) and what outcome they
want. And some of them already look at `fib_params->l.ifindex` anyway.

Note that is just a cleanup, there shouldn't be any change in behavior.

While at it also update the big code comment to reflect current reality.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
XDP programs have no business using redirect_neigh(). This turns a
dormant runtime error into an immediate build-time bug.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann julianwiedmann force-pushed the pr/jwi/main/bpf-fib-modernization branch from 91d6864 to 7a5933e Compare May 12, 2025 06:18
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann changed the title Pr/jwi/main/bpf fib modernization bpf: fib: stream-line fib_do_redirect() May 12, 2025
@julianwiedmann julianwiedmann marked this pull request as ready for review May 12, 2025 07:44
@julianwiedmann julianwiedmann requested review from a team as code owners May 12, 2025 07:44
Copy link
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

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

A very welcomed LGTM :-D.

@julianwiedmann julianwiedmann enabled auto-merge May 12, 2025 13:58
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@julianwiedmann julianwiedmann added this pull request to the merge queue May 13, 2025
@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 May 13, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 13, 2025
@julianwiedmann julianwiedmann added this pull request to the merge queue May 14, 2025
Merged via the queue into main with commit 5bb0ef2 May 14, 2025
405 of 407 checks passed
@julianwiedmann julianwiedmann deleted the pr/jwi/main/bpf-fib-modernization branch May 14, 2025 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants