-
Notifications
You must be signed in to change notification settings - Fork 3.4k
datapath: require TCP EDT support and writeable skb queue_mapping #34491
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.
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
datapath: require TCP EDT support and writeable skb queue_mapping #34491
Conversation
/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>
a1d921f
to
9751c23
Compare
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This pull request has been automatically marked as stale because it |
There was a problem hiding this 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.
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. |
There was a problem hiding this 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...
Imho to catch cases where one required feature was backported (to say kernel 4.18), but another required feature wasn't. |
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>
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>
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.