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;
Date:   Wed, 8 Jun 2022 08:40:23 -0400
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Jiri Olsa <olsajiri@gmail.com>
Cc:     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>
Subject: Re: [PATCHv2 bpf 3/3] bpf: Force cookies array to follow symbols
 sorting
Message-ID: <20220608084023.4be8ffe2@gandalf.local.home>
In-Reply-To: <YqBynO64am32z13X@krava>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

-- Steve