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.
> >
> > 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',
> [...]
