-
Notifications
You must be signed in to change notification settings - Fork 235
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
Add an eBPF program to measure synchronous connect() calls latencies #254
Add an eBPF program to measure synchronous connect() calls latencies #254
Conversation
}; | ||
|
||
struct { | ||
__uint(type, BPF_MAP_TYPE_HASH); |
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.
BPF_MAP_TYPE_LRU_HASH
is preferred for resiliency against map getting full.
metrics: | ||
histograms: | ||
- name: connect_latency_seconds | ||
help: Latency histogram for TCP connect() syscall |
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.
If it's TCP only, then let's call the metric tcp_connect_latency_seconds
and rename the file into tcp-connect-latency.yaml
.
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.
Since we do this:
// Filter out non-blocking sockets and errors
It's probably a good idea to add blocking
in the name as well.
__type(value, u64); | ||
} connect_latency_seconds SEC(".maps"); | ||
|
||
static inline __u16 ntohs(__u16 value) { |
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.
Any reason not to use bpf_ntohs
?
|
||
bpf_probe_read(&sa, sizeof(sa), addr); | ||
|
||
if (sa.sa_family == 1) { |
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.
There are lots of families and you probably only care about AF_INET
:
I suggest you add #define AF_INET 2
and use it here.
return ((value & 0x00FF) << 8) | ((value & 0xFF00) >> 8); | ||
} | ||
|
||
SEC("kprobe/__sys_connect") |
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.
fentry
is a lot faster:
You also get:
- Function arguments in
fexit
, allowing you to key onaddr
rather than pid - No need for
BPF_CORE_READ
as BTF allows direct reads
return 0; | ||
} | ||
} else { | ||
const char debug_str[] = "Unexpected addrlen: %d, address family: %d\n"; |
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.
We generally don't leave debug statements around.
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 would recommend bpf_printk()
when you debug locally:
start_val.d_ip = BPF_CORE_READ(&v6.sin6_addr.in6_u, u6_addr32[3]); | ||
start_val.d_port = v6.sin6_port; | ||
} else { | ||
const char debug_str[] = "This is native ipv6, I'm giving up!\n"; |
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.
Please implement IPv6 as well. Normally it's in a separate map: #251.
} | ||
const char debug_str[] = "Return code is: %d\n"; | ||
bpf_trace_printk(debug_str, sizeof(debug_str), ret); | ||
struct connect_latency_key_t key = {}; |
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.
Please put all definitions at the top of the function.
struct connect_start_val_t { | ||
u64 ts; | ||
int addrlen; | ||
u32 d_ip; // Destination IPv4 address |
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.
clang-format
is not happy here
|
||
struct connect_start_val_t { | ||
u64 ts; | ||
int addrlen; |
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.
You aren't really using addrlen
.
No description provided.