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

[Google]

On Thu,  3 Sep 2026 21:24:07 +0800
Pengfei Li <ljdlns1987@xxxxxxxxx> wrote:

> Add a lock-free hash map (ftrace_stackmap) that deduplicates kernel
> stack traces for the ftrace ring buffer. Instead of storing full
> stack traces (80-160 bytes each) in the ring buffer for every event,
> ftrace can store a 4-byte stack_id when the stackmap option is enabled.

Thanks, I have some comments below.

[...]
> 
> 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.

> 
> Signed-off-by: Pengfei Li <lipengfei28@xxxxxxxxxx>

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

> ---
>  kernel/trace/Kconfig          |  22 +
>  kernel/trace/Makefile         |   1 +
>  kernel/trace/trace_stackmap.c | 871 ++++++++++++++++++++++++++++++++++
>  kernel/trace/trace_stackmap.h |  55 +++
>  4 files changed, 949 insertions(+)
>  create mode 100644 kernel/trace/trace_stackmap.c
>  create mode 100644 kernel/trace/trace_stackmap.h

[...]

> +/* --- Stats --- */
> +
> +static int stackmap_stat_show(struct seq_file *m, void *v)
> +{
> +	struct ftrace_stackmap *smap = m->private;
> +	u64 successes = 0, drops = 0;
> +	u32 entries;
> +	int cpu;
> +
> +	if (!smap) {
> +		seq_puts(m, "stackmap not initialized\n");
> +		return 0;
> +	}
> +

You also need down_read(&smap->reader_sem) here for serializing.

> +	entries = atomic_read(&smap->next_elt);
> +	for_each_possible_cpu(cpu) {
> +		successes += local_read(per_cpu_ptr(smap->successes, cpu));
> +		drops += local_read(per_cpu_ptr(smap->drops, cpu));
> +	}
> +
> +	seq_printf(m, "entries:      %u / %u\n", entries, smap->max_elts);
> +	seq_printf(m, "table_size:   %u\n", smap->map_size);
> +	seq_printf(m, "successes:    %llu\n", successes);
> +	seq_printf(m, "drops:        %llu\n", drops);
> +	if (successes + drops > 0) {
> +		/*
> +		 * mul_u64_u64_div_u64() uses a 128-bit intermediate, so
> +		 * (successes * 100) cannot overflow even when successes
> +		 * approaches U64_MAX on a very long trace.
> +		 */
> +		successes = mul_u64_u64_div_u64(successes, 100, successes + drops);
> +	} else {
> +		successes = 0;
> +	}
> +	seq_printf(m, "success_rate: %llu%%\n", successes);

and up_read(&smap->reader_sem) too.

> +	return 0;
> +}
> +
> +static int stackmap_stat_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, stackmap_stat_show, inode->i_private);
> +}
> +
> +const struct file_operations ftrace_stackmap_stat_fops = {
> +	.open		= stackmap_stat_open,
> +	.read		= seq_read,
> +	.llseek		= seq_lseek,
> +	.release	= single_release,
> +};
> +
> +/* --- Binary export --- */
> +
> +struct stackmap_bin_snapshot {
> +	/*
> +	 * Use u64 (not size_t) so data[] is 8-byte aligned on both
> +	 * 32-bit and 64-bit architectures. The IP array within data[]
> +	 * is accessed as u64*, which would alignment-fault on strict
> +	 * architectures (e.g. older ARM, SPARC) if data[] started at
> +	 * a 4-byte boundary.
> +	 */
> +	u64	size;
> +	char	data[];
> +};
> +
> +static int stackmap_bin_open(struct inode *inode, struct file *file)
> +{
> +	struct ftrace_stackmap *smap = inode->i_private;
> +	struct stackmap_bin_snapshot *snap;
> +	struct ftrace_stackmap_bin_header *hdr;
> +	struct ftrace_stackmap_bin_entry *e;
> +	size_t alloc_size, off;
> +	u32 nr_entries, i, nr_stacks;
> +
> +	if (!smap)
> +		return -ENODEV;
> +
> +	/*
> +	 * Serialize opens: only one snapshot may be pinned at a time
> +	 * (see @bin_open). Released in stackmap_bin_release().
> +	 */
> +	if (atomic_cmpxchg(&smap->bin_open, 0, 1) != 0)
> +		return -EBUSY;
> +
> +	/*
> +	 * Worst-case allocation size: every populated entry uses a
> +	 * full-depth stack. The (+1) gives one slack slot in case a
> +	 * concurrent insert lands between this snapshot and iteration.
> +	 * The loop below performs an explicit bounds check anyway.
> +	 *
> +	 * 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.

Thank you,

-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>