Re: [RFC PATCH v6 1/3] trace: add lock-free stackmap for stack trace deduplication

[Pengfei Li]

On Tue, 08 Sep 2026 10:21:40 +0900 Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> wrote:
> > Kernel command line parameter:
> > - ftrace_stackmap.bits=N: set map capacity (2^N unique stacks,
> >   range 10-18, default 14)
>
> Ah, this kernel cmdline parameter is also be a separated patch,
> because this is not fundamentary needed.

Right, it is not needed for the basic functionality. In v7 the core
patch hardcodes the default capacity, and a later patch adds the
early_param along with the [10, 18] clamp and the memory-footprint
documentation that goes with it.

> It is OK to use an official address for SoB, but to make sure this
> address work, please at least Cc to this address.

lipengfei28@xxxxxxxxxx is on the Cc list of this series and receives
the list traffic; it is a working address. It will stay on Cc for v7
and any follow-up.

> > +	if (!smap) {
> > +		seq_puts(m, "stackmap not initialized\n");
> > +		return 0;
> > +	}
> > +
>
> You also need down_read(&smap->reader_sem) here for serializing.
[...]
> > +	seq_printf(m, "success_rate: %llu%%\n", successes);
>
> and up_read(&smap->reader_sem) too.

Correct, this is a real hole. Reset clears next_elt and the per-CPU
successes/drops counters under the write side of reader_sem, so an
unserialized stat read can straddle it and mix pre- and post-reset
values -- for instance a non-zero entries count next to counters that
have already been zeroed. v7 takes the rwsem for read around the whole
sampling and formatting block.

> > +	 * At bits=18 this caps at ~135 MB. The file is mode 0440
> > +	 * (TRACE_MODE_READ), so only privileged users can open it.
>
> Hmm, this is too huge to make a copy inside the kernel.
> If the stackmap is only increasing, and can avoid resetting by
> reader_sem, what about rewriting this as a raw-output mode of
> seq_file?
> You can use seq_write() to seq_file buffer.

Agreed on both counts: the copy is far too large, and seq_file with
seq_write() removes the need for it entirely. The element pool only
grows and slots are never recycled, so an iterator can walk the table
in index order and emit each populated entry through seq_write() as it
goes, holding reader_sem for read across each pass the way the text
export already does. That drops the per-open cost to the seq_file
buffer, and the header layout and version stay as they are: open()
counts the populated entries once to fill nr_stacks, which is a plain
memory scan of the table rather than a copy of it.

One caveat on relying on reader_sem alone: seq_file releases it between
read() calls, so a reset landing between two reads of the same fd would
otherwise splice two generations of the map into a single output stream.
For the text export that is merely confusing output, but a binary
consumer would silently parse it as one coherent dump. So the reworked
export also carries a generation counter, bumped by reset; open()
records it along with the entry count, and each pass revalidates it and
fails the read if it changed. A reader that raced a reset gets an error
and can retry instead of receiving a spliced dump.

Pengfei