On Thu, Jul 14, 2022 at 11:13 PM Yonghong Song <yhs@xxxxxx> wrote:
>
>
>
> 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.
>
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',
> >> [...]
