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

Replaced declare_tailcall_if with logic in the loader #30467

Merged
merged 5 commits into from
Feb 29, 2024
Merged

Replaced declare_tailcall_if with logic in the loader #30467

merged 5 commits into from
Feb 29, 2024

Conversation

dylandreimerink
Copy link
Member

@dylandreimerink dylandreimerink commented Jan 26, 2024 •
edited
Loading

This PR replaces the declare_tailcall_if macro with loader logic in Go. Before this we had the declare_tailcall_if and invoke_tailcall_if. These were used together to determine if a function should be inlined or be emitted and called as a tailcall. For this to work, both the declare and invoke side needed to have the same condition or risk a missed tailcall.

This change simplifies the datapath code a bit without performance regressions. We now always emit all tailcalls into the ELF, but we use logic in the loader to see if any invoke_tailcall_if or any non-conditional tail_call_static references a given tailcall. If a tailcall is unreachable it will not be loaded, saving us load time.

This is part of a larger movement from compile time logic to load time logic to enable clang-free in the future, and to make the datapath code easier today.

Replaced `declare_tailcall_if` with logic in the loader

@dylandreimerink dylandreimerink added the release-note/misc This PR makes changes that have no direct user impact. label Jan 26, 2024
@dylandreimerink dylandreimerink force-pushed the feature/replace-declare-tailcall-if branch from 627c9e9 to 7395b27 Compare January 26, 2024 15:10
@dylandreimerink
Copy link
Member Author

/test

@dylandreimerink dylandreimerink force-pushed the feature/replace-declare-tailcall-if branch 2 times, most recently from cb91cd0 to 8e7864f Compare January 26, 2024 16:01
@dylandreimerink
Copy link
Member Author

/test

@julianwiedmann julianwiedmann mentioned this pull request Jan 29, 2024
Closed
@dylandreimerink
Copy link
Member Author

/test

@dylandreimerink dylandreimerink force-pushed the feature/replace-declare-tailcall-if branch from 9cf79de to dc51a32 Compare January 29, 2024 14:09
@dylandreimerink
Copy link
Member Author

/test

@dylandreimerink dylandreimerink force-pushed the feature/replace-declare-tailcall-if branch from dc51a32 to 00386a4 Compare January 29, 2024 14:57
@dylandreimerink
Copy link
Member Author

/test

@dylandreimerink dylandreimerink force-pushed the feature/replace-declare-tailcall-if branch from 00386a4 to ac1568b Compare January 29, 2024 15:29
@dylandreimerink
Copy link
Member Author

/test

@dylandreimerink dylandreimerink force-pushed the feature/replace-declare-tailcall-if branch from ac1568b to 1f52b46 Compare January 29, 2024 16:24
@dylandreimerink
Copy link
Member Author

/test

@dylandreimerink dylandreimerink force-pushed the feature/replace-declare-tailcall-if branch from 1f52b46 to 42e1c41 Compare January 29, 2024 17:10
@dylandreimerink
Copy link
Member Author

/test

@dylandreimerink dylandreimerink force-pushed the feature/replace-declare-tailcall-if branch 3 times, most recently from d02e4fc to a0e3c11 Compare January 30, 2024 10:21
@dylandreimerink
Copy link
Member Author

/test

@dylandreimerink dylandreimerink changed the title Feature/replace declare tailcall if Replaced declare_tailcall_if with logic in the loader Jan 30, 2024
@dylandreimerink dylandreimerink marked this pull request as ready for review January 30, 2024 13:00
@dylandreimerink dylandreimerink requested review from a team as code owners January 30, 2024 13:00
@dylandreimerink dylandreimerink force-pushed the feature/replace-declare-tailcall-if branch from a0e3c11 to b2c0343 Compare January 31, 2024 14:45
@dylandreimerink
Copy link
Member Author

/test

@dylandreimerink dylandreimerink deleted the feature/replace-declare-tailcall-if branch February 29, 2024 21:14
jschwinger233 added a commit to jschwinger233/cilium that referenced this pull request Mar 1, 2024
@jschwinger233
bpf/tests: Remove SKIP_ICMPV6_NS_HANDLING from tc_nodeport_l3_dev.c
44fcc5f 
SKIP_ICMPV6_NS_HANDLING was there to pass bpf coverage test, which is
gone by cilium#28090.

