Re: [RFC PATCH v4 1/3] trace: add lock-free stackmap for stack trace deduplication
Li Pengfei <[email protected]>
| Newsgroups | org.kernel.vger.linux-trace-kernel,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <[email protected]> |
From: Pengfei Li <[email protected]> On Tue, 14 Jul 2026 17:11:44 -0400 Steven Rostedt wrote: > smap->entries = vcalloc(smap->map_size, sizeof(*smap->entries)); > Make the error paths have: ... goto fail; ... Will switch both entries and elts to vcalloc(), and collapse the error paths into a single goto-fail ladder (freeing in reverse alloc order, including the drops percpu). > Do not add anonymous blocks in functions. Will move the cpu declaration to the top of ftrace_stackmap_reset(). > Really should have ftrace_stackmap_bin_entry have a flexible > array: u64 ips[]; Agreed - will add the u64 ips[] flexible member and use struct_size(e, ips, nr) in stackmap_bin_open(). On-disk layout is unchanged, so the bin format stays compatible. I'll also fold in two issues I found locally: a missing lock on one init failure path, and TRACE_STACK_ID not being handled in the function_graph output (it falls through to print_graph_comment() instead of being punted like TRACE_STACK). One design point I'd like your steer on before respinning: reset checks tracer_tracing_is_on() once under trace_types_lock, but traceon triggers (ftrace_traceon / traceon_trigger) can re-enable tracing without that lock during reset's clear phase. As far as I can tell this is a semantic-contract issue, not a memory-safety one: the map is protected by the resetting flag (get_id bails with -EINVAL and synchronize_rcu() drains in-flight callers), and the ring buffer pages aren't freed by reset, so the worst case is a non-empty / inconsistent buffer after reset rather than corruption. So I'm leaning toward documenting reset as best-effort ("stop tracing, including traceon triggers, before reset") rather than adding machinery to block the window. Does that match your view, or is there a ring-buffer-state hazard I'm missing that would justify blocking it explicitly? On the element pool: your `-l '*lock*'` run is a good illustration - the ~326K-line stack_map dump is the 16K-entry pool (bits=14) filling up quickly under broad function tracing. I'm leaning toward keeping eager allocation for v5 (it keeps the hot path free of an allocation-failure path), but I'm happy to switch to lazy allocation on the first 'echo 1 > options/stackmap' if you'd rather not pay the ~8MB resident cost when the option is never enabled. I'd keep the stack_map_bin interface as-is. Thanks, Pengfei