* [PATCH bpf-next] libbpf: fallback to tracefs mount point if debugfs is not mounted
@ 2022-07-14 23:21 Andrii Nakryiko
2022-07-15 0:29 ` Yonghong Song
0 siblings, 1 reply; 5+ messages in thread
From: Andrii Nakryiko @ 2022-07-14 23:21 UTC (permalink / raw)
To: bpf, ast, daniel; +Cc: andrii, kernel-team, Connor O'Brien
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@google.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
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";
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,
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',
@@ -10223,7 +10236,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 = "/sys/kernel/debug/tracing/uprobe_events";
+ const char *file = use_debugfs() ? DEBUGFS"/uprobe_events" : TRACEFS"/uprobe_events";
return append_to_file(file, "-:%s/%s", retprobe ? "uretprobes" : "uprobes", probe_name);
}
@@ -10232,8 +10245,8 @@ static int determine_uprobe_perf_type_legacy(const char *probe_name, bool retpro
{
char file[512];
- 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,
retprobe ? "uretprobes" : "uprobes", probe_name);
return parse_uint_from_file(file, "%d\n");
@@ -10782,8 +10795,8 @@ static int determine_tracepoint_id(const char *tp_category,
char file[PATH_MAX];
int ret;
- ret = snprintf(file, sizeof(file),
- "/sys/kernel/debug/tracing/events/%s/%s/id",
+ ret = snprintf(file, sizeof(file), "%s/events/%s/%s/id",
+ use_debugfs() ? DEBUGFS : TRACEFS,
tp_category, tp_name);
if (ret < 0)
return -errno;
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] libbpf: fallback to tracefs mount point if debugfs is not mounted
2022-07-14 23:21 [PATCH bpf-next] libbpf: fallback to tracefs mount point if debugfs is not mounted Andrii Nakryiko
@ 2022-07-15 0:29 ` Yonghong Song
2022-07-15 5:25 ` Andrii Nakryiko
0 siblings, 1 reply; 5+ messages in thread
From: Yonghong Song @ 2022-07-15 0:29 UTC (permalink / raw)
To: Andrii Nakryiko, bpf, ast, daniel; +Cc: kernel-team, Connor O'Brien
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@google.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Ack with a few suggestions below.
Acked-by: Yonghong Song <yhs@fb.com>
> ---
> 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.
>
> 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().
> 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',
[...]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] libbpf: fallback to tracefs mount point if debugfs is not mounted
2022-07-15 0:29 ` Yonghong Song
@ 2022-07-15 5:25 ` Andrii Nakryiko
2022-07-15 6:12 ` Yonghong Song
0 siblings, 1 reply; 5+ messages in thread
From: Andrii Nakryiko @ 2022-07-15 5:25 UTC (permalink / raw)
To: Yonghong Song
Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann,
Kernel Team, Connor O'Brien
On Thu, Jul 14, 2022 at 5:29 PM Yonghong Song <yhs@fb.com> 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@google.com>
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>
> Ack with a few suggestions below.
>
> Acked-by: Yonghong Song <yhs@fb.com>
>
> > ---
> > 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.
> >
> > 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',
> [...]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] libbpf: fallback to tracefs mount point if debugfs is not mounted
2022-07-15 5:25 ` Andrii Nakryiko
@ 2022-07-15 6:12 ` Yonghong Song
2022-07-15 18:46 ` Andrii Nakryiko
0 siblings, 1 reply; 5+ messages in thread
From: Yonghong Song @ 2022-07-15 6:12 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann,
Kernel Team, Connor O'Brien
On 7/14/22 10:25 PM, Andrii Nakryiko wrote:
> On Thu, Jul 14, 2022 at 5:29 PM Yonghong Song <yhs@fb.com> 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@google.com>
>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>>
>> Ack with a few suggestions below.
>>
>> Acked-by: Yonghong Song <yhs@fb.com>
>>
>>> ---
>>> 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',
>> [...]
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] libbpf: fallback to tracefs mount point if debugfs is not mounted
2022-07-15 6:12 ` Yonghong Song
@ 2022-07-15 18:46 ` Andrii Nakryiko
0 siblings, 0 replies; 5+ messages in thread
From: Andrii Nakryiko @ 2022-07-15 18:46 UTC (permalink / raw)
To: Yonghong Song
Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann,
Kernel Team, Connor O'Brien
On Thu, Jul 14, 2022 at 11:13 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 7/14/22 10:25 PM, Andrii Nakryiko wrote:
> > On Thu, Jul 14, 2022 at 5:29 PM Yonghong Song <yhs@fb.com> 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@google.com>
> >>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> >>
> >> Ack with a few suggestions below.
> >>
> >> Acked-by: Yonghong Song <yhs@fb.com>
> >>
> >>> ---
> >>> 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.
>
Ok, I'll send v2 with use_debugfs() hidden.
> >
> >>>
> >>> 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',
> >> [...]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-15 18:46 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-14 23:21 [PATCH bpf-next] libbpf: fallback to tracefs mount point if debugfs is not mounted Andrii Nakryiko
2022-07-15 0:29 ` Yonghong Song
2022-07-15 5:25 ` Andrii Nakryiko
2022-07-15 6:12 ` Yonghong Song
2022-07-15 18:46 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).