Re: [PATCH v1] perf sample: Make user_regs and intr_regs optional

[Namhyung Kim]

On Tue, Feb 11, 2025 at 09:46:11AM -0800, Namhyung Kim wrote:
> On Mon, Feb 10, 2025 at 08:43:40PM -0800, Ian Rogers wrote:
> > On Mon, Feb 10, 2025 at 6:51 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> > >
> > > On Mon, Feb 10, 2025 at 10:15:22AM -0800, Ian Rogers wrote:
> > > > On Mon, Jan 13, 2025 at 11:43 AM Ian Rogers <irogers@xxxxxxxxxx> wrote:
> > > > >
> > > > > The struct dump_regs contains 512 bytes of cache_regs, meaning the two
> > > > > values in perf_sample contribute 1088 bytes of its total 1384 bytes
> > > > > size. Initializing this much memory has a cost reported by Tavian
> > > > > Barnes <tavianator@xxxxxxxxxxxxxx> as about 2.5% when running `perf
> > > > > script --itrace=i0`:
> > > > > https://lore.kernel.org/lkml/d841b97b3ad2ca8bcab07e4293375fb7c32dfce7.1736618095.git.tavianator@xxxxxxxxxxxxxx/
> > > > >
> > > > > Adrian Hunter <adrian.hunter@xxxxxxxxx> replied that the zero
> > > > > initialization was necessary and couldn't simply be removed.
> > > > >
> > > > > This patch aims to strike a middle ground of still zeroing the
> > > > > perf_sample, but removing 79% of its size by make user_regs and
> > > > > intr_regs optional pointers to zalloc-ed memory. To support the
> > > > > allocation accessors are created for user_regs and intr_regs. To
> > > > > support correct cleanup perf_sample__init and perf_sample__exit
> > > > > functions are created and added throughout the code base.
> > > >
> > > > Ping. Given the memory savings and performance wins it would be nice
> > > > to see this land. Andi Kleen commented on doing a reimplementation,
> > > > which is fine but out-of-scope of what I'm doing here.
> > >
> > > Yeah, I like the core of the change.  Andi's concern is that it touches
> > > too many places.  It'd be nice if we can do that without allocating
> > > memory for regs and eliminating the perf_sample__{init,exit}.  But I'm
> > > not if it's possible.
> > 
> > Moving from no allocations to 2 possible allocations means there has
> > to be corresponding frees. Putting the frees into an __exit function
> > is the norm for this kind of cleanup. I don't see how you can move to
> > the approach presented without adding the frees and not introduce a
> > memory leak. I don't see what's actionable for me to do here.
> 
> Right, I'm inclined to merge this patch.  But I need to think a bit more
> about the Andi's approach before that.

Probably we can use a global (or per-thread) variable, but I think it
could grow to another pain point in the future.  Using __init/exit will
make it easier for potential future changes.

Thanks,
Namhyung