On 7/14/22 10:25 PM, Andrii Nakryiko wrote:
On Thu, Jul 14, 2022 at 5:29 PM Yonghong Song <yhs@xxxxxx> wrote:
On 7/14/22 4:21 PM, Andrii Nakryiko wrote:
Teach libbpf to fallback to tracefs mount point (/sys/kernel/tracing) if
debugfs (/sys/kernel/debug/tracing) isn't mounted.
Suggested-by: Connor O'Brien <connoro@xxxxxxxxxx>
Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
Ack with a few suggestions below.
Acked-by: Yonghong Song <yhs@xxxxxx>
---
tools/lib/bpf/libbpf.c | 33 +++++++++++++++++++++++----------
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 68da1aca406c..4acdc174cc73 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -9828,6 +9828,19 @@ static int append_to_file(const char *file, const char *fmt, ...)
return err;
}
+#define DEBUGFS "/sys/kernel/debug/tracing"
+#define TRACEFS "/sys/kernel/tracing"
+
+static bool use_debugfs(void)
+{
+ static int has_debugfs = -1;
+
+ if (has_debugfs < 0)
+ has_debugfs = access(DEBUGFS, F_OK) == 0;
+
+ return has_debugfs == 1;
+}
+
static void gen_kprobe_legacy_event_name(char *buf, size_t buf_sz,
const char *kfunc_name, size_t offset)
{
@@ -9840,7 +9853,7 @@ static void gen_kprobe_legacy_event_name(char *buf, size_t buf_sz,
static int add_kprobe_event_legacy(const char *probe_name, bool retprobe,
const char *kfunc_name, size_t offset)
{
- const char *file = "/sys/kernel/debug/tracing/kprobe_events";
+ const char *file = use_debugfs() ? DEBUGFS"/kprobe_events" : TRACEFS"/kprobe_events";
I am wondering whether we can have a helper function to return
use_debugfs() ? DEBUGFS"/kprobe_events" : TRACEFS"/kprobe_events"
so use_debugfs() won't appear in add_kprobe_event_legacy() function.
So I'm not sure what exactly you are proposing. We have 3 different
paths involving DEBUGS/TRACEFS prefix: DEBUGFS/kprobe_events,
DEBUGFS/uprobe_events, and "%s/events/%s/%s/id where first part is
DEBUGFS/TRACEFS.
Are you proposing to add two extra helper functions effectively returning:
- use_debugfs() ? DEBUGFS"/kprobe_events" : TRACEFS"/kprobe_events"
- use_debugfs() ? DEBUGFS"/uprobe_events" : TRACEFS"/uprobe_events"
and leave the third case as is? That seems inconsistent, and extra
function just makes it slightly harder to track what actual path is
used.
In general, I've always argued for using such string constants inline
without extra #defines and I'd love to be able to still do that, but
this debugfs vs tracefs unfortunately means I can't do it. The current
approach was the closest I could come up with. But at least I don't
want to dig those even deeper unnecessarily into some extra helper
funcs.
The following is what I mean (on top of your patch):
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 4acdc174cc73..38cdeab1622d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -9841,6 +9841,18 @@ static bool use_debugfs(void)
return has_debugfs == 1;
}
+static const char *kprobe_events_path(void) {
+ return use_debugfs() ? DEBUGFS"/kprobe_events" :
TRACEFS"/kprobe_events";
+}
+
+static const char *uprobe_events_path(void) {
+ return use_debugfs() ? DEBUGFS"/uprobe_events" :
TRACEFS"/uprobe_events";
+}
+
+static const char *tracefs_path(void) {
+ return use_debugfs() ? DEBUGFS : TRACEFS;
+}
+
static void gen_kprobe_legacy_event_name(char *buf, size_t buf_sz,
const char *kfunc_name, size_t
offset)
{
@@ -9853,7 +9865,7 @@ static void gen_kprobe_legacy_event_name(char
*buf, size_t buf_sz,
static int add_kprobe_event_legacy(const char *probe_name, bool retprobe,
const char *kfunc_name, size_t offset)
{
- const char *file = use_debugfs() ? DEBUGFS"/kprobe_events" :
TRACEFS"/kprobe_events";
+ const char *file = kprobe_events_path();
return append_to_file(file, "%c:%s/%s %s+0x%zx",
retprobe ? 'r' : 'p',
@@ -9863,7 +9875,7 @@ static int add_kprobe_event_legacy(const char
*probe_name, bool retprobe,
static int remove_kprobe_event_legacy(const char *probe_name, bool
retprobe)
{
- const char *file = use_debugfs() ? DEBUGFS"/kprobe_events" :
TRACEFS"/kprobe_events";
+ const char *file = kprobe_events_path();
return append_to_file(file, "-:%s/%s", retprobe ? "kretprobes"
: "kprobes", probe_name);
}
@@ -9873,7 +9885,7 @@ static int determine_kprobe_perf_type_legacy(const
char *probe_name, bool retpro
char file[256];
snprintf(file, sizeof(file), "%s/events/%s/%s/id",
- use_debugfs() ? DEBUGFS : TRACEFS,
+ tracefs_path(),
retprobe ? "kretprobes" : "kprobes", probe_name);
return parse_uint_from_file(file, "%d\n");
@@ -10226,7 +10238,7 @@ static void gen_uprobe_legacy_event_name(char
*buf, size_t buf_sz,
static inline int add_uprobe_event_legacy(const char *probe_name, bool
retprobe,
const char *binary_path,
size_t offset)
{
- const char *file = use_debugfs() ? DEBUGFS"/uprobe_events" :
TRACEFS"/uprobe_events";
+ const char *file = uprobe_events_path();
return append_to_file(file, "%c:%s/%s %s:0x%zx",
retprobe ? 'r' : 'p',
@@ -10236,7 +10248,7 @@ static inline int add_uprobe_event_legacy(const
char *probe_name, bool retprobe,
static inline int remove_uprobe_event_legacy(const char *probe_name,
bool retprobe)
{
- const char *file = use_debugfs() ? DEBUGFS"/uprobe_events" :
TRACEFS"/uprobe_events";
+ const char *file = uprobe_events_path();
return append_to_file(file, "-:%s/%s", retprobe ? "uretprobes"
: "uprobes", probe_name);
}
@@ -10246,7 +10258,7 @@ static int
determine_uprobe_perf_type_legacy(const char *probe_name, bool retpro
char file[512];
snprintf(file, sizeof(file), "%s/events/%s/%s/id",
- use_debugfs() ? DEBUGFS : TRACEFS,
+ tracefs_path(),
retprobe ? "uretprobes" : "uprobes", probe_name);
return parse_uint_from_file(file, "%d\n");
@@ -10796,7 +10808,7 @@ static int determine_tracepoint_id(const char
*tp_category,
int ret;
ret = snprintf(file, sizeof(file), "%s/events/%s/%s/id",
- use_debugfs() ? DEBUGFS : TRACEFS,
+ tracefs_path(),
tp_category, tp_name);
if (ret < 0)
return -errno;
The goal is to hide use_debugfs() from functions
{add,remove)_kprobe_event_legacy and {add,remove)_uprobe_event_legacy.
Previously I missed the different usage of kprobe/uprobe, so now my
approach has three (inlinable) static functions instead two.
I guess your current approach should be okay then. I have acked anyway.
return append_to_file(file, "%c:%s/%s %s+0x%zx",
retprobe ? 'r' : 'p',
@@ -9850,7 +9863,7 @@ static int add_kprobe_event_legacy(const char *probe_name, bool retprobe,
static int remove_kprobe_event_legacy(const char *probe_name, bool retprobe)
{
- const char *file = "/sys/kernel/debug/tracing/kprobe_events";
+ const char *file = use_debugfs() ? DEBUGFS"/kprobe_events" : TRACEFS"/kprobe_events";
return append_to_file(file, "-:%s/%s", retprobe ? "kretprobes" : "kprobes", probe_name);
}
@@ -9859,8 +9872,8 @@ static int determine_kprobe_perf_type_legacy(const char *probe_name, bool retpro
{
char file[256];
- snprintf(file, sizeof(file),
- "/sys/kernel/debug/tracing/events/%s/%s/id",
+ snprintf(file, sizeof(file), "%s/events/%s/%s/id",
+ use_debugfs() ? DEBUGFS : TRACEFS,
The same here, a helper function can hide the details of use_debugfs().
well here I can't hide just DEBUGFS/TRACEFS parts, or are you
suggesting to move the entire snprintf() into a separate function? Not
sure I see benefits of the latter, tbh.
retprobe ? "kretprobes" : "kprobes", probe_name);
return parse_uint_from_file(file, "%d\n");
@@ -10213,7 +10226,7 @@ static void gen_uprobe_legacy_event_name(char *buf, size_t buf_sz,
static inline int add_uprobe_event_legacy(const char *probe_name, bool retprobe,
const char *binary_path, size_t offset)
{
- const char *file = "/sys/kernel/debug/tracing/uprobe_events";
+ const char *file = use_debugfs() ? DEBUGFS"/uprobe_events" : TRACEFS"/uprobe_events";
return append_to_file(file, "%c:%s/%s %s:0x%zx",
retprobe ? 'r' : 'p',
[...]
