Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220606184731.437300-1-jolsa@kernel.org> <20220606184731.437300-4-jolsa@kernel.org>
 <CAADnVQJA54Ra8+tV0e0KwSXAg93JRoiefDXWR-Lqatya5YWKpg@mail.gmail.com>
 <Yp+tTsqPOuVdjpba@krava> <CAADnVQJGoM9eqcODx2LGo-qLo0=O05gSw=iifRsWXgU0XWifAA@mail.gmail.com>
 <YqBW65t+hlWNok8e@krava> <YqBynO64am32z13X@krava> <20220608084023.4be8ffe2@gandalf.local.home>
In-Reply-To: <20220608084023.4be8ffe2@gandalf.local.home>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Wed, 8 Jun 2022 08:59:50 -0700
Message-ID: <CAEf4BzYkHkB60qPxGu0D=x-BidxObX95+1wjEYN8xsK7Dg_4cw@mail.gmail.com>
Subject: Re: [PATCHv2 bpf 3/3] bpf: Force cookies array to follow symbols sorting
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     Jiri Olsa <olsajiri@gmail.com>,
        Alexei Starovoitov <alexei.starovoitov@gmail.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Network Development <netdev@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>, lkml <linux-kernel@vger.kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@chromium.org>,
        Masami Hiramatsu <mhiramat@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jun 8, 2022 at 5:40 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 8 Jun 2022 11:57:48 +0200
> Jiri Olsa <olsajiri@gmail.com> wrote:
>
> > Steven,
> > is there a reason to show '__ftrace_invalid_address___*' symbols in
> > available_filter_functions? it seems more like debug message to me
> >
>
> Yes, because set_ftrace_filter may be set by index. That is, if schedule is
> the 43,245th entry in available_filter_functions, then you can do:
>
>   # echo 43245 > set_ftrace_filter
>   # cat set_ftrace_filter
>   schedule
>
> That index must match the array index of the entries in the function list
> internally. The reason for this is that entering a name is an O(n)
> operation, where n is the number of functions in
> available_filter_functions. If you want to enable half of those functions,
> then it takes O(n^2) to do so.
>
> I first implemented this trick to help with bisecting bad functions. That
> is, every so often a function that should be annotated with notrace, isn't
> and if it gets traced it cause the machine to reboot. To bisect this, I
> would enable half the functions at a time and enable tracing to see if it
> reboots or not, and if it does, I know that one of the half I enabled is
> the culprit, if not, it's in the other half. It would take over 5 minutes
> to enable half the functions. Where as the number trick took one second,
> not only was it O(1) per function, but it did not need to do kallsym
> lookups either. It simply enabled the function at the index.
>
> Later, libtracefs (used by trace-cmd and others) would allow regex(3)
> enabling of functions. That is, it would search available_filter_functions
> in user space, match them via normal regex, create an index of the
> functions to know where they are, and then write in those numbers to enable
> them. It's much faster than writing in strings.
>
> My original fix was to simply ignore those functions, but then it would
> make the index no longer match what got set. I noticed this while writing
> my slides for Kernel Recipes, and then fixed it.
>
> The commit you mention above even states this:
>
>       __ftrace_invalid_address___<invalid-offset>
>
>     (showing the offset that caused it to be invalid).
>
>     This is required for tools that use libtracefs (like trace-cmd does) that
>     scan the available_filter_functions and enable set_ftrace_filter and
>     set_ftrace_notrace using indexes of the function listed in the file (this
>     is a speedup, as enabling thousands of files via names is an O(n^2)
>     operation and can take minutes to complete, where the indexing takes less
>     than a second).
>
> In other words, having a placeholder is required to keep from breaking user
> space.

Would it be possible to preprocess ftrace_pages to remove such invalid
records (so that by the time we have to report
available_filter_functions there are no invalid records)? Or that data
is read-only when kernel is running?

>
> -- Steve
>
>