-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
libbpf-tools: add funcinterval #5100
Conversation
Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
.parser = parse_arg, | ||
.doc = argp_program_doc, | ||
}; | ||
struct funcinterval_bpf *obj; |
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.
Maybe you can add ensure_core_btf
and cleanup_core_btf
like
bcc/libbpf-tools/funclatency.c
Line 349 in 6acb86e
err = ensure_core_btf(&open_opts); |
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 have modified and submitted 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.
how about squashing this into the previous commit?
Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
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.
It would be good to check the readability of the code, the independence of functions, and the style of comments and code overall.
It seems that many parts of funcinterval are a subset of funclatency. You can refer to libbpf-tools/funclatency for code style.
|
||
struct { | ||
__uint(type, BPF_MAP_TYPE_HASH); | ||
__uint(max_entries, 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.
If max_entries is always one, why is a hash map needed for hists? Wouldn't an array like in funclatency.bpf.c be sufficient?
If a single array is sufficient, several logics of this patch can be changed.
Please refer to funclatency.
.parser = parse_arg, | ||
.doc = argp_program_doc, | ||
}; | ||
struct funcinterval_bpf *obj; |
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.
how about squashing this into the previous commit?
return 0; | ||
} | ||
|
||
static int split_pattern(const char *raw_pattern, enum TRACE_TYPE *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.
The function name should be descriptive enough to convey its purpose without needing to read the code.
static int split_pattern(const char *raw_pattern, enum TRACE_TYPE *type, | ||
const char **library, const char **pattern) | ||
{ | ||
const char *string1, *string2, *string3; |
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.
variable name is not descriptive.
lookup_key = next_key; | ||
} | ||
|
||
fd = bpf_map__fd(obj->maps.start); |
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.
Reusing a single variable for multiple purposes can confuse the reader.
Based on funcinterval.py from BCC by Edward Wu