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.
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
trace connections using pinned eBPF map #2057
trace connections using pinned eBPF map #2057
Changes from all commits
a3b0086
f70c674
09ca232
2f054f0
4e251b6
62cd5f7
5f958e2
d228e08
886db13
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I am not sure if this should be a ternary flag.
If I understand correctly, the intent of defining this as a ternary flag is to lazy-load the information from the eBPF map. However, I am not sure if that should be done, because the map is an LRU.
I think we should try to load the information as soon as the server-side of the socket is created, to avoid the risk of the entry corresponding to the created socket evicted from the eBPF map. The positive side effect of making such a change would be that this can then be an ordinary boolean flag, because the "unknown" state becomes unnecessary.
WDYT?
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.
Yes exactly - but the LRU relatively small data retention is a good point indeed !
My initial idea behind the lazily-loaded ternary state was to completely isolate the tracing path from the rest of the code.
h2o_socket_is_traced
is then the only gateway to the tracing code, which is only executed once a probe is attached.This works if
h2o_socket_is_traced
isn't called too late after the arrival of the connection, so the relevant information is hopefully still within the map at this time.I'm not sure what to conclude from here. I implemented the non-ternary-state version in this commit - feel free to tell me what you think, I can cherry-pick it to this branch if you think it's a better setup 👍
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.
Thank you for the non-ternary-state version. Applied.
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.
Is the returned value a boolean? Assuming that it is, I think using 1 (true) / 0 (false) would make the code consistent.
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.
The syscall returns
0
if the key is found,-1
otherwise. As the output is going to the ternary state variable_is_traced
, we need to shift the output (1
if present within map,-1
otherwise).I changed this to a
1
if present0
otherwise setup in the non-ternary-state version discussed below.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.
I think we might call
init_ebpf_map_key
function with a UNIX socket as an argument?Assuming that that could happen, I think we might call
getsockopt
first, bail out immediately if sock_type is neither IPv4 ar IPv6.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.
I'm not sure I understand here - is the idea to early exit if we can't determine the socket type ?
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.
FTR, the problem was that we were failing to setup the eBPF key when the socket was unix socket. Fixed in 262f9c9.