In the meantime, removing SKIP_ICMPV6_NS_HANDLING from
tc_nodeport_l3_dev.c prevents "potential missed tailcall" errors
introduced by cilium#30467, as
tail_icmp6_handle_ns() doesn't exist when SKIP_ICMPV6_NS_HANDLING is
defined, but still gets tail-called by icmp6_handle_ns().

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
github-merge-queue bot pushed a commit that referenced this pull request Mar 1, 2024
@jschwinger233 @julianwiedmann
bpf/tests: Remove SKIP_ICMPV6_NS_HANDLING from tc_nodeport_l3_dev.c
60c5e76 
SKIP_ICMPV6_NS_HANDLING was there to pass bpf coverage test, which is
gone by #28090.

In the meantime, removing SKIP_ICMPV6_NS_HANDLING from
tc_nodeport_l3_dev.c prevents "potential missed tailcall" errors
introduced by #30467, as
tail_icmp6_handle_ns() doesn't exist when SKIP_ICMPV6_NS_HANDLING is
defined, but still gets tail-called by icmp6_handle_ns().

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
jschwinger233 added a commit to jschwinger233/cilium that referenced this pull request Mar 5, 2024
@jschwinger233
bpf/tests: Remove SKIP_ICMPV6_NS_HANDLING from tc_nodeport_l3_dev.c
116f0e7 
[ upstream commit: 60c5e76 ]

SKIP_ICMPV6_NS_HANDLING was there to pass bpf coverage test, which is
gone by cilium#28090.

In the meantime, removing SKIP_ICMPV6_NS_HANDLING from
tc_nodeport_l3_dev.c prevents "potential missed tailcall" errors
introduced by cilium#30467, as
tail_icmp6_handle_ns() doesn't exist when SKIP_ICMPV6_NS_HANDLING is
defined, but still gets tail-called by icmp6_handle_ns().

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
jschwinger233 added a commit to jschwinger233/cilium that referenced this pull request Mar 5, 2024
@jschwinger233
bpf/tests: Remove SKIP_ICMPV6_NS_HANDLING from tc_nodeport_l3_dev.c
ed69836 
[ upstream commit: 60c5e76 ]

SKIP_ICMPV6_NS_HANDLING was there to pass bpf coverage test, which is
gone by cilium#28090.

In the meantime, removing SKIP_ICMPV6_NS_HANDLING from
tc_nodeport_l3_dev.c prevents "potential missed tailcall" errors
introduced by cilium#30467, as
tail_icmp6_handle_ns() doesn't exist when SKIP_ICMPV6_NS_HANDLING is
defined, but still gets tail-called by icmp6_handle_ns().

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
jschwinger233 added a commit to jschwinger233/cilium that referenced this pull request Mar 5, 2024
@jschwinger233
bpf/tests: Add IPv6 NDP bpf test
f3e7943 
[ upstream commit: 8d4db89 ]

