Add documentation for Wireguard Threaded NAPI #9440
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
marvin-tigera
added
release-note-required
5 tasks
|
cc: @mazdakn |
|
Hey @mazdakn did I put these docs in the right spot? |
|
@jrcichra thanks for your patience. Yes, this is the right place. We are discussing internally about this PR. |
ctauchen
reviewed
Nov 22, 2024
| // WireguardThreadingEnabled controls whether Wireguard has NAPI threading enabled. [Default: false] | ||
| // WireguardThreadingEnabled controls whether Wireguard has Threaded NAPI enabled. [Default: false] | ||
| // This increases the maximum number of packets a Wireguard interface can process. | ||
| // There is a known issue https://lore.kernel.org/netdev/CALrw=nEoT2emQ0OAYCjM1d_6Xe_kNLSZ6dhjb5FxrLFYh4kozA@mail.gmail.com/T/ with this setting |
There was a problem hiding this comment.
@fasaxc I think including a link in the description text is worthwhile. But we don't seen to have anything like this currently.
[1] Are there reasons not to include a link here, or in these descriptions generally?
[2] What would be a good way to format these from source so we turn them into proper links in the site?
There was a problem hiding this comment.
I converted the links to use markdown instead. If there's a way to run the docs with this version of the FelixConfigurationSpec docs embedded I can test that markdown will work.
Comment on lines
778
to
784
| // WireguardThreadingEnabled controls whether Wireguard has Threaded NAPI enabled. [Default: false] | ||
| // This increases the maximum number of packets a Wireguard interface can process. | ||
| // There is a known issue https://lore.kernel.org/netdev/CALrw=nEoT2emQ0OAYCjM1d_6Xe_kNLSZ6dhjb5FxrLFYh4kozA@mail.gmail.com/T/ with this setting | ||
| // that may cause NAPI to get stuck holding the global `rtnl_mutex` when a peer is removed. | ||
| // Wireguard peers are removed during node reboots. | ||
| // Kernels which include this patch: https://lore.kernel.org/netdev/20240228121000.526645-2-bigeasy@linutronix.de/ are able to recover after a node drain. | ||
| // This feature should only be considered if you have high packets per second workloads that are causing dropping packets due to a saturated `softirq` CPU core. |
There was a problem hiding this comment.
Suggested change
| // WireguardThreadingEnabled controls whether Wireguard has Threaded NAPI enabled. [Default: false] | |
| // This increases the maximum number of packets a Wireguard interface can process. | |
| // There is a known issue https://lore.kernel.org/netdev/CALrw=nEoT2emQ0OAYCjM1d_6Xe_kNLSZ6dhjb5FxrLFYh4kozA@mail.gmail.com/T/ with this setting | |
| // that may cause NAPI to get stuck holding the global `rtnl_mutex` when a peer is removed. | |
| // Wireguard peers are removed during node reboots. | |
| // Kernels which include this patch: https://lore.kernel.org/netdev/20240228121000.526645-2-bigeasy@linutronix.de/ are able to recover after a node drain. | |
| // This feature should only be considered if you have high packets per second workloads that are causing dropping packets due to a saturated `softirq` CPU core. | |
| // WireguardThreadingEnabled controls whether Wireguard has Threaded NAPI enabled. [Default: false] | |
| // This increases the maximum number of packets a Wireguard interface can process. | |
| // Consider threaded NAPI only if you have high packets per second workloads that are causing dropping packets due to a saturated `softirq` CPU core. | |
| // There is a known issue https://lore.kernel.org/netdev/CALrw=nEoT2emQ0OAYCjM1d_6Xe_kNLSZ6dhjb5FxrLFYh4kozA@mail.gmail.com/T/ with this setting | |
| // that may cause NAPI to get stuck holding the global `rtnl_mutex` when a peer is removed. | |
| // Workaround: Make sure your Linux kernel includes this patch: https://lore.kernel.org/netdev/20240228121000.526645-2-bigeasy@linutronix.de/. |
Some edits for clarity and concision. A few other ideas:
- If there's a patch for this, do we expect this to be fixed in most distros? For one, it would be useful to specify major distro versions that are clear (for example: run Ubuntu 20.02 or later). Second, if it's likely to get distributed widely, this note may go stale very quickly.
- I removed a clause from your original, but I assumed you meant the same thing when you spoke of node drain and node reboot. If you intend for them to be different, let me know.
There was a problem hiding this comment.
I included the Github link to the patch and it shows the tags that include the commit. It is in the main kernel so it may go stale.
Yes I meant the same thing. Your refactor of the docs looks good!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Type: Documentation
Why: Document the caveats to using Wireguard Threaded NAPI. This was a better spot than in the docs repo.
How: I edited the code comment and ran
make generate.Affects: CRDs, OpenAPI and autogenerated docs
Related issues/PRs
documents #9260
Todos
Release Note
Reminder for the reviewer
Make sure that this PR has the correct labels and milestone set.
Every PR needs one
docs-*label.docs-pr-required: This change requires a change to the documentation that has not been completed yet.docs-completed: This change has all necessary documentation completed.docs-not-required: This change has no user-facing impact and requires no docs.Every PR needs one
release-note-*label.release-note-required: This PR has user-facing changes. Most PRs should have this label.release-note-not-required: This PR has no user-facing changes.Other optional labels:
cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.