MIME-Version: 1.0
References: <20230201135737.800527-1-jolsa@kernel.org> <Y+UAsr8A+xT0bUY/@krava>
In-Reply-To: <Y+UAsr8A+xT0bUY/@krava>
From:   Namhyung Kim <namhyung@gmail.com>
Date:   Thu, 9 Feb 2023 11:38:13 -0800
Message-ID: <CAM9d7cgrwyzrnDkkU2sAExonKbjSrs=p7Qyr=cww2zg4DTDBFw@mail.gmail.com>
Subject: Re: [RFC 0/5] mm/bpf/perf: Store build id in file object
To:     Jiri Olsa <olsajiri@gmail.com>
Cc:     Alexei Starovoitov <ast@kernel.org>,
        Andrii Nakryiko <andrii@kernel.org>,
        Hao Luo <haoluo@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Matthew Wilcox <willy@infradead.org>, bpf@vger.kernel.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, linux-perf-users@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>,
        Stanislav Fomichev <sdf@google.com>,
        Daniel Borkmann <daniel@iogearbox.net>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Jiri,

On Thu, Feb 9, 2023 at 6:25 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Wed, Feb 01, 2023 at 02:57:32PM +0100, Jiri Olsa wrote:
> > hi,
> > we have a use cases for bpf programs to use binary file's build id.
> >
> > After some attempts to add helpers/kfuncs [1] [2] Andrii had an idea [3]
> > to store build id directly in the file object. That would solve our use
> > case and might be beneficial for other profiling/tracing use cases with
> > bpf programs.
> >
> > This RFC patchset adds new config CONFIG_FILE_BUILD_ID option, which adds
> > build id object pointer to the file object when enabled. The build id is
> > read/populated when the file is mmap-ed.
> >
> > I also added bpf and perf changes that would benefit from this.

Thanks for working on this!

> >
> > I'm not sure what's the policy on adding stuff to file object, so apologies
> > if that's out of line. I'm open to any feedback or suggestions if there's
> > better place or way to do this.
>
> hi,
> Matthew suggested on irc to consider inode for storing build id

Yeah, that's my idea too.

>
> I tried that and it seems to have better stats wrt allocated build
> id objects, because inode is being shared among file objects
>
> I took /proc/slabinfo output after running bpf tests
>
> - build id stored in file:
>
>   # name            <active_objs> <num_objs> <objsize> <objperslab> <pagesperslab> : tunables <limit> <batchcount> <sharedfactor> : slabdata <active_slabs> <num_slabs> <sharedavail>
>   build_id             668    775    160   25    1 : tunables    0    0    0 : slabdata     31     31      0
>
> - build id stored in inode:
>
>   # name            <active_objs> <num_objs> <objsize> <objperslab> <pagesperslab> : tunables <limit> <batchcount> <sharedfactor> : slabdata <active_slabs> <num_slabs> <sharedavail>
>   build_id             222    225    160   25    1 : tunables    0    0    0 : slabdata      9      9      0

Cool!

>
>
> I'm stranger to inode/fs/mm code so I'll spend some time checking on
> what I possibly broke in there before I send it, but I'd appreciate
> any early feedback ;-)
>
> the code is in here:
>   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
>   inode_build_id
>
> I'll send another version with inode if there's no objection

I'll take a look.

Thanks,
Namhyung


> >
> > [1] https://lore.kernel.org/bpf/20221108222027.3409437-1-jolsa@kernel.org/
> > [2] https://lore.kernel.org/bpf/20221128132915.141211-1-jolsa@kernel.org/
> > [3] https://lore.kernel.org/bpf/CAEf4BzaZCUoxN_X2ALXwQeFTCwtL17R4P_B_-hUCcidfyO2xyQ@mail.gmail.com/
> > ---
> > Jiri Olsa (5):
> >       mm: Store build id in file object
> >       bpf: Use file object build id in stackmap
> >       perf: Use file object build id in perf_event_mmap_event
> >       selftests/bpf: Add file_build_id test
> >       selftests/bpf: Add iter_task_vma_buildid test
> >
> >  fs/file_table.c                                               |  3 +++
> >  include/linux/buildid.h                                       | 17 +++++++++++++++++
> >  include/linux/fs.h                                            |  3 +++
> >  kernel/bpf/stackmap.c                                         |  8 ++++++++
> >  kernel/events/core.c                                          | 43 +++++++++++++++++++++++++++++++++++++++----
> >  lib/buildid.c                                                 | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >  mm/Kconfig                                                    |  7 +++++++
> >  mm/mmap.c                                                     | 15 +++++++++++++++
> >  tools/testing/selftests/bpf/prog_tests/bpf_iter.c             | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tools/testing/selftests/bpf/prog_tests/file_build_id.c        | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tools/testing/selftests/bpf/progs/bpf_iter_task_vma_buildid.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  tools/testing/selftests/bpf/progs/file_build_id.c             | 34 ++++++++++++++++++++++++++++++++++
> >  tools/testing/selftests/bpf/trace_helpers.c                   | 35 +++++++++++++++++++++++++++++++++++
> >  tools/testing/selftests/bpf/trace_helpers.h                   |  1 +
> >  14 files changed, 413 insertions(+), 4 deletions(-)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/file_build_id.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_task_vma_buildid.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/file_build_id.c