Disable failing kfunc #1432
Disable failing kfunc #1432
Conversation
viktormalik
force-pushed
the
disable-failing-kfunc
branch
from
4fa7540
to
961cf71
Compare
|
Thanks for fixing this. Looks good to me. |
src/btf.h
Outdated
| @@ -44,9 +44,11 @@ class BTF | |||
| std::unique_ptr<std::istream> get_funcs(std::regex* re, | |||
| bool params, | |||
| std::string prefix) const; | |||
| bool is_traceable_func(std::string& func_name) const; | |||
There was a problem hiding this comment.
Nit
| bool is_traceable_func(std::string& func_name) const; | |
| bool is_traceable_func(const std::string& func_name) const; |
| if (available_funs.fail()) | ||
| { | ||
| if (bt_debug != DebugLevel::kNone) | ||
| { | ||
| std::cerr << "Error while reading traceable functions from " | ||
| << kprobe_path << ": " << strerror(errno); |
There was a problem hiding this comment.
Hmm, this seems unlikely to fail. But when it does fail, kfuncs are disabled. I think this message would help user figure out what went wrong in that case. Not sure if it should be hidden behind -d flag.
There was a problem hiding this comment.
That's true, although I don't think that the message should be shown every time the user lists probes and the file is not available. I'd suggest leaving it behind -d flag and displaying a similar message when user tries to attach a kfunc and it fails due to this error. I'd implement that as a separate PR that will improve error messages for kfuncs in general.
There was a problem hiding this comment.
Why not raise an exception? That way the caller can figure out what to do with it which makes more sense for a "library" function like this imo.
If kfuncs depend on this we should probably hook it into --info too, would be confusing if bpftrace reports that kfuncs should work when it doesn.t
There was a problem hiding this comment.
Why not raise an exception? That way the caller can figure out what to do with it which makes more sense for a "library" function like this imo.
I agree that raising an exception is cleaner, however, the caller is always the same - the constructor of BTF, called from the constructor of BPFtrace, so we would end up with the same problem again. A solution could be to postpone reading of the file to the phase when the function list is actually queried, but the function doing it (BTF::get_funcs) is marked as const and I didn't want to break that.
If kfuncs depend on this we should probably hook it into
--infotoo, would be confusing if bpftrace reports that kfuncs should work when it doesn.t
I agree, will do.
viktormalik
force-pushed
the
disable-failing-kfunc
branch
from
961cf71
to
f0c411d
Compare
The list of available kfunc symbols is taken from the BTF data, however, it should not include non-traceable functions (those that are not listed in /sys/kernel/debug/tracing/available_filter_functions) as tracing of these may fail or be unstable. These are usually inlined functions or functions marked as "notrace". Resolves iovisor#1366.
Currently, when creating a BPF trampoline (in function arch_prepare_bpf_trampoline), kernel only supports saving arguments of the probed function that are passed in registers (at least for x86, which is now the only architecture that has BPF trampolines). If the function has more arguments, it is not attached, causing bpftrace to fail. This patch disables tracing of functions that have more arguments than the number passed in registers for the current architecture.
viktormalik
force-pushed
the
disable-failing-kfunc
branch
from
f0c411d
to
3454181
Compare
There is a number of functions that are listed as traceable in kfunc, but their tracing crashes bpftrace. This PR disables tracing of 2 kinds of such functions:
/sys/kernel/debug/tracing/available_filter_functions(e.g. inlined functions or functions marked as "notrace"). These should not be traceable even though there is BTF info for them (which makes them appear on the kfunc list). This fixes Inline functions are listed as traceable in kfunc #1366.Checklist
docs/reference_guide.mdCHANGELOG.md