Re: [PATCH 02/26] perf: Introduce mmap3 version of mmap event

From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>, Jiri Olsa <jolsa@kernel.org>,
	lkml <linux-kernel@vger.kernel.org>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>,
	Ingo Molnar <mingo@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Michael Petlan <mpetlan@redhat.com>,
	Song Liu <songliubraving@fb.com>,
	"Frank Ch. Eigler" <fche@redhat.com>,
	Ian Rogers <irogers@google.com>,
	Stephane Eranian <eranian@google.com>,
	Alexey Budankov <alexey.budankov@linux.intel.com>,
	Andi Kleen <ak@linux.intel.com>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Yonghong Song <yhs@fb.com>
Subject: Re: [PATCH 02/26] perf: Introduce mmap3 version of mmap event
Date: Mon, 14 Sep 2020 19:00:14 -0300	[thread overview]
Message-ID: <20200914220014.GH166601@kernel.org> (raw)
In-Reply-To: <20200914193907.GO1714160@krava>

Em Mon, Sep 14, 2020 at 09:39:07PM +0200, Jiri Olsa escreveu:
> On Mon, Sep 14, 2020 at 12:28:41PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Sep 14, 2020 at 02:38:27PM +0900, Namhyung Kim escreveu:
> > > On Mon, Sep 14, 2020 at 6:03 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > Add new version of mmap event. The MMAP3 record is an
> > > > augmented version of MMAP2, it adds build id value to
> > > > identify the exact binary object behind memory map:

> > > >   struct {
> > > >     struct perf_event_header header;

> > > >     u32                      pid, tid;
> > > >     u64                      addr;
> > > >     u64                      len;
> > > >     u64                      pgoff;
> > > >     u32                      maj;
> > > >     u32                      min;
> > > >     u64                      ino;
> > > >     u64                      ino_generation;
> > > >     u32                      prot, flags;
> > > >     u32                      reserved;

> > What for this reserved? its all nicely aligned already, u64 followed by
> > two u32 (prot, flags).

> > > >     u8                       buildid[20];

> > > Do we need maj, min, ino, ino_generation for mmap3 event?
> > > I think they are to compare binaries, then we can do it with
> > > build-id (and I think it'd be better)..

> > Humm, I thought MMAP2 would be a superset of MMAP and MMAP3 would be a
> > superset of MMAP2.

> > If we want to ditch useless stuff, then trow away pid, tid too, as we
> > can select those via sample_type.

> > Having said that, at this point I don't even know if adding new
> > PERF_RECORD_ that are an update for a preexisting one is the right way
> > to proceed.

> > Perhaps we should attach a BPF program to point where a mmap/munmap is
> > being done (perf_event_mmap()) and allow userspace to ask for whatever
> > it wants?  With a kprobes there right now we can implement this MMAP3
> > easily, no?

> hmm, I'm always woried about solutions based on kprobes,
> because once the function is moved/removed you're screwed
> and need to keep up with function name changes and be
> backward compatible..

Well, I'm not advocating to have it as kprobes permanently, but we can
implement it now using a kprobes, i.e. systems wouldn't have to have its
kernel updated to have this feature, but once then need, for some other
reason, to have their kernel upgraded, then perf would notice that there
is a tracepoint for that and would happily use it.

So we would be able to use that tracepoint with things like ftrace,
bpftrace, everything that knows about tracepoints, and perf would get
build-ids and whatever else is needed to have a mmap record, in the
future we could even ask for some more (or less) according to the what
is needed for some new feature.

I.e. the point wasn't about kprobes was about using BPF to state what
we want to collect when a mmap is being put in place.

- Arnaldo