bpf: Enable monitor aggregation for all events in bpf_network.c #31015
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
learnitall
added
sig/datapath
gentoo-root
approved these changes
Mar 4, 2024
There was a problem hiding this comment.
LGTM.
This change helps improve resource usage by
reducing the overall number of events that the datapath emits
Do you have numbers? I.e. how much CPU usage were saved in some test, how did the packet rate improve?
IPSec
Nit: the correct spelling is IPsec. (Although it looks weird, and the spelling varies even in the related RFCs.)
I didn't pull numbers for this change just to save some time and I figured it would be ok since it's fairly similar to similar PRs that were made before.
Oh thank you for letting me know! I had no idea 😄. |
learnitall
force-pushed
the
pr/learnitall/add-monitor-agg-network
branch
from
5fc9dd7
to
5bce5f2
Compare
|
/test |
This commit adjusts the usage of send_trace_notify in bpf_network.c to enable monitor aggregation for all events emitted at this observation point in the datapath. This change helps improve resource usage by reducing the overall number of events that the datapath emits, while still enabling packet observability with Hubble. The events in bpf_network.c enable observability into the IPSec processing of the datapath. Before this commit, multiple other efforts have been made to increase the aggregation of events related to IPSec to reduce resource usage, see cilium#29616 and cilium#27168. These efforts were related to packets that were specifically marked as encrypted or decrypted by IPSec and did not include events in bpf_network.c that were emitted when either: (a) a plaintext packet has been received from the network, or (b) a packet was decrypted and reinserted into the stack by XFRM. Both of these events are candidates for aggregation because similar to-stack events will be emitted down the line in the datapath anyways. Additionally, these events are mainly useful for root-cause analysis or debugging and are not necessarily helpful from an overall observability standpoint. Signed-off-by: Ryan Drew <ryan.drew@isovalent.com>
learnitall
force-pushed
the
pr/learnitall/add-monitor-agg-network
branch
from
5bce5f2
to
83210bd
Compare
|
/test |
This was referenced Mar 7, 2024
maintainer-s-little-helper
bot
added
the
ready-to-merge
jibi
added
backport-pending/1.13
maintainer-s-little-helper
bot
moved this from Backport pending to v1.14
to Needs backport from main
in 1.14.8
maintainer-s-little-helper
bot
moved this from Backport pending to v1.13
to Needs backport from main
in 1.13.13
jibi
added
backport-pending/1.13
maintainer-s-little-helper
bot
moved this from Needs backport from main
to Backport pending to v1.14
in 1.14.8
jibi
added
backport-pending/1.15
github-actions
bot
added
backport-done/1.13
jrajahalme
moved this from Needs backport from main
to Backport done to v1.15
in 1.15.3
jrajahalme
moved this from Backport pending to v1.14
to Backport done to v1.14
in 1.14.9
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.
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXXline if the commit addresses a particularGitHub issue.
Fixes: <commit-id>tag, thenplease add the commit author[s] as reviewer[s] to this issue.
This commit adjusts the usage of send_trace_notify in bpf_network.c to enable monitor aggregation for all events emitted at this observation point in the datapath. This change helps improve resource usage by reducing the overall number of events that the datapath emits, while still enabling packet observability with Hubble.
The events in bpf_network.c enable observability into the IPSec processing of the datapath. Before this commit, multiple other efforts have been made to increase the aggregation of events related to IPSec to reduce resource usage, see #29616 and #27168. These efforts were related to packets that were specifically marked as encrypted or decrypted by IPSec and did not include events in bpf_network.c that were emitted when either: (a) a plaintext packet has been received from the network, or (b) a packet was decrypted and reinserted into the stack by XFRM. Both of these events are candidates for aggregation because similar to-stack events will be emitted down the line in the datapath anyways. Additionally, these events are mainly useful for root-cause analysis or debugging and are not necessarily helpful from an overall observability standpoint.