Skip to content

Conversation

julianwiedmann
Copy link
Member

These features are available as of kernel 5.1, which means we can just probe once at startup and bail out.

Simplify the datapath and bandwidth manager accordingly.

Note: from checking the RHEL8 source code (linux-4.18.0-372.32.1.el8_6) the skb->tstamp and skb->queue_mapping are writeable there as well.

@julianwiedmann julianwiedmann added area/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 Aug 21, 2024
@julianwiedmann
Copy link
Member Author

/test

These features are available as of kernel 5.1, which means we can just
probe once at startup and bail out.

Simplify the datapath and bandwidth manager accordingly.

Note: from checking the RHEL8 source code (linux-4.18.0-372.32.1.el8_6) the
skb->tstamp and skb->queue_mapping are writeable there as well.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann julianwiedmann force-pushed the 1.17-writeable-queue-mapping-v2 branch from a1d921f to 9751c23 Compare August 21, 2024 14:43
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann marked this pull request as ready for review August 22, 2024 06:26
@julianwiedmann julianwiedmann requested review from a team as code owners August 22, 2024 06:26
Copy link
Member

@borkmann borkmann left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link

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.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Sep 22, 2024
Copy link
Contributor

@rgo3 rgo3 left a comment

Choose a reason for hiding this comment

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

Do we probe for this helper because we actually use it in the datapath or do we probe for it as a general placeholder to determine if we have a 5.1+ kernel version? If yes, I think we could entirely remove the probe now, as other probes already make sure that we are on a 5.1+ kernel.

@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 Sep 25, 2024
@rgo3 rgo3 removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 25, 2024
@github-actions github-actions bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Sep 26, 2024
@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 Sep 26, 2024
@youngnick youngnick removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 30, 2024
@julianwiedmann
Copy link
Member Author

Do we probe for this helper because we actually use it in the datapath or do we probe for it as a general placeholder to determine if we have a 5.1+ kernel version? If yes, I think we could entirely remove the probe now, as other probes already make sure that we are on a 5.1+ kernel.

It's more of a probe for "does the kernel roughly have this feature set" - either because we're on a 5.1 kernel, or the patch set was backported (-> RHEL 8). I'm intentionally not touching the existing probe logic too much, this is all a bit too brittle for me :).

For instance I don't think we truly probe for "is this a 5.1+ kernel" - we just probe for random features that are expected on a 5.1 kernel. But those features could also exist on a frankenkernel with sufficient backports.

Copy link
Contributor

@rgo3 rgo3 left a comment

Choose a reason for hiding this comment

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

For instance I don't think we truly probe for "is this a 5.1+ kernel" - we just probe for random features that are expected on a 5.1 kernel. But those features could also exist on a frankenkernel with sufficient backports.

I think that's why I am wondering why we still need to probe for two different "random" features (writable skb queue mapping and dead code elimination) that would point to the same kernel version feature set (5.1+).
But I'm fine with leaving it there for now so that this PR doesn't touch which probes we execute and revisit which probes are actually still necessary at a later time.

Also agree that it's all a bit brittle, but so far we haven't been able to come up with better alternatives for some of the probes that get executed...

@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 Oct 1, 2024
@julianwiedmann
Copy link
Member Author

I think that's why I am wondering why we still need to probe for two different "random" features (writable skb queue mapping and dead code elimination) that would point to the same kernel version feature set (5.1+).

Imho to catch cases where one required feature was backported (to say kernel 4.18), but another required feature wasn't.

@julianwiedmann julianwiedmann added this pull request to the merge queue Oct 1, 2024
Merged via the queue into cilium:main with commit ff8c24d Oct 1, 2024
76 checks passed
@julianwiedmann julianwiedmann deleted the 1.17-writeable-queue-mapping-v2 branch October 1, 2024 11:02
julianwiedmann added a commit to julianwiedmann/cilium that referenced this pull request Oct 27, 2024
Cilium now requires a 5.4 kernel, remove this stale dependency from the
docs.

Note that RHEL8 also contains the relevant kernel changes, see
cilium#34491.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
github-merge-queue bot pushed a commit that referenced this pull request Oct 28, 2024
Cilium now requires a 5.4 kernel, remove this stale dependency from the
docs.

Note that RHEL8 also contains the relevant kernel changes, see
#34491.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
rastislavs pushed a commit that referenced this pull request Oct 28, 2024
[ upstream commit 56fb789 ]

Cilium now requires a 5.4 kernel, remove this stale dependency from the
docs.

Note that RHEL8 also contains the relevant kernel changes, see
#34491.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Signed-off-by: Rastislav Szabo <rastislav.szabo@isovalent.com>
github-merge-queue bot pushed a commit that referenced this pull request Oct 28, 2024
[ upstream commit 56fb789 ]

Cilium now requires a 5.4 kernel, remove this stale dependency from the
docs.

Note that RHEL8 also contains the relevant kernel changes, see
#34491.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Signed-off-by: Rastislav Szabo <rastislav.szabo@isovalent.com>
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. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants