datapath: Introduce helpers for __ctx_is checks #23820

spacewander · 2023-02-16T14:20:04Z

Only macros in function body are updated so it doesn't affect function definitions.

Please ensure your pull request adheres to the following guidelines:

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
If your commit description contains a Fixes: <commit-id> tag, then
please add the commit author[s] as reviewer[s] to this issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Thanks for contributing!

borkmann · 2023-02-17T10:57:29Z

bpf/lib/qm.h

@@ -8,7 +8,10 @@

 static inline void reset_queue_mapping(struct __ctx_buff *ctx __maybe_unused)
 {
-#if defined(RESET_QUEUES) && __ctx_is == __ctx_skb
+#if defined(RESET_QUEUES)


nit: ifdef RESET_QUEUES

borkmann · 2023-02-17T10:58:38Z

bpf/lib/nodeport.h

+				 * request.
+				 */
+				return CTX_ACT_OK;
+			}


nit: you could move the comment above the if (ctx_is_xdp()), and then just

if (ctx_is_xdp()) return CTX_ACT_OK;

without the braces as you have in IPv6 path.

spacewander · 2023-02-25T02:48:43Z

@borkmann @NikAleksandrov
Will this PR get accepted?

sayboras · 2023-02-25T22:30:26Z

/test

spacewander · 2023-03-01T07:43:22Z

interesting. The verifier failed with 5.15 but passed in kernel earlier and later than it.
https://github.com/cilium/cilium/actions/runs/4272121832/jobs/7437056192

I tried to set up a vm box to test it locally, but I got kernel version 5.17.5-300.fc36.x86_64 not supported by verifier complexity tests error. Any suggestion?

bpf/lib/nodeport.h

pchaigno · 2023-04-10T04:36:15Z

/test

borkmann

lgtm

joestringer · 2023-05-05T17:18:51Z

Reviews are in, but the PR is now out-of-date with the main branch and many of the tests failed (which we can't see the results any more due to expiry/timeouts.)

Next steps should be:

@spacewander please rebase the PR against the tip of main branch
Once the PR is rebased, a Cilium organization member can re-run the tests with /test
If the tests pass, this can be merged.

maintainer-s-little-helper · 2023-05-06T12:17:41Z

Commit 25d732654c75fb45269861f8d4a7e997f368ad70 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

github-actions · 2023-06-08T02:05:03Z

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

joestringer · 2023-06-15T16:18:11Z

Sorry, this slipped through my review queue. Unfortunately the branch is out-of-date again. Also, the way that this PR has a "merge" commit breaks the merging - could you rebase without a merge commit against the latest upstream/main? Then we can follow up to get this merged. Feel free to ping me on Slack in #development channel if I'm taking too long to respond, it might be just that I lost track of the PR again.

Only macros in function body are updated so it doesn't affect function definitions. Fixes: cilium#23008 Signed-off-by: spacewander <spacewanderlzx@gmail.com>

spacewander · 2023-06-16T03:07:36Z

Sorry, this slipped through my review queue. Unfortunately the branch is out-of-date again. Also, the way that this PR has a "merge" commit breaks the merging - could you rebase without a merge commit against the latest upstream/main? Then we can follow up to get this merged. Feel free to ping me on Slack in #development channel if I'm taking too long to respond, it might be just that I lost track of the PR again.

Done

sayboras · 2023-06-19T12:45:20Z

/test

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 16, 2023

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Feb 16, 2023

spacewander force-pushed the lkl branch 3 times, most recently from b7fec34 to 1e948ad Compare February 16, 2023 15:28

spacewander marked this pull request as ready for review February 17, 2023 01:38

spacewander requested a review from a team as a code owner February 17, 2023 01:38

spacewander requested a review from NikAleksandrov February 17, 2023 01:38

borkmann reviewed Feb 17, 2023

View reviewed changes

spacewander force-pushed the lkl branch from 1e948ad to ddbabd8 Compare February 17, 2023 11:25

NikAleksandrov approved these changes Feb 18, 2023

View reviewed changes

sayboras added the release-note/misc This PR makes changes that have no direct user impact. label Feb 25, 2023

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 25, 2023

sayboras requested a review from borkmann February 25, 2023 22:30

borkmann reviewed Mar 8, 2023

View reviewed changes

bpf/lib/nodeport.h Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Apr 10, 2023

pchaigno requested a review from borkmann April 10, 2023 04:35

pchaigno removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Apr 10, 2023

borkmann approved these changes Apr 21, 2023

View reviewed changes

joestringer added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label May 5, 2023

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 6, 2023

spacewander force-pushed the lkl branch from 25d7326 to 596deda Compare May 8, 2023 14:07

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 8, 2023

github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jun 8, 2023

joestringer self-assigned this Jun 15, 2023

github-actions bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jun 16, 2023

spacewander force-pushed the lkl branch from 596deda to eb4a738 Compare June 16, 2023 02:57

datapath: Introduce helpers for __ctx_is checks

73e9eed

Only macros in function body are updated so it doesn't affect function definitions. Fixes: cilium#23008 Signed-off-by: spacewander <spacewanderlzx@gmail.com>

spacewander force-pushed the lkl branch from eb4a738 to 73e9eed Compare June 16, 2023 03:06

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 Jun 19, 2023

sayboras removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jun 19, 2023

borkmann merged commit c1dffab into cilium:main Jun 20, 2023
61 of 62 checks passed

joestringer mentioned this pull request Jun 28, 2023

Prepare for release v1.14.0-rc.0 #26544

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

datapath: Introduce helpers for __ctx_is checks #23820

datapath: Introduce helpers for __ctx_is checks #23820

spacewander commented Feb 16, 2023

borkmann Feb 17, 2023

borkmann Feb 17, 2023 •

edited

spacewander commented Feb 25, 2023

sayboras commented Feb 25, 2023

spacewander commented Mar 1, 2023

This comment was marked as resolved.

pchaigno commented Apr 10, 2023

borkmann left a comment

joestringer commented May 5, 2023

maintainer-s-little-helper bot commented May 6, 2023

github-actions bot commented Jun 8, 2023

joestringer commented Jun 15, 2023

spacewander commented Jun 16, 2023

sayboras commented Jun 19, 2023

datapath: Introduce helpers for __ctx_is checks #23820

datapath: Introduce helpers for __ctx_is checks #23820

Conversation

spacewander commented Feb 16, 2023

borkmann Feb 17, 2023

Choose a reason for hiding this comment

borkmann Feb 17, 2023 • edited

Choose a reason for hiding this comment

spacewander commented Feb 25, 2023

sayboras commented Feb 25, 2023

spacewander commented Mar 1, 2023

This comment was marked as resolved.

pchaigno commented Apr 10, 2023

borkmann left a comment

Choose a reason for hiding this comment

joestringer commented May 5, 2023

maintainer-s-little-helper bot commented May 6, 2023

github-actions bot commented Jun 8, 2023

joestringer commented Jun 15, 2023

spacewander commented Jun 16, 2023

sayboras commented Jun 19, 2023

borkmann Feb 17, 2023 •

edited