[ backporter's notes: minor changes due to lack of cilium#30467 in v1.15 ]

This commit adds bpf/tests/ipv6_ndp_from_netdev_test.c to cover two
scenarios:
1. from_netdev receives IPv6 NS for a pod IP on the same host
2. from_netdev receives IPv6 NS for the node IP (eth0's addr)

For case 1, from_netdev should return a NA on behalf of the target pod
to avoid cilium#30926.
for case 2, it must return the NS to stack to address cilium#14509.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
joestringer pushed a commit that referenced this pull request Mar 5, 2024
@jschwinger233 @joestringer
bpf/tests: Remove SKIP_ICMPV6_NS_HANDLING from tc_nodeport_l3_dev.c
beb08ce 
[ upstream commit: 60c5e76 ]

SKIP_ICMPV6_NS_HANDLING was there to pass bpf coverage test, which is
gone by #28090.

In the meantime, removing SKIP_ICMPV6_NS_HANDLING from
tc_nodeport_l3_dev.c prevents "potential missed tailcall" errors
introduced by #30467, as
tail_icmp6_handle_ns() doesn't exist when SKIP_ICMPV6_NS_HANDLING is
defined, but still gets tail-called by icmp6_handle_ns().

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
joestringer pushed a commit that referenced this pull request Mar 5, 2024
@jschwinger233 @joestringer
bpf/tests: Add IPv6 NDP bpf test
b9d1c19 
[ upstream commit: 8d4db89 ]

[ backporter's notes: minor changes due to lack of #30467 in v1.15 ]

This commit adds bpf/tests/ipv6_ndp_from_netdev_test.c to cover two
scenarios:
1. from_netdev receives IPv6 NS for a pod IP on the same host
2. from_netdev receives IPv6 NS for the node IP (eth0's addr)

For case 1, from_netdev should return a NA on behalf of the target pod
to avoid #30926.
for case 2, it must return the NS to stack to address #14509.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
jschwinger233 added a commit to jschwinger233/cilium that referenced this pull request Mar 6, 2024
@jschwinger233
bpf/tests: Remove SKIP_ICMPV6_NS_HANDLING from tc_nodeport_l3_dev.c
d9cf213 
SKIP_ICMPV6_NS_HANDLING was there to pass bpf coverage test, which is
gone by cilium#28090.

In the meantime, removing SKIP_ICMPV6_NS_HANDLING from
tc_nodeport_l3_dev.c prevents "potential missed tailcall" errors
introduced by cilium#30467, as
tail_icmp6_handle_ns() doesn't exist when SKIP_ICMPV6_NS_HANDLING is
defined, but still gets tail-called by icmp6_handle_ns().

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
jschwinger233 added a commit to jschwinger233/cilium that referenced this pull request Mar 6, 2024
@jschwinger233
bpf/tests: Add IPv6 NDP bpf test
3f26c14 
[ upstream commit: 8d4db89 ]

[ backporter's notes: minor changes due to lack of cilium#30467 and cilium#27134 ]

This commit adds bpf/tests/ipv6_ndp_from_netdev_test.c to cover two
scenarios:
1. from_netdev receives IPv6 NS for a pod IP on the same host
2. from_netdev receives IPv6 NS for the node IP (eth0's addr)

For case 1, from_netdev should return a NA on behalf of the target pod
to avoid cilium#30926.
for case 2, it must return the NS to stack to address cilium#14509.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
jschwinger233 added a commit to jschwinger233/cilium that referenced this pull request Mar 6, 2024
@jschwinger233
bpf/tests: Add IPv6 NDP bpf test
62d7a67 
[ upstream commit: 8d4db89 ]

[ backporter's notes: minor changes due to lack of cilium#30467 and cilium#27134 ]

This commit adds bpf/tests/ipv6_ndp_from_netdev_test.c to cover two
scenarios:
1. from_netdev receives IPv6 NS for a pod IP on the same host
2. from_netdev receives IPv6 NS for the node IP (eth0's addr)

For case 1, from_netdev should return a NA on behalf of the target pod
to avoid cilium#30926.
for case 2, it must return the NS to stack to address cilium#14509.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
jschwinger233 added a commit to jschwinger233/cilium that referenced this pull request Mar 6, 2024
@jschwinger233
bpf/tests: Remove SKIP_ICMPV6_NS_HANDLING from tc_nodeport_l3_dev.c
1677363 
[ upstream commit: 60c5e76 ]

SKIP_ICMPV6_NS_HANDLING was there to pass bpf coverage test, which is
gone by cilium#28090.

In the meantime, removing SKIP_ICMPV6_NS_HANDLING from
tc_nodeport_l3_dev.c prevents "potential missed tailcall" errors
introduced by cilium#30467, as
tail_icmp6_handle_ns() doesn't exist when SKIP_ICMPV6_NS_HANDLING is
defined, but still gets tail-called by icmp6_handle_ns().

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
jschwinger233 added a commit to jschwinger233/cilium that referenced this pull request Mar 6, 2024
@jschwinger233
bpf/tests: Add IPv6 NDP bpf test
ea3049c 
[ upstream commit: 8d4db89 ]

[ backporter's notes: minor changes due to lack of cilium#30467 and cilium#27134 ]

This commit adds bpf/tests/ipv6_ndp_from_netdev_test.c to cover two
scenarios:
1. from_netdev receives IPv6 NS for a pod IP on the same host
2. from_netdev receives IPv6 NS for the node IP (eth0's addr)

For case 1, from_netdev should return a NA on behalf of the target pod
to avoid cilium#30926.
for case 2, it must return the NS to stack to address cilium#14509.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
jschwinger233 added a commit to jschwinger233/cilium that referenced this pull request Mar 6, 2024
@jschwinger233
bpf/tests: Add IPv6 NDP bpf test
6614ed1 
[ upstream commit: 8d4db89 ]

[ backporter's notes: minor changes due to lack of cilium#30467 and cilium#27134 ]

This commit adds bpf/tests/ipv6_ndp_from_netdev_test.c to cover two
scenarios:
1. from_netdev receives IPv6 NS for a pod IP on the same host
2. from_netdev receives IPv6 NS for the node IP (eth0's addr)

For case 1, from_netdev should return a NA on behalf of the target pod
to avoid cilium#30926.
for case 2, it must return the NS to stack to address cilium#14509.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
julianwiedmann
julianwiedmann reviewed Mar 7, 2024
View reviewed changes
bpf/lib/l3.h
Comment on lines +37 to +40
#ifndef SKIP_ICMPV6_HOPLIMIT_HANDLING
if (ret == DROP_TTL_EXCEEDED)
return icmp6_send_time_exceeded(ctx, l3_off, direction);

#endif
Copy link
Member

Choose a reason for hiding this comment

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

Oh nice catch, thank you! I think we'll need to wire that up properly at some point for XDP.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is what this is doing. The only place where we ever declare this macro is in bpf_xdp.c. https://github.com/cilium/cilium/blob/main/bpf/bpf_xdp.c#L22

Copy link
Member

Choose a reason for hiding this comment

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

I meant that one day we should actually implement the hoplimit handling for XDP :).

julianwiedmann
julianwiedmann reviewed Mar 7, 2024
View reviewed changes
bpf/bpf_host.c
Comment on lines +598 to +604
#ifdef ENABLE_IPV6
if (ret == NAT_46X64_RECIRC) {
ctx_store_meta(ctx, CB_SRC_LABEL, secctx);
return tail_call_internal(ctx, CILIUM_CALL_IPV6_FROM_NETDEV,
ext_err);
}

#endif
Copy link
Member

Choose a reason for hiding this comment

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

Curious, why didn't we catch the same case in bpf_xdp?

(I would hope that the agent enforces IPv6=true when using the NAT46x64 gateway, so this is probably harmless for real deployments)

Copy link
Member Author

Choose a reason for hiding this comment

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

Curious, why didn't we catch the same case in bpf_xdp?

It could just be that we are not testing an instance where we have IPv4=true, IPv6=false, NodeportAcceleration=true ?

jschwinger233 added a commit to jschwinger233/cilium that referenced this pull request Mar 7, 2024
@jschwinger233
bpf/tests: Add IPv6 NDP bpf test
e04fac8 
[ upstream commit: 8d4db89 ]

[ backporter's notes: minor changes due to lack of cilium#30467 and cilium#27134 ]

This commit adds bpf/tests/ipv6_ndp_from_netdev_test.c to cover two
scenarios:
1. from_netdev receives IPv6 NS for a pod IP on the same host
2. from_netdev receives IPv6 NS for the node IP (eth0's addr)

For case 1, from_netdev should return a NA on behalf of the target pod
to avoid cilium#30926.
for case 2, it must return the NS to stack to address cilium#14509.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
jschwinger233 added a commit to jschwinger233/cilium that referenced this pull request Mar 7, 2024
@jschwinger233
bpf/tests: Remove SKIP_ICMPV6_NS_HANDLING from tc_nodeport_l3_dev.c
fb2639e 
[ upstream commit: 60c5e76 ]

[ backporter's notes: adds tc_nodeport_l3_dev.o into NOCOVER_PATTERN to
  avoid verifier issue as v1.14 still has bpf coverage test. ]

SKIP_ICMPV6_NS_HANDLING was there to pass bpf coverage test, which is
gone by cilium#28090.

In the meantime, removing SKIP_ICMPV6_NS_HANDLING from
tc_nodeport_l3_dev.c prevents "potential missed tailcall" errors
introduced by cilium#30467, as
tail_icmp6_handle_ns() doesn't exist when SKIP_ICMPV6_NS_HANDLING is
defined, but still gets tail-called by icmp6_handle_ns().

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
jschwinger233 added a commit to jschwinger233/cilium that referenced this pull request Mar 7, 2024
@jschwinger233
bpf/tests: Remove SKIP_ICMPV6_NS_HANDLING from tc_nodeport_l3_dev.c
7c2932e 
[ upstream commit: 60c5e76 ]

[ backporter's notes: adds tc_nodeport_l3_dev.o into NOCOVER_PATTERN to
  avoid verifier issue as v1.14 still has bpf coverage test. ]

SKIP_ICMPV6_NS_HANDLING was there to pass bpf coverage test, which is
gone by cilium#28090.

In the meantime, removing SKIP_ICMPV6_NS_HANDLING from
tc_nodeport_l3_dev.c prevents "potential missed tailcall" errors
introduced by cilium#30467, as
tail_icmp6_handle_ns() doesn't exist when SKIP_ICMPV6_NS_HANDLING is
defined, but still gets tail-called by icmp6_handle_ns().

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
jschwinger233 added a commit to jschwinger233/cilium that referenced this pull request Mar 7, 2024
@jschwinger233
bpf/tests: Add IPv6 NDP bpf test
aef19fd 
[ upstream commit: 8d4db89 ]

[ backporter's notes: minor changes due to lack of cilium#30467 and cilium#27134 ]

This commit adds bpf/tests/ipv6_ndp_from_netdev_test.c to cover two
scenarios:
1. from_netdev receives IPv6 NS for a pod IP on the same host
2. from_netdev receives IPv6 NS for the node IP (eth0's addr)

For case 1, from_netdev should return a NA on behalf of the target pod
to avoid cilium#30926.
for case 2, it must return the NS to stack to address cilium#14509.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
julianwiedmann pushed a commit that referenced this pull request Mar 8, 2024
@jschwinger233 @julianwiedmann
bpf/tests: Remove SKIP_ICMPV6_NS_HANDLING from tc_nodeport_l3_dev.c
caf0bb6 
[ upstream commit: 60c5e76 ]

[ backporter's notes: adds tc_nodeport_l3_dev.o into NOCOVER_PATTERN to
  avoid verifier issue as v1.14 still has bpf coverage test. ]

SKIP_ICMPV6_NS_HANDLING was there to pass bpf coverage test, which is
gone by #28090.

In the meantime, removing SKIP_ICMPV6_NS_HANDLING from
tc_nodeport_l3_dev.c prevents "potential missed tailcall" errors
introduced by #30467, as
tail_icmp6_handle_ns() doesn't exist when SKIP_ICMPV6_NS_HANDLING is
defined, but still gets tail-called by icmp6_handle_ns().

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
julianwiedmann pushed a commit that referenced this pull request Mar 8, 2024
@jschwinger233 @julianwiedmann
bpf/tests: Add IPv6 NDP bpf test
c8c4737 
[ upstream commit: 8d4db89 ]

[ backporter's notes: minor changes due to lack of #30467 and #27134 ]

This commit adds bpf/tests/ipv6_ndp_from_netdev_test.c to cover two
scenarios:
1. from_netdev receives IPv6 NS for a pod IP on the same host
2. from_netdev receives IPv6 NS for the node IP (eth0's addr)

For case 1, from_netdev should return a NA on behalf of the target pod
to avoid #30926.
for case 2, it must return the NS to stack to address #14509.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
@marseel marseel mentioned this pull request Mar 11, 2024
Closed
This was referenced Mar 15, 2024
Merged
Merged
@ti-mo ti-mo mentioned this pull request Mar 22, 2024
Merged
2 tasks
@ti-mo ti-mo added the backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. label Mar 22, 2024
@github-actions github-actions bot added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels Mar 25, 2024
@jrajahalme jrajahalme mentioned this pull request Mar 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julianwiedmann julianwiedmann julianwiedmann left review comments

@lmb lmb lmb approved these changes

@ti-mo ti-mo ti-mo approved these changes

@learnitall learnitall learnitall approved these changes

@aditighag aditighag aditighag approved these changes

@michi-covalent michi-covalent michi-covalent approved these changes

@gentoo-root gentoo-root Awaiting requested review from gentoo-root gentoo-root is a code owner automatically assigned from cilium/sig-datapath

@tklauser tklauser Awaiting requested review from tklauser tklauser is a code owner automatically assigned from cilium/ci-structure

Assignees

@dylandreimerink dylandreimerink

Labels
backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
cilium v1.15.3
Status: Released
Milestone
clang-free
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@dylandreimerink @ti-mo @lmb @gentoo-root @learnitall @aditighag @julianwiedmann @michi-covalent