* Re: [PATCH -next] cgroup: increase maximum subsystem count from 16 to 32
From: Chen Ridong @ 2026-01-31 3:00 UTC (permalink / raw)
To: Michal Koutný
Cc: tj, hannes, rostedt, mhiramat, mathieu.desnoyers, inwardvessel,
shakeel.butt, cgroups, linux-kernel, linux-trace-kernel,
lujialin4
In-Reply-To: <stsf73lnkx2luuak3a7oi3q4l5axosrxogi2lncw4dkndnc2ge@3tioqa6ww5q7>
On 2026/1/30 18:22, Michal Koutný wrote:
> On Thu, Jan 29, 2026 at 05:51:33PM +0800, Chen Ridong <chenridong@huaweicloud.com> wrote:
>> We compiled with 'make allmodconfig'.
>
> A-ha.
>
>> The BUILD_BUG_ON(CGROUP_SUBSYS_COUNT > 16) macro worked correctly.
>
> Good.
>
>> Can I propose increasing the maximum number now? If we switch certain configs to
>> default N and then a new subsystem is added later, the default configuration may
>> work fine, but it will become a problem under allmodconfig — which some users
>> actually rely on.
>>
>> Besides, this shouldn't be a major change, right?
>
> I'd like there to be gradual move away from legacy controllers code
> captured in config defaults.
> Could you adjust the commit message to stress out the allmodconfig tests?
>
Sure, will update.
--
Best regards,
Ridong
^ permalink raw reply
* Re: [PATCH] bootconfig: Fix to terminate value search if it hits a newline
From: Masami Hiramatsu @ 2026-01-31 2:19 UTC (permalink / raw)
To: Julius Werner
Cc: Steven Rostedt, Masami Hiramatsu (Google), linux-trace-kernel,
LKML
In-Reply-To: <CAODwPW90ALuMy6kFPqQSj_vN7krRz6cpksAUm2WHJNYwRNxHnA@mail.gmail.com>
On Fri, 30 Jan 2026 15:26:46 -0800
Julius Werner <jwerner@google.com> wrote:
> > > Note that if the line has a comment, it still keep searching the
> > > value to the next line::
> > >
> > > foo = # comment
> > > value
>
> This seems inconsistent to me tbh. I think someone could reasonably
> write a bootconfig like this:
>
> foo = # We leave foo empty here because <reason>
> bar = value
>
> and expect it to work. There's nothing in the documentation that
> suggests empty values would behave differently if there is a comment
> behind them.
But we already accept that example in the tree.
tools/bootconfig/samples/good-array-space-comment.bconf
So it is hard to change this syntax. But without comment, the
document defines the value is terminated with a newline. Thus
it needs to be fixed.
>
> Also, I believe your patch might inadvertently break this case (which
> I think(?) is supposed to work):
>
> foo = first array value,
> second array value,
> third array value
Oops, good catch! I should add this as a good sample.
>
> Because __xbc_parse_value() returns from the first value with `&next`
> pointing at the newline behind the comma, and that is passed to the
> next iteration of __xbc_parse_value() which will now no longer skip
> it. I think an already started array may need to be treated as a
> special case here and you may want to add another
> skip_spaces()/skip_comment() to the loop in xbc_parse_array().
OK, let me fix it.
Thanks,
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCH] bootconfig: Fix to terminate value search if it hits a newline
From: Julius Werner @ 2026-01-30 23:26 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Masami Hiramatsu (Google), linux-trace-kernel, LKML
In-Reply-To: <20260130095331.5c9f754e@gandalf.local.home>
[-- Attachment #1: Type: text/plain, Size: 1027 bytes --]
> > Note that if the line has a comment, it still keep searching the
> > value to the next line::
> >
> > foo = # comment
> > value
This seems inconsistent to me tbh. I think someone could reasonably
write a bootconfig like this:
foo = # We leave foo empty here because <reason>
bar = value
and expect it to work. There's nothing in the documentation that
suggests empty values would behave differently if there is a comment
behind them.
Also, I believe your patch might inadvertently break this case (which
I think(?) is supposed to work):
foo = first array value,
second array value,
third array value
Because __xbc_parse_value() returns from the first value with `&next`
pointing at the newline behind the comma, and that is passed to the
next iteration of __xbc_parse_value() which will now no longer skip
it. I think an already started array may need to be treated as a
special case here and you may want to add another
skip_spaces()/skip_comment() to the loop in xbc_parse_array().
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5279 bytes --]
^ permalink raw reply
* Re: [PATCHv2 bpf-next 0/6] x86/fgraph,bpf: Fix ORC stack unwind from kprobe_multi
From: patchwork-bot+netdevbpf @ 2026-01-30 21:50 UTC (permalink / raw)
To: Jiri Olsa
Cc: mhiramat, rostedt, jpoimboe, peterz, bpf, linux-trace-kernel, x86,
yhs, songliubraving, andrii, mahe.tardy
In-Reply-To: <20260126211837.472802-1-jolsa@kernel.org>
Hello:
This series was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:
On Mon, 26 Jan 2026 22:18:31 +0100 you wrote:
> hi,
> Mahe reported missing function from stack trace on top of kprobe multi
> program. It turned out the latest fix [1] needs some more fixing.
>
> v2 changes:
> - keep the unwind same as for kprobes, attached function
> is part of entry probe stacktrace, not kretprobe [Steven]
> - several change in trigger bench [Andrii]
> - added selftests for standard kprobes and fentry/fexit probes [Andrii]
>
> [...]
Here is the summary with links:
- [PATCHv2,bpf-next,1/6] x86/fgraph: Fix return_to_handler regs.rsp value
https://git.kernel.org/bpf/bpf-next/c/8bc11700e0d2
- [PATCHv2,bpf-next,2/6] x86/fgraph,bpf: Switch kprobe_multi program stack unwind to hw_regs path
https://git.kernel.org/bpf/bpf-next/c/aea251799998
- [PATCHv2,bpf-next,3/6] selftests/bpf: Fix kprobe multi stacktrace_ips test
https://git.kernel.org/bpf/bpf-next/c/0207f94971e7
- [PATCHv2,bpf-next,4/6] selftests/bpf: Add stacktrace ips test for kprobe/kretprobe
https://git.kernel.org/bpf/bpf-next/c/7373f97e868a
- [PATCHv2,bpf-next,5/6] selftests/bpf: Add stacktrace ips test for fentry/fexit
https://git.kernel.org/bpf/bpf-next/c/e5d532be4a3b
- [PATCHv2,bpf-next,6/6] selftests/bpf: Allow to benchmark trigger with stacktrace
https://git.kernel.org/bpf/bpf-next/c/4173b494d93a
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH] tracing: Fix duration calculation bug (rettime - calltime) on ARM architecture
From: Steven Rostedt @ 2026-01-30 20:55 UTC (permalink / raw)
To: jempty.liang
Cc: mhiramat, mark.rutland, mathieu.desnoyers, linux-kernel,
linux-trace-kernel
In-Reply-To: <20260130015740.212343-1-imntjempty@163.com>
On Fri, 30 Jan 2026 01:57:40 +0000
"jempty.liang" <imntjempty@163.com> wrote:
> This issue is reproducible on the QEMU vexpress-a9 platform with the
> following steps:
>
> Step 1: Launch the QEMU vexpress-a9 VM
How is this unique to arm? I'm not able to duplicate this on x86, but the
code here is all generic code. What's different?
>
> ~# qemu-system-arm -M vexpress-a9 -smp 1 -m 256 -kernel zImage -dtb \
> vexpress-v2p-ca9.dtb -drive file=rootfs.ext2,if=sd,format=raw \
> -append "console=ttyAMA0,115200 rootwait" -net nic,model=lan9118 \
> -net user -serial stdio
>
> Step 2: Capture and view the trace with trace-cmd
>
> ~# trace-cmd record -p function_graph -g do_open_execat \
> --max-graph-depth 1 dd if=/dev/zero of=/tmp/1.bin \
> bs=1M count=1
> ~# trace-cmd report
Note, you are using trace-cmd that records the raw data directly thus some
of the updates to the prints below wouldn't affect this.
>
> Step 3: Observed behavior
>
> Before:
>
> dd-89 ... 5.764026: funcgraph_entry: $ 2757369004 us | do_open_execat()
> dd-89 ... 5.765222: funcgraph_entry: $ 2034790547 us | do_open_execat();
>
> After:
>
> dd-91 ... 891.945577: funcgraph_entry: ! 185.542 us | do_open_execat();
> dd-91 ... 891.946840: funcgraph_entry: + 98.625 us | do_open_execat();
You show what the wrong output is and the correct output, but you don't
mention what was the bug and how this fixes it. From the change log, it
looks like you just tried something and it worked, but do not know why it
worked.
I'd like to know what the bug was and how this fixes it. It's not obvious.
>
> Signed-off-by: jempty.liang <imntjempty@163.com>
> ---
> include/linux/ftrace.h | 2 ++
> kernel/trace/trace_entries.h | 8 ++++----
> kernel/trace/trace_functions_graph.c | 19 +++++++++----------
> 3 files changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index a3a8989e3268..52727a342273 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -1191,6 +1191,8 @@ struct ftrace_graph_ret {
> int depth;
> /* Number of functions that overran the depth limit for current task */
> unsigned int overrun;
> + unsigned long long calltime;
> + unsigned long long rettime;
> } __packed;
>
> struct fgraph_ops;
> diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
> index f6a8d29c0d76..362a757e65a2 100644
> --- a/kernel/trace/trace_entries.h
> +++ b/kernel/trace/trace_entries.h
> @@ -127,8 +127,8 @@ FTRACE_ENTRY_PACKED(funcgraph_exit, ftrace_graph_ret_entry,
> __field_packed( unsigned long, ret, retval )
> __field_packed( unsigned int, ret, depth )
> __field_packed( unsigned int, ret, overrun )
> - __field(unsigned long long, calltime )
> - __field(unsigned long long, rettime )
> + __field_packed(unsigned long long, ret, calltime)
> + __field_packed(unsigned long long, ret, rettime)
> ),
>
> F_printk("<-- %ps (%u) (start: %llx end: %llx) over: %u retval: %lx",
> @@ -149,8 +149,8 @@ FTRACE_ENTRY_PACKED(funcgraph_exit, ftrace_graph_ret_entry,
> __field_packed( unsigned long, ret, func )
> __field_packed( unsigned int, ret, depth )
> __field_packed( unsigned int, ret, overrun )
> - __field(unsigned long long, calltime )
> - __field(unsigned long long, rettime )
> + __field_packed(unsigned long long, ret, calltime)
> + __field_packed(unsigned long long, ret, rettime)
> ),
>
> F_printk("<-- %ps (%u) (start: %llx end: %llx) over: %u",
> diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> index 1de6f1573621..5798aeb31f05 100644
> --- a/kernel/trace/trace_functions_graph.c
> +++ b/kernel/trace/trace_functions_graph.c
> @@ -317,6 +317,8 @@ __trace_graph_function(struct trace_array *tr,
> struct ftrace_graph_ret ret = {
> .func = ip,
> .depth = 0,
> + .calltime = time,
> + .rettime = time,
If you assign here, then you should change the line below this code.
> };
>
> __trace_graph_entry(tr, &ent, trace_ctx);
The next line is this:
__trace_graph_return(tr, &ret, trace_ctx, time, time);
Where that function has:
entry->ret = *trace;
entry->ret.calltime = calltime;
entry->ret.rettime = rettime;
That is, ret is assigned "*trace" which includes the calltime and rettime,
and now it's doing more work to override it with the current data?
> @@ -346,8 +348,8 @@ void __trace_graph_return(struct trace_array *tr,
> return;
> entry = ring_buffer_event_data(event);
> entry->ret = *trace;
> - entry->calltime = calltime;
> - entry->rettime = rettime;
> + entry->ret.calltime = calltime;
> + entry->ret.rettime = rettime;
> trace_buffer_unlock_commit_nostack(buffer, event);
> }
>
> @@ -372,10 +374,9 @@ void trace_graph_return(struct ftrace_graph_ret *trace,
> struct trace_array *tr = gops->private;
> struct fgraph_times *ftimes;
> unsigned int trace_ctx;
> - u64 calltime, rettime;
> int size;
>
> - rettime = trace_clock_local();
> + trace->rettime = trace_clock_local();
>
> ftrace_graph_addr_finish(gops, trace);
>
> @@ -390,10 +391,10 @@ void trace_graph_return(struct ftrace_graph_ret *trace,
>
> handle_nosleeptime(tr, trace, ftimes, size);
>
> - calltime = ftimes->calltime;
> + trace->calltime = ftimes->calltime;
>
> trace_ctx = tracing_gen_ctx();
> - __trace_graph_return(tr, trace, trace_ctx, calltime, rettime);
> + __trace_graph_return(tr, trace, trace_ctx, trace->calltime, trace->rettime);
And here again, the passed in calltime and rettime is just being
overwritten.
> }
>
> static void trace_graph_thresh_return(struct ftrace_graph_ret *trace,
> @@ -956,7 +957,7 @@ print_graph_entry_leaf(struct trace_iterator *iter,
>
> graph_ret = &ret_entry->ret;
> call = &entry->graph_ent;
> - duration = ret_entry->rettime - ret_entry->calltime;
> + duration = graph_ret->rettime - graph_ret->calltime;
>
> if (data) {
> struct fgraph_cpu_data *cpu_data;
> @@ -1280,9 +1281,7 @@ print_graph_return(struct ftrace_graph_ret_entry *retentry, struct trace_seq *s,
> u32 flags)
> {
> struct ftrace_graph_ret *trace = &retentry->ret;
> - u64 calltime = retentry->calltime;
> - u64 rettime = retentry->rettime;
> - unsigned long long duration = rettime - calltime;
> + unsigned long long duration = trace->rettime - trace->calltime;
> struct fgraph_data *data = iter->private;
> struct trace_array *tr = iter->tr;
> unsigned long func;
Now moving rettime and calltime into ftrace_graph_ret may be a nice
cleanup, but you are saying this is a fix. What was the bug, and what did
this fix?
-- Steve
^ permalink raw reply
* Re: [PATCH v2] tracing/dma: Cap dma_map_sg tracepoint arrays to prevent buffer overflow
From: Steven Rostedt @ 2026-01-30 16:57 UTC (permalink / raw)
To: Sean Anderson
Cc: Deepanshu Kartikey, mhiramat, mathieu.desnoyers, ptesarik,
m.szyprowski, jgg, leon, kbusch, linux-kernel, linux-trace-kernel,
syzbot+28cea38c382fd15e751a
In-Reply-To: <d6c7d83c-687b-4844-b515-9f7342278628@linux.dev>
On Fri, 30 Jan 2026 11:27:47 -0500
Sean Anderson <sean.anderson@linux.dev> wrote:
> Reviwed-by: Sean Anderson <sean.anderson@linux.dev>
>
> Although it's a bit unusual that there's no limit on dynamic arrays like there is for %*ph.
Dynamic arrays are done in fast paths. I rather not add an extra branch to
test cases where its most likely not going to exceed the limit.
-- Steve
^ permalink raw reply
* Re: [PATCH v2] tracing/dma: Cap dma_map_sg tracepoint arrays to prevent buffer overflow
From: Sean Anderson @ 2026-01-30 16:27 UTC (permalink / raw)
To: Deepanshu Kartikey, rostedt, mhiramat, mathieu.desnoyers,
ptesarik
Cc: m.szyprowski, jgg, leon, kbusch, linux-kernel, linux-trace-kernel,
syzbot+28cea38c382fd15e751a
In-Reply-To: <20260130155215.69737-1-kartikey406@gmail.com>
On 1/30/26 10:52, Deepanshu Kartikey wrote:
> The dma_map_sg tracepoint can trigger a perf buffer overflow when
> tracing large scatter-gather lists. With devices like virtio-gpu
> creating large DRM buffers, nents can exceed 1000 entries, resulting
> in:
>
> phys_addrs: 1000 * 8 bytes = 8,000 bytes
> dma_addrs: 1000 * 8 bytes = 8,000 bytes
> lengths: 1000 * 4 bytes = 4,000 bytes
> Total: ~20,000 bytes
>
> This exceeds PERF_MAX_TRACE_SIZE (8192 bytes), causing:
>
> WARNING: CPU: 0 PID: 5497 at kernel/trace/trace_event_perf.c:405
> perf buffer not large enough, wanted 24620, have 8192
>
> Cap all three dynamic arrays at 128 entries using min() in the array
> size calculation. This ensures arrays are only as large as needed
> (up to the cap), avoiding unnecessary memory allocation for small
> operations while preventing overflow for large ones.
>
> The tracepoint now records the full nents/ents counts and a truncated
> flag so users can see when data has been capped.
>
> Changes in v2:
> - Use min(nents, DMA_TRACE_MAX_ENTRIES) for dynamic array sizing
> instead of fixed DMA_TRACE_MAX_ENTRIES allocation (feedback from
> Steven Rostedt)
> - This allocates only what's needed up to the cap, avoiding waste
> for small operations
>
> Reported-by: syzbot+28cea38c382fd15e751a@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=28cea38c382fd15e751a
> Tested-by: syzbot+28cea38c382fd15e751a@syzkaller.appspotmail.com
> Signed-off-by: Deepanshu Kartikey <Kartikey406@gmail.com>
> ---
> include/trace/events/dma.h | 25 +++++++++++++++++++------
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/include/trace/events/dma.h b/include/trace/events/dma.h
> index b3fef140ae15..33e99e792f1a 100644
> --- a/include/trace/events/dma.h
> +++ b/include/trace/events/dma.h
> @@ -275,6 +275,8 @@ TRACE_EVENT(dma_free_sgt,
> sizeof(u64), sizeof(u64)))
> );
>
> +#define DMA_TRACE_MAX_ENTRIES 128
> +
> TRACE_EVENT(dma_map_sg,
> TP_PROTO(struct device *dev, struct scatterlist *sgl, int nents,
> int ents, enum dma_data_direction dir, unsigned long attrs),
> @@ -282,9 +284,12 @@ TRACE_EVENT(dma_map_sg,
>
> TP_STRUCT__entry(
> __string(device, dev_name(dev))
> - __dynamic_array(u64, phys_addrs, nents)
> - __dynamic_array(u64, dma_addrs, ents)
> - __dynamic_array(unsigned int, lengths, ents)
> + __field(int, full_nents)
> + __field(int, full_ents)
> + __field(bool, truncated)
> + __dynamic_array(u64, phys_addrs, min(nents, DMA_TRACE_MAX_ENTRIES))
> + __dynamic_array(u64, dma_addrs, min(ents, DMA_TRACE_MAX_ENTRIES))
> + __dynamic_array(unsigned int, lengths, min(ents, DMA_TRACE_MAX_ENTRIES))
> __field(enum dma_data_direction, dir)
> __field(unsigned long, attrs)
> ),
> @@ -292,11 +297,16 @@ TRACE_EVENT(dma_map_sg,
> TP_fast_assign(
> struct scatterlist *sg;
> int i;
> + int traced_nents = min_t(int, nents, DMA_TRACE_MAX_ENTRIES);
> + int traced_ents = min_t(int, ents, DMA_TRACE_MAX_ENTRIES);
>
> __assign_str(device);
> - for_each_sg(sgl, sg, nents, i)
> + __entry->full_nents = nents;
> + __entry->full_ents = ents;
> + __entry->truncated = (nents > DMA_TRACE_MAX_ENTRIES) || (ents > DMA_TRACE_MAX_ENTRIES);
> + for_each_sg(sgl, sg, traced_nents, i)
> ((u64 *)__get_dynamic_array(phys_addrs))[i] = sg_phys(sg);
> - for_each_sg(sgl, sg, ents, i) {
> + for_each_sg(sgl, sg, traced_ents, i) {
> ((u64 *)__get_dynamic_array(dma_addrs))[i] =
> sg_dma_address(sg);
> ((unsigned int *)__get_dynamic_array(lengths))[i] =
> @@ -306,9 +316,12 @@ TRACE_EVENT(dma_map_sg,
> __entry->attrs = attrs;
> ),
>
> - TP_printk("%s dir=%s dma_addrs=%s sizes=%s phys_addrs=%s attrs=%s",
> + TP_printk("%s dir=%s nents=%d/%d ents=%d/%d%s dma_addrs=%s sizes=%s phys_addrs=%s attrs=%s",
> __get_str(device),
> decode_dma_data_direction(__entry->dir),
> + min_t(int, __entry->full_nents, DMA_TRACE_MAX_ENTRIES), __entry->full_nents,
> + min_t(int, __entry->full_ents, DMA_TRACE_MAX_ENTRIES), __entry->full_ents,
> + __entry->truncated ? " [TRUNCATED]" : "",
> __print_array(__get_dynamic_array(dma_addrs),
> __get_dynamic_array_len(dma_addrs) /
> sizeof(u64), sizeof(u64)),
Reviwed-by: Sean Anderson <sean.anderson@linux.dev>
Although it's a bit unusual that there's no limit on dynamic arrays like there is for %*ph.
^ permalink raw reply
* [PATCH v2] tracing/dma: Cap dma_map_sg tracepoint arrays to prevent buffer overflow
From: Deepanshu Kartikey @ 2026-01-30 15:52 UTC (permalink / raw)
To: rostedt, mhiramat, mathieu.desnoyers, ptesarik
Cc: m.szyprowski, jgg, leon, kbusch, sean.anderson, linux-kernel,
linux-trace-kernel, Deepanshu Kartikey,
syzbot+28cea38c382fd15e751a, Deepanshu Kartikey
The dma_map_sg tracepoint can trigger a perf buffer overflow when
tracing large scatter-gather lists. With devices like virtio-gpu
creating large DRM buffers, nents can exceed 1000 entries, resulting
in:
phys_addrs: 1000 * 8 bytes = 8,000 bytes
dma_addrs: 1000 * 8 bytes = 8,000 bytes
lengths: 1000 * 4 bytes = 4,000 bytes
Total: ~20,000 bytes
This exceeds PERF_MAX_TRACE_SIZE (8192 bytes), causing:
WARNING: CPU: 0 PID: 5497 at kernel/trace/trace_event_perf.c:405
perf buffer not large enough, wanted 24620, have 8192
Cap all three dynamic arrays at 128 entries using min() in the array
size calculation. This ensures arrays are only as large as needed
(up to the cap), avoiding unnecessary memory allocation for small
operations while preventing overflow for large ones.
The tracepoint now records the full nents/ents counts and a truncated
flag so users can see when data has been capped.
Changes in v2:
- Use min(nents, DMA_TRACE_MAX_ENTRIES) for dynamic array sizing
instead of fixed DMA_TRACE_MAX_ENTRIES allocation (feedback from
Steven Rostedt)
- This allocates only what's needed up to the cap, avoiding waste
for small operations
Reported-by: syzbot+28cea38c382fd15e751a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=28cea38c382fd15e751a
Tested-by: syzbot+28cea38c382fd15e751a@syzkaller.appspotmail.com
Signed-off-by: Deepanshu Kartikey <Kartikey406@gmail.com>
---
include/trace/events/dma.h | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/include/trace/events/dma.h b/include/trace/events/dma.h
index b3fef140ae15..33e99e792f1a 100644
--- a/include/trace/events/dma.h
+++ b/include/trace/events/dma.h
@@ -275,6 +275,8 @@ TRACE_EVENT(dma_free_sgt,
sizeof(u64), sizeof(u64)))
);
+#define DMA_TRACE_MAX_ENTRIES 128
+
TRACE_EVENT(dma_map_sg,
TP_PROTO(struct device *dev, struct scatterlist *sgl, int nents,
int ents, enum dma_data_direction dir, unsigned long attrs),
@@ -282,9 +284,12 @@ TRACE_EVENT(dma_map_sg,
TP_STRUCT__entry(
__string(device, dev_name(dev))
- __dynamic_array(u64, phys_addrs, nents)
- __dynamic_array(u64, dma_addrs, ents)
- __dynamic_array(unsigned int, lengths, ents)
+ __field(int, full_nents)
+ __field(int, full_ents)
+ __field(bool, truncated)
+ __dynamic_array(u64, phys_addrs, min(nents, DMA_TRACE_MAX_ENTRIES))
+ __dynamic_array(u64, dma_addrs, min(ents, DMA_TRACE_MAX_ENTRIES))
+ __dynamic_array(unsigned int, lengths, min(ents, DMA_TRACE_MAX_ENTRIES))
__field(enum dma_data_direction, dir)
__field(unsigned long, attrs)
),
@@ -292,11 +297,16 @@ TRACE_EVENT(dma_map_sg,
TP_fast_assign(
struct scatterlist *sg;
int i;
+ int traced_nents = min_t(int, nents, DMA_TRACE_MAX_ENTRIES);
+ int traced_ents = min_t(int, ents, DMA_TRACE_MAX_ENTRIES);
__assign_str(device);
- for_each_sg(sgl, sg, nents, i)
+ __entry->full_nents = nents;
+ __entry->full_ents = ents;
+ __entry->truncated = (nents > DMA_TRACE_MAX_ENTRIES) || (ents > DMA_TRACE_MAX_ENTRIES);
+ for_each_sg(sgl, sg, traced_nents, i)
((u64 *)__get_dynamic_array(phys_addrs))[i] = sg_phys(sg);
- for_each_sg(sgl, sg, ents, i) {
+ for_each_sg(sgl, sg, traced_ents, i) {
((u64 *)__get_dynamic_array(dma_addrs))[i] =
sg_dma_address(sg);
((unsigned int *)__get_dynamic_array(lengths))[i] =
@@ -306,9 +316,12 @@ TRACE_EVENT(dma_map_sg,
__entry->attrs = attrs;
),
- TP_printk("%s dir=%s dma_addrs=%s sizes=%s phys_addrs=%s attrs=%s",
+ TP_printk("%s dir=%s nents=%d/%d ents=%d/%d%s dma_addrs=%s sizes=%s phys_addrs=%s attrs=%s",
__get_str(device),
decode_dma_data_direction(__entry->dir),
+ min_t(int, __entry->full_nents, DMA_TRACE_MAX_ENTRIES), __entry->full_nents,
+ min_t(int, __entry->full_ents, DMA_TRACE_MAX_ENTRIES), __entry->full_ents,
+ __entry->truncated ? " [TRUNCATED]" : "",
__print_array(__get_dynamic_array(dma_addrs),
__get_dynamic_array_len(dma_addrs) /
sizeof(u64), sizeof(u64)),
--
2.43.0
^ permalink raw reply related
* [PATCH] tracing: Add kerneldoc to trace_event_buffer_reserve()
From: Steven Rostedt @ 2026-01-30 15:37 UTC (permalink / raw)
To: LKML, Linux Trace Kernel; +Cc: Masami Hiramatsu, Mathieu Desnoyers
From: Steven Rostedt <rostedt@goodmis.org>
Add a appropriate kerneldoc to trace_event_buffer_reserve() to make it
easier to understand how that function is used.
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
kernel/trace/trace_events.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 4972e1a2b5f3..af6d1fe5cab7 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -649,6 +649,22 @@ bool trace_event_ignore_this_pid(struct trace_event_file *trace_file)
}
EXPORT_SYMBOL_GPL(trace_event_ignore_this_pid);
+/**
+ * trace_event_buffer_reserve - reserve space on the ring buffer for an event
+ * @fbuffer: information about how to save the event
+ * @trace_file: the instance file descriptor for the event
+ * @len: The length of the event
+ *
+ * The @fbuffer has information about the ring buffer and data will
+ * be added to it to be used by the call to trace_event_buffer_commit().
+ * The @trace_file is the desrciptor with information about the status
+ * of the given event for a specific trace_array instance.
+ * The @len is the length of data to save for the event.
+ *
+ * Returns a pointer to the data on the ring buffer or NULL if the
+ * event was not reserved (event was filtered, too big, or the buffer
+ * simply was disabled for write).
+ */
void *trace_event_buffer_reserve(struct trace_event_buffer *fbuffer,
struct trace_event_file *trace_file,
unsigned long len)
--
2.51.0
^ permalink raw reply related
* Re: [PATCH v5 2/4] tracing: Make the backup instance non-reusable
From: Steven Rostedt @ 2026-01-30 15:21 UTC (permalink / raw)
To: Masami Hiramatsu (Google)
Cc: Mathieu Desnoyers, linux-kernel, linux-trace-kernel
In-Reply-To: <20260130182058.abb5c2b401f104afd25261d3@kernel.org>
On Fri, 30 Jan 2026 18:20:58 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> > > - trace_create_file("free_buffer", 0200, d_tracer,
> > > - tr, &tracing_free_buffer_fops);
> > > + if (!readonly) {
> > > + trace_create_file("free_buffer", 0200, d_tracer,
> > > + tr, &tracing_free_buffer_fops);
> >
> > Hmm, why remove the free_buffer. It just shrinks the buffer down to a
> > minimum. Perhaps its useless, but I it doesn't write to the buffer. Sure it
> > removes data but so does trace_pipe.
>
> I can keep it but free_buffer and free instance is a bit different on
> the persistent ring buffer and its backup. For both cases, since the
> scratch area needs to be kept, it does not free the reserved memory.
> (but the minimum ring buffer is dynamically allocated.)
> IMHO, the buffer resize interfaces are not useful for persistent ring
> buffer.
Ah yeah. OK, lets not add free_buffer to read only instances.
-- Steve
^ permalink raw reply
* Re: [PATCH] trace/hwlat: prevent false sharing in get_sample()
From: Steven Rostedt @ 2026-01-30 15:19 UTC (permalink / raw)
To: Colin Lord
Cc: linux-kernel, linux-trace-kernel, Masami Hiramatsu,
Mathieu Desnoyers
In-Reply-To: <20260130090906.24108-1-clord@mykolab.com>
On Fri, 30 Jan 2026 01:09:06 -0800
Colin Lord <clord@mykolab.com> wrote:
> The get_sample() function in the hwlat tracer assumes the caller holds
> hwlat_data.lock, but this is not actually happening. The result is
> unprotected data access to hwlat_data, and in per-cpu mode can result in
> false sharing. The false sharing can cause false positive latency
> events, since the sample_width member is involved and gets read as part
> of the main latency detection loop.
>
> Lock before accessing hwlat_data members, and prevent false sharing by
> pulling sample_width into a local variable.
>
> One system this was tested on was a dual socket server with 32 CPUs on
> each numa node. With settings of 1us threshold, 1000us width, and
> 2000us window, this change reduced the number of latency events from
> 500 per second down to approximately 1 event per minute. Some machines
> tested did not exhibit measurable latency from the false sharing.
Thanks for the report!
>
> Signed-off-by: Colin Lord <clord@mykolab.com>
> ---
> Hello, while debugging some poor hwlat results on a server I found this
> false sharing. I've tested the patch on multiple servers with many of
> the configs suggested by the patch submission checklist. A notable
> exception is I wasn't able to test with an SMP disabled build as
> multiple tags, including unmodified v6.18, were unable to finish booting
> with my config/hardware, however it did compile successfully. My
> understanding is that SMP is on its way to being required so I didn't
> spend more time on it, but I can do so if it's important. Thanks for
> your time and any feedback!
Don't worry about testing every flavor, that's my job ;-)
All I ask is basic tests, and see if it fixes the issue for you on your
machines.
>
> kernel/trace/trace_hwlat.c | 22 +++++++++++++++++-----
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/trace/trace_hwlat.c b/kernel/trace/trace_hwlat.c
> index 2f7b94e98317..1a4b1409226b 100644
> --- a/kernel/trace/trace_hwlat.c
> +++ b/kernel/trace/trace_hwlat.c
> @@ -193,8 +193,7 @@ void trace_hwlat_callback(bool enter)
> * get_sample - sample the CPU TSC and look for likely hardware latencies
> *
> * Used to repeatedly capture the CPU TSC (or similar), looking for potential
> - * hardware-induced latency. Called with interrupts disabled and with
> - * hwlat_data.lock held.
> + * hardware-induced latency. Disables interrupts during measurement.
I would remove the comment about the hwlat_data.lock held but still call
this with interrupts disabled.
> */
> static int get_sample(void)
> {
> @@ -204,6 +203,7 @@ static int get_sample(void)
> time_type start, t1, t2, last_t2;
> s64 diff, outer_diff, total, last_total = 0;
> u64 sample = 0;
> + u64 sample_width;
> u64 thresh = tracing_thresh;
> u64 outer_sample = 0;
> int ret = -1;
> @@ -211,6 +211,12 @@ static int get_sample(void)
>
> do_div(thresh, NSEC_PER_USEC); /* modifies interval value */
>
> + mutex_lock(&hwlat_data.lock);
> + sample_width = hwlat_data.sample_width;
> + mutex_unlock(&hwlat_data.lock);
We don't need to take the mutex here, a simple READ_ONCE() would do.
sample_width isn't a critical number, and as long as we get a consistent
number per sample (one that existed just before a user updated it or the
new number the user entered).
sample_width = READ_ONCE(hwlat_data.sample_width);
Especially since it doesn't even look like the hwlat_data.lock is used to
protect the updates of sample_width :-p
> +
> + local_irq_disable();
Then we don't need to move the disabling of interrupts here.
> +
> kdata->nmi_total_ts = 0;
> kdata->nmi_count = 0;
> /* Make sure NMIs see this first */
> @@ -267,12 +273,14 @@ static int get_sample(void)
> if (diff > sample)
> sample = diff; /* only want highest value */
>
> - } while (total <= hwlat_data.sample_width);
> + } while (total <= sample_width);
>
> barrier(); /* finish the above in the view for NMIs */
> trace_hwlat_callback_enabled = false;
> barrier(); /* Make sure nmi_total_ts is no longer updated */
>
> + local_irq_enable();
> +
> ret = 0;
>
> /* If we exceed the threshold value, we have found a hardware latency */
> @@ -285,8 +293,11 @@ static int get_sample(void)
> if (kdata->nmi_total_ts)
> do_div(kdata->nmi_total_ts, NSEC_PER_USEC);
>
> + mutex_lock(&hwlat_data.lock);
> hwlat_data.count++;
> s.seqnum = hwlat_data.count;
> + mutex_unlock(&hwlat_data.lock);
Let's make the counter an atomic_t and use:
s.seqnum = atomic_inc_return(&hwlat_data.count);
-- Steve
> +
> s.duration = sample;
> s.outer_duration = outer_sample;
> s.nmi_total_ts = kdata->nmi_total_ts;
> @@ -303,7 +314,10 @@ static int get_sample(void)
> }
> }
>
> + return ret;
> +
> out:
> + local_irq_enable();
> return ret;
> }
>
> @@ -361,9 +375,7 @@ static int kthread_fn(void *data)
> if (hwlat_data.thread_mode == MODE_ROUND_ROBIN)
> move_to_next_cpu();
>
> - local_irq_disable();
> get_sample();
> - local_irq_enable();
>
> mutex_lock(&hwlat_data.lock);
> interval = hwlat_data.sample_window - hwlat_data.sample_width;
>
> base-commit: 24d479d26b25bce5faea3ddd9fa8f3a6c3129ea7
^ permalink raw reply
* Re: [PATCH] trace/hwlat: prevent false sharing in get_sample()
From: Steven Rostedt @ 2026-01-30 15:19 UTC (permalink / raw)
To: Colin Lord
Cc: linux-kernel, linux-trace-kernel, Masami Hiramatsu,
Mathieu Desnoyers
In-Reply-To: <20260130090906.24108-1-clord@mykolab.com>
On Fri, 30 Jan 2026 01:09:06 -0800
Colin Lord <clord@mykolab.com> wrote:
> The get_sample() function in the hwlat tracer assumes the caller holds
> hwlat_data.lock, but this is not actually happening. The result is
> unprotected data access to hwlat_data, and in per-cpu mode can result in
> false sharing. The false sharing can cause false positive latency
> events, since the sample_width member is involved and gets read as part
> of the main latency detection loop.
>
> Lock before accessing hwlat_data members, and prevent false sharing by
> pulling sample_width into a local variable.
>
> One system this was tested on was a dual socket server with 32 CPUs on
> each numa node. With settings of 1us threshold, 1000us width, and
> 2000us window, this change reduced the number of latency events from
> 500 per second down to approximately 1 event per minute. Some machines
> tested did not exhibit measurable latency from the false sharing.
Thanks for the report!
>
> Signed-off-by: Colin Lord <clord@mykolab.com>
> ---
> Hello, while debugging some poor hwlat results on a server I found this
> false sharing. I've tested the patch on multiple servers with many of
> the configs suggested by the patch submission checklist. A notable
> exception is I wasn't able to test with an SMP disabled build as
> multiple tags, including unmodified v6.18, were unable to finish booting
> with my config/hardware, however it did compile successfully. My
> understanding is that SMP is on its way to being required so I didn't
> spend more time on it, but I can do so if it's important. Thanks for
> your time and any feedback!
Don't worry about testing every flavor, that's my job ;-)
All I ask is basic tests, and see if it fixes the issue for you on your
machines.
>
> kernel/trace/trace_hwlat.c | 22 +++++++++++++++++-----
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/trace/trace_hwlat.c b/kernel/trace/trace_hwlat.c
> index 2f7b94e98317..1a4b1409226b 100644
> --- a/kernel/trace/trace_hwlat.c
> +++ b/kernel/trace/trace_hwlat.c
> @@ -193,8 +193,7 @@ void trace_hwlat_callback(bool enter)
> * get_sample - sample the CPU TSC and look for likely hardware latencies
> *
> * Used to repeatedly capture the CPU TSC (or similar), looking for potential
> - * hardware-induced latency. Called with interrupts disabled and with
> - * hwlat_data.lock held.
> + * hardware-induced latency. Disables interrupts during measurement.
I would remove the comment about the hwlat_data.lock held but still call
this with interrupts disabled.
> */
> static int get_sample(void)
> {
> @@ -204,6 +203,7 @@ static int get_sample(void)
> time_type start, t1, t2, last_t2;
> s64 diff, outer_diff, total, last_total = 0;
> u64 sample = 0;
> + u64 sample_width;
> u64 thresh = tracing_thresh;
> u64 outer_sample = 0;
> int ret = -1;
> @@ -211,6 +211,12 @@ static int get_sample(void)
>
> do_div(thresh, NSEC_PER_USEC); /* modifies interval value */
>
> + mutex_lock(&hwlat_data.lock);
> + sample_width = hwlat_data.sample_width;
> + mutex_unlock(&hwlat_data.lock);
We don't need to take the mutex here, a simple READ_ONCE() would do.
sample_width isn't a critical number, and as long as we get a consistent
number per sample (one that existed just before a user updated it or the
new number the user entered).
sample_width = READ_ONCE(hwlat_data.sample_width);
Especially since it doesn't even look like the hwlat_data.lock is used to
protect the updates of sample_width :-p
> +
> + local_irq_disable();
Then we don't need to move the disabling of interrupts here.
> +
> kdata->nmi_total_ts = 0;
> kdata->nmi_count = 0;
> /* Make sure NMIs see this first */
> @@ -267,12 +273,14 @@ static int get_sample(void)
> if (diff > sample)
> sample = diff; /* only want highest value */
>
> - } while (total <= hwlat_data.sample_width);
> + } while (total <= sample_width);
>
> barrier(); /* finish the above in the view for NMIs */
> trace_hwlat_callback_enabled = false;
> barrier(); /* Make sure nmi_total_ts is no longer updated */
>
> + local_irq_enable();
> +
> ret = 0;
>
> /* If we exceed the threshold value, we have found a hardware latency */
> @@ -285,8 +293,11 @@ static int get_sample(void)
> if (kdata->nmi_total_ts)
> do_div(kdata->nmi_total_ts, NSEC_PER_USEC);
>
> + mutex_lock(&hwlat_data.lock);
> hwlat_data.count++;
> s.seqnum = hwlat_data.count;
> + mutex_unlock(&hwlat_data.lock);
Let's make the counter an atomic_t and use:
s.seqnum = atomic_inc_return(&hwlat_data.count);
-- Steve
> +
> s.duration = sample;
> s.outer_duration = outer_sample;
> s.nmi_total_ts = kdata->nmi_total_ts;
> @@ -303,7 +314,10 @@ static int get_sample(void)
> }
> }
>
> + return ret;
> +
> out:
> + local_irq_enable();
> return ret;
> }
>
> @@ -361,9 +375,7 @@ static int kthread_fn(void *data)
> if (hwlat_data.thread_mode == MODE_ROUND_ROBIN)
> move_to_next_cpu();
>
> - local_irq_disable();
> get_sample();
> - local_irq_enable();
>
> mutex_lock(&hwlat_data.lock);
> interval = hwlat_data.sample_window - hwlat_data.sample_width;
>
> base-commit: 24d479d26b25bce5faea3ddd9fa8f3a6c3129ea7
^ permalink raw reply
* Re: [PATCH] tracing/dma: Cap dma_map_sg tracepoint arrays to prevent buffer overflow
From: Steven Rostedt @ 2026-01-30 14:57 UTC (permalink / raw)
To: Deepanshu Kartikey
Cc: mhiramat, mathieu.desnoyers, m.szyprowski, leon, jgg, ptesarik,
kbusch, linux-kernel, linux-trace-kernel,
syzbot+28cea38c382fd15e751a
In-Reply-To: <20260130060317.54522-1-kartikey406@gmail.com>
On Fri, 30 Jan 2026 11:33:17 +0530
Deepanshu Kartikey <kartikey406@gmail.com> wrote:
> The dma_map_sg tracepoint can trigger a perf buffer overflow when
> tracing large scatter-gather lists. With devices like virtio-gpu
> creating large DRM buffers, nents can exceed 1000 entries, resulting
> in:
>
> phys_addrs: 1000 * 8 bytes = 8,000 bytes
> dma_addrs: 1000 * 8 bytes = 8,000 bytes
> lengths: 1000 * 4 bytes = 4,000 bytes
> Total: ~20,000 bytes
>
> This exceeds PERF_MAX_TRACE_SIZE (8192 bytes), causing:
>
> WARNING: CPU: 0 PID: 5497 at kernel/trace/trace_event_perf.c:405
> perf buffer not large enough, wanted 24620, have 8192
>
> Cap all three dynamic arrays at a fixed size of 128 entries. This limits
> the total event size to approximately 2,760 bytes, safely under the 8KB
> limit while still providing sufficient debugging information for typical
> cases.
>
> The tracepoint now records the full nents/ents counts and a truncated
> flag so users can see when data has been capped.
>
> Reported-by: syzbot+28cea38c382fd15e751a@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=28cea38c382fd15e751a
> Signed-off-by: Deepanshu Kartikey <Kartikey406@gmail.com>
> ---
> include/trace/events/dma.h | 25 +++++++++++++++++++------
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/include/trace/events/dma.h b/include/trace/events/dma.h
> index b3fef140ae15..c4e1a9f0c9c4 100644
> --- a/include/trace/events/dma.h
> +++ b/include/trace/events/dma.h
> @@ -275,6 +275,8 @@ TRACE_EVENT(dma_free_sgt,
> sizeof(u64), sizeof(u64)))
> );
>
> +#define DMA_TRACE_MAX_ENTRIES 128
> +
> TRACE_EVENT(dma_map_sg,
> TP_PROTO(struct device *dev, struct scatterlist *sgl, int nents,
> int ents, enum dma_data_direction dir, unsigned long attrs),
> @@ -282,9 +284,12 @@ TRACE_EVENT(dma_map_sg,
>
> TP_STRUCT__entry(
> __string(device, dev_name(dev))
> - __dynamic_array(u64, phys_addrs, nents)
> - __dynamic_array(u64, dma_addrs, ents)
> - __dynamic_array(unsigned int, lengths, ents)
> + __field(int, full_nents)
> + __field(int, full_ents)
> + __field(bool, truncated)
> + __dynamic_array(u64, phys_addrs, DMA_TRACE_MAX_ENTRIES)
> + __dynamic_array(u64, dma_addrs, DMA_TRACE_MAX_ENTRIES)
> + __dynamic_array(unsigned int, lengths, DMA_TRACE_MAX_ENTRIES)
This isn't doing what you want. You just used a dynamic array and allocated
a fixed size for it, regardless of if you use all of it or not. What you
want to do is:
__dynamic_array(u64, phys_addrs, min(nents, DMA_TRACE_MAX_ENTRIES))
__dynamic_array(u64, dma_addrs, min(ents, DMA_TRACE_MAX_ENTRIES))
__dynamic_array(unsigned int, lengths, min(ents, DMA_TRACE_MAX_ENTRIES))
And the same for the rest.
-- Steve
> __field(enum dma_data_direction, dir)
> __field(unsigned long, attrs)
> ),
^ permalink raw reply
* Re: [PATCH] bootconfig: Fix to terminate value search if it hits a newline
From: Steven Rostedt @ 2026-01-30 14:53 UTC (permalink / raw)
To: Masami Hiramatsu (Google); +Cc: linux-trace-kernel, Julius Werner, LKML
In-Reply-To: <176974810577.124104.16012554973099890517.stgit@devnote2>
On Fri, 30 Jan 2026 13:41:45 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> Note that if the line has a comment, it still keep searching the
> value to the next line::
>
> foo = # comment
> value
Hmm, so the above is the same as: foo = "value";
But what is:
foo = value # comment
bar
?
Would that be foo = value bar;
Or simply an error. Or would both be an error?
OK, this isn't related to the patch, but as comment processing was brought
to my attention, I'm wondering how it works for the above.
>
> This is interpreted as `foo = "value"`.
> This also updates the test script so that it can check the above cases.
>
> Fixes: 76db5a27a827 ("bootconfig: Add Extra Boot Config support")
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
> tools/bootconfig/test-bootconfig.sh | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
As for the patch itself:
Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
-- Steve
^ permalink raw reply
* Re: [PATCH v5 4/4] tracing/Documentation: Add a section about backup instance
From: Steven Rostedt @ 2026-01-30 14:27 UTC (permalink / raw)
To: Masami Hiramatsu (Google)
Cc: Mathieu Desnoyers, linux-kernel, linux-trace-kernel
In-Reply-To: <20260130092329.23ee13f9@gandalf.local.home>
On Fri, 30 Jan 2026 09:23:29 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> Nope ;-) In this case, leaving out the "the" is appropriate. Now you would
> need article if it was: the "boot_map" instance
"need the article"
God, I can't even get it right :-p
-- Steve
^ permalink raw reply
* Re: [PATCH v5 4/4] tracing/Documentation: Add a section about backup instance
From: Steven Rostedt @ 2026-01-30 14:23 UTC (permalink / raw)
To: Masami Hiramatsu (Google)
Cc: Mathieu Desnoyers, linux-kernel, linux-trace-kernel
In-Reply-To: <20260130222449.bc25b973597721b6a22e4f6d@kernel.org>
On Fri, 30 Jan 2026 22:24:49 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> > > +
> > > +When the boot up, the previous data in the`boot_map` is copied to "backup"
> >
> > s/When the boot up/On boot up/ copied to the "backup"
>
> Ah, thanks,
>
> >
> > > +instance, and the "sched:*" and "irq:*" events for current boot are traced on
> >
> > for the current boot are traced in
> >
> >
> > > +"boot_map". Thus user can read the previous boot data from the "backup" instance
>
> ^ the "boot_map".
Nope ;-) In this case, leaving out the "the" is appropriate. Now you would
need article if it was: the "boot_map" instance
But because you left out "instance" you don't need the article, as
"boot_map" is a name of an object and doesn't need the article when used by
itself.
You wouldn't use an article with:
The code was written by Steven.
But you would if you said:
The code was written by the Steven person.
;-)
> >
> > Definite articles are a pain ;-)
As I said. Definite articles in English are a pain ;)
-- Steve
^ permalink raw reply
* Re: [PATCH v5 4/4] tracing/Documentation: Add a section about backup instance
From: Masami Hiramatsu @ 2026-01-30 13:24 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Mathieu Desnoyers, linux-kernel, linux-trace-kernel
In-Reply-To: <20260129151900.44b6182c@gandalf.local.home>
On Thu, 29 Jan 2026 15:19:00 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 28 Jan 2026 09:10:11 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
>
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
> > Add a section about backup instance to the debugging.rst.
> >
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> > Documentation/trace/debugging.rst | 19 +++++++++++++++++++
> > 1 file changed, 19 insertions(+)
> >
> > diff --git a/Documentation/trace/debugging.rst b/Documentation/trace/debugging.rst
> > index 4d88c346fc38..73e2154bcf98 100644
> > --- a/Documentation/trace/debugging.rst
> > +++ b/Documentation/trace/debugging.rst
> > @@ -159,3 +159,22 @@ If setting it from the kernel command line, it is recommended to also
> > disable tracing with the "traceoff" flag, and enable tracing after boot up.
> > Otherwise the trace from the most recent boot will be mixed with the trace
> > from the previous boot, and may make it confusing to read.
> > +
> > +Using a backup instance for keeping previous boot data
> > +------------------------------------------------------
> > +
> > +It is also possible to record trace data at system boot time by specifying
> > +events with the persistent ring buffer, but in this case the data before the
> > +reboot will be lost before it can be read. This problem can be solved by a
> > +backup instance. From the kernel command line::
> > +
> > + reserve_mem=12M:4096:trace trace_instance=boot_map@trace,sched,irq trace_instance=backup=boot_map
> > +
> > +When the boot up, the previous data in the`boot_map` is copied to "backup"
>
> s/When the boot up/On boot up/ copied to the "backup"
Ah, thanks,
>
> > +instance, and the "sched:*" and "irq:*" events for current boot are traced on
>
> for the current boot are traced in
>
>
> > +"boot_map". Thus user can read the previous boot data from the "backup" instance
^ the "boot_map".
>
> Thus the user
>
> > +without stopping trace.
>
> stopping the trace.
OK.
>
> > +
> > +Note that this "backup" instance is readonly, and will be removed automatically
> > +if you clear the trace data or read out all trace data from "trace_pipe" or
> > +"trace_pipe_raw" files.
> > \ No newline at end of file
>
> Definite articles are a pain ;-)
Thanks!
>
> -- Steve
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCH -next] cgroup: increase maximum subsystem count from 16 to 32
From: Michal Koutný @ 2026-01-30 10:22 UTC (permalink / raw)
To: Chen Ridong
Cc: tj, hannes, rostedt, mhiramat, mathieu.desnoyers, inwardvessel,
shakeel.butt, cgroups, linux-kernel, linux-trace-kernel,
lujialin4
In-Reply-To: <3a12eb16-3a91-4278-9dfd-6c6f424e7f9f@huaweicloud.com>
[-- Attachment #1: Type: text/plain, Size: 781 bytes --]
On Thu, Jan 29, 2026 at 05:51:33PM +0800, Chen Ridong <chenridong@huaweicloud.com> wrote:
> We compiled with 'make allmodconfig'.
A-ha.
> The BUILD_BUG_ON(CGROUP_SUBSYS_COUNT > 16) macro worked correctly.
Good.
> Can I propose increasing the maximum number now? If we switch certain configs to
> default N and then a new subsystem is added later, the default configuration may
> work fine, but it will become a problem under allmodconfig — which some users
> actually rely on.
>
> Besides, this shouldn't be a major change, right?
I'd like there to be gradual move away from legacy controllers code
captured in config defaults.
Could you adjust the commit message to stress out the allmodconfig tests?
The change is OK technically.
Thanks,
Michal
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]
^ permalink raw reply
* Re: [PATCH v4 5/5] blktrace: Make init_blk_tracer() asynchronous when trace_async_init set
From: Yaxiong Tian @ 2026-01-30 9:59 UTC (permalink / raw)
To: Masami Hiramatsu (Google), Steven Rostedt, Jens Axboe
Cc: Jens Axboe, mathieu.desnoyers, corbet, skhan, linux-trace-kernel,
linux-block, linux-kernel, linux-doc
In-Reply-To: <20260130183022.add316050dd9638e6d71a469@kernel.org>
在 2026/1/30 17:30, Masami Hiramatsu (Google) 写道:
> On Thu, 29 Jan 2026 15:29:58 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
>> On Wed, 28 Jan 2026 19:25:46 -0700
>> Jens Axboe <axboe@kernel.dk> wrote:
>>
>>> On Jan 28, 2026, at 5:40 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>>>>
>>>> Jens,
>>>>
>>>> Can you give me an acked-by on this patch and I can take the series through
>>>> my tree.
>>> On phone, hope this works:
>>>
>>> Acked-by: Jens Axboe <axboe@kernel.dk>
>> Thanks!
>>
>>>> Or perhaps this doesn't even need to test the trace_async_init flag and can
>>>> always do the work queue? Does blk_trace ever do tracing at boot up? That
>>>> is, before user space starts?
>>> Not via the traditonal way of running blktrace.
>> Masami and Yaxiong,
>>
>> I've been thinking about this more and I'm not sure we need the
>> trace_async_init kernel parameter at all. As blktrace should only be
>> enabled by user space, it can always use the work queue.
>>
>> For kprobes, if someone is adding a kprobe on the kernel command line, then
>> they are already specifying that tracing is more important.
>>
>> Patch 3 already keeps kprobes from being an issue with contention of the
>> tracing locks, so I don't think it ever needs to use the work queue.
>>
>> Wouldn't it just be better to remove the trace_async_init and make blktrace
>> always use the work queue and kprobes never do it (but exit out early if
>> there were no kprobes registered)?
> Yeah, for kprobes event case, that sounds good to me. I think [3/5] is
> enough to speed it up if user does not define kprobe events on cmdline.
>
> Thank you,
Agreed.
Hi Jens：
what do you think about this proposal (making blktrace always use the
work queue)?
>
>> That is, remove patch 2 and 4 and make this patch always use the work queue.
>>
>> -- Steve
>
^ permalink raw reply
* Re: [PATCH v5] stm: class: Add MIPI OST protocol support
From: Alexander Shishkin @ 2026-01-30 9:48 UTC (permalink / raw)
To: Yingchao Deng, Steven Rostedt, Masami Hiramatsu,
Mathieu Desnoyers, Jonathan Corbet, Maxime Coquelin,
Alexandre Torgue
Cc: linux-kernel, linux-trace-kernel, linux-doc, linux-stm32,
linux-arm-kernel, quic_yingdeng, Tingwei Zhang, Yuanfang Zhang,
Jinlong Mao, Yingchao Deng, alexander.shishkin
In-Reply-To: <20260129-p_ost-v5-1-2b14fff39428@oss.qualcomm.com>
Yingchao Deng <yingchao.deng@oss.qualcomm.com> writes:
> + for (i = 1; i < ARRAY_SIZE(str_ost_entity_type); i++) {
> + if (i == pn->entity_type)
> + sz += sysfs_emit_at(page, sz, "[%s] ", str_ost_entity_type[i]);
> + else
> + sz += sysfs_emit_at(page, sz, "%s ", str_ost_entity_type[i]);
> + }
Greg hates this. Documentation [0] says "preferably": "Attributes should
be ASCII text files, preferably with only one value per file.", but
somebody will get yelled at if this gets spotted, and since it's
probably going to be me, let's maybe not do this.
Also, the ST Microsystems people don't need to be CC'd on Software Trace
Module patches: it's the same TLA meaning 2 different things. Everybody
does this, so I'm assuming they're cool with it, but FYI.
[0] https://docs.kernel.org/filesystems/sysfs.html#attributes
Thanks,
--
Alex
^ permalink raw reply
* Re: [PATCH v4 5/5] blktrace: Make init_blk_tracer() asynchronous when trace_async_init set
From: Masami Hiramatsu @ 2026-01-30 9:30 UTC (permalink / raw)
To: Steven Rostedt
Cc: Jens Axboe, Yaxiong Tian, mhiramat, mathieu.desnoyers, corbet,
skhan, linux-trace-kernel, linux-block, linux-kernel, linux-doc
In-Reply-To: <20260129152958.05c1ca46@gandalf.local.home>
On Thu, 29 Jan 2026 15:29:58 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 28 Jan 2026 19:25:46 -0700
> Jens Axboe <axboe@kernel.dk> wrote:
>
> > On Jan 28, 2026, at 5:40 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > >
> > >
> > > Jens,
> > >
> > > Can you give me an acked-by on this patch and I can take the series through
> > > my tree.
> >
> > On phone, hope this works:
> >
> > Acked-by: Jens Axboe <axboe@kernel.dk>
>
> Thanks!
>
> >
> > > Or perhaps this doesn't even need to test the trace_async_init flag and can
> > > always do the work queue? Does blk_trace ever do tracing at boot up? That
> > > is, before user space starts?
> >
> > Not via the traditonal way of running blktrace.
>
> Masami and Yaxiong,
>
> I've been thinking about this more and I'm not sure we need the
> trace_async_init kernel parameter at all. As blktrace should only be
> enabled by user space, it can always use the work queue.
>
> For kprobes, if someone is adding a kprobe on the kernel command line, then
> they are already specifying that tracing is more important.
>
> Patch 3 already keeps kprobes from being an issue with contention of the
> tracing locks, so I don't think it ever needs to use the work queue.
>
> Wouldn't it just be better to remove the trace_async_init and make blktrace
> always use the work queue and kprobes never do it (but exit out early if
> there were no kprobes registered)?
Yeah, for kprobes event case, that sounds good to me. I think [3/5] is
enough to speed it up if user does not define kprobe events on cmdline.
Thank you,
>
> That is, remove patch 2 and 4 and make this patch always use the work queue.
>
> -- Steve
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCH v5 3/4] tracing: Remove the backup instance automatically after read
From: Masami Hiramatsu @ 2026-01-30 9:24 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Mathieu Desnoyers, linux-kernel, linux-trace-kernel
In-Reply-To: <20260129151352.12e2be72@gandalf.local.home>
On Thu, 29 Jan 2026 15:13:52 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 28 Jan 2026 09:10:04 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
>
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
> > Since the backup instance is readonly, after reading all data
> > via pipe, no data is left on the instance. Thus it can be
> > removed safely after closing all files.
> > This also removes it if user resets the ring buffer manually
> > via 'trace' file.
> >
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> > Changes in v4:
> > - Update description.
> > ---
> > kernel/trace/trace.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++-
> > kernel/trace/trace.h | 6 +++++
> > 2 files changed, 69 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index d39f6509c12a..7d615a74f915 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -590,6 +590,55 @@ void trace_set_ring_buffer_expanded(struct trace_array *tr)
> > tr->ring_buffer_expanded = true;
> > }
> >
> > +static int __remove_instance(struct trace_array *tr);
> > +
> > +static void trace_array_autoremove(struct work_struct *work)
> > +{
> > + struct trace_array *tr = container_of(work, struct trace_array, autoremove_work);
> > +
> > + guard(mutex)(&event_mutex);
> > + guard(mutex)(&trace_types_lock);
> > +
> > + /*
> > + * This can be fail if someone gets @tr before starting this
> > + * function, but in that case, this will be kicked again when
> > + * putting it. So we don't care the result.
>
> "So we don't care about the result."
OK.
>
> > + */
> > + __remove_instance(tr);
> > +}
> > +
> > +static struct workqueue_struct *autoremove_wq;
> > +
> > +static void trace_array_init_autoremove(struct trace_array *tr)
> > +{
> > + INIT_WORK(&tr->autoremove_work, trace_array_autoremove);
> > +}
> > +
> > +static void trace_array_kick_autoremove(struct trace_array *tr)
> > +{
> > + if (!work_pending(&tr->autoremove_work) && autoremove_wq)
> > + queue_work(autoremove_wq, &tr->autoremove_work);
> > +}
> > +
> > +static void trace_array_cancel_autoremove(struct trace_array *tr)
> > +{
> > + if (work_pending(&tr->autoremove_work))
> > + cancel_work(&tr->autoremove_work);
> > +}
> > +
> > +__init static int trace_array_init_autoremove_wq(void)
> > +{
>
> This isn't needed if there's no backup trace_array right?
>
> Instead of creating a work queue when its not needed, just exit out if
> there's no backup trace_array.
>
> Oh, and the above functions should always test autoremove_wq for NULL.
Ah, right. OK, let me fix it.
>
> > + autoremove_wq = alloc_workqueue("tr_autoremove_wq",
> > + WQ_UNBOUND | WQ_HIGHPRI, 0);
> > + if (!autoremove_wq) {
> > + pr_err("Unable to allocate tr_autoremove_wq\n");
> > + return -ENOMEM;
> > + }
> > + return 0;
> > +}
> > +
> > +late_initcall_sync(trace_array_init_autoremove_wq);
> > +
> > LIST_HEAD(ftrace_trace_arrays);
> >
> > int trace_array_get(struct trace_array *this_tr)
> > @@ -598,7 +647,7 @@ int trace_array_get(struct trace_array *this_tr)
> >
> > guard(mutex)(&trace_types_lock);
> > list_for_each_entry(tr, &ftrace_trace_arrays, list) {
> > - if (tr == this_tr) {
> > + if (tr == this_tr && !tr->free_on_close) {
> > tr->ref++;
> > return 0;
> > }
>
> Break the above into:
>
> if (tr == this_tr) {
> if (tr->free_on_close)
> break;
> tr->ref++;
> return 0;
> }
>
> Why continue the loop if we found the trace_array but it's in the process
> of closing?
Ah, indeed. OK, I'll fix it.
Thank you!
>
> -- Steve
>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCH v5 2/4] tracing: Make the backup instance non-reusable
From: Masami Hiramatsu @ 2026-01-30 9:20 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Mathieu Desnoyers, linux-kernel, linux-trace-kernel
In-Reply-To: <20260129150745.1f9ab283@gandalf.local.home>
On Thu, 29 Jan 2026 15:07:45 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 28 Jan 2026 09:09:56 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
>
> > @@ -10632,17 +10658,23 @@ static __init void create_trace_instances(struct dentry *d_tracer)
> > static void
> > init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer)
> > {
> > + umode_t writable_mode = TRACE_MODE_WRITE;
> > + bool readonly = trace_array_is_readonly(tr);
> > int cpu;
> >
> > + if (readonly)
> > + writable_mode = TRACE_MODE_READ;
> > +
> > trace_create_file("available_tracers", TRACE_MODE_READ, d_tracer,
> > - tr, &show_traces_fops);
> > + tr, &show_traces_fops);
> >
> > - trace_create_file("current_tracer", TRACE_MODE_WRITE, d_tracer,
> > - tr, &set_tracer_fops);
> > + trace_create_file("current_tracer", writable_mode, d_tracer,
> > + tr, &set_tracer_fops);
> >
> > - trace_create_file("tracing_cpumask", TRACE_MODE_WRITE, d_tracer,
> > + trace_create_file("tracing_cpumask", writable_mode, d_tracer,
> > tr, &tracing_cpumask_fops);
> >
> > + /* Options are used for changing print-format even for readonly instance. */
> > trace_create_file("trace_options", TRACE_MODE_WRITE, d_tracer,
> > tr, &tracing_iter_fops);
> >
> > @@ -10652,27 +10684,35 @@ init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer)
> > trace_create_file("trace_pipe", TRACE_MODE_READ, d_tracer,
> > tr, &tracing_pipe_fops);
> >
> > - trace_create_file("buffer_size_kb", TRACE_MODE_WRITE, d_tracer,
> > + trace_create_file("buffer_size_kb", writable_mode, d_tracer,
> > tr, &tracing_entries_fops);
> >
> > trace_create_file("buffer_total_size_kb", TRACE_MODE_READ, d_tracer,
> > tr, &tracing_total_entries_fops);
> >
> > - trace_create_file("free_buffer", 0200, d_tracer,
> > - tr, &tracing_free_buffer_fops);
> > + if (!readonly) {
> > + trace_create_file("free_buffer", 0200, d_tracer,
> > + tr, &tracing_free_buffer_fops);
>
> Hmm, why remove the free_buffer. It just shrinks the buffer down to a
> minimum. Perhaps its useless, but I it doesn't write to the buffer. Sure it
> removes data but so does trace_pipe.
I can keep it but free_buffer and free instance is a bit different on
the persistent ring buffer and its backup. For both cases, since the
scratch area needs to be kept, it does not free the reserved memory.
(but the minimum ring buffer is dynamically allocated.)
IMHO, the buffer resize interfaces are not useful for persistent ring
buffer.
>
> >
> > - trace_create_file("trace_marker", 0220, d_tracer,
> > - tr, &tracing_mark_fops);
> > + trace_create_file("trace_marker", 0220, d_tracer,
> > + tr, &tracing_mark_fops);
> >
> > - tr->trace_marker_file = __find_event_file(tr, "ftrace", "print");
> > + tr->trace_marker_file = __find_event_file(tr, "ftrace", "print");
> >
> > - trace_create_file("trace_marker_raw", 0220, d_tracer,
> > - tr, &tracing_mark_raw_fops);
> > + trace_create_file("trace_marker_raw", 0220, d_tracer,
> > + tr, &tracing_mark_raw_fops);
> >
> > - trace_create_file("trace_clock", TRACE_MODE_WRITE, d_tracer, tr,
> > + trace_create_file("buffer_percent", TRACE_MODE_WRITE, d_tracer,
> > + tr, &buffer_percent_fops);
> > +
> > + trace_create_file("syscall_user_buf_size", TRACE_MODE_WRITE, d_tracer,
> > + tr, &tracing_syscall_buf_fops);
> > + }
> > +
> > + trace_create_file("trace_clock", writable_mode, d_tracer, tr,
> > &trace_clock_fops);
> >
> > - trace_create_file("tracing_on", TRACE_MODE_WRITE, d_tracer,
> > + trace_create_file("tracing_on", writable_mode, d_tracer,
> > tr, &rb_simple_fops);
>
> Hmm, should tracing_on exist in read only mode?
Ah, indeed. I'll remove this too.
> >
> > trace_create_file("timestamp_mode", TRACE_MODE_READ, d_tracer, tr,
> > @@ -10680,41 +10720,38 @@ init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer)
> >
> > tr->buffer_percent = 50;
> >
> > - trace_create_file("buffer_percent", TRACE_MODE_WRITE, d_tracer,
> > - tr, &buffer_percent_fops);
> > -
> > - trace_create_file("buffer_subbuf_size_kb", TRACE_MODE_WRITE, d_tracer,
> > + trace_create_file("buffer_subbuf_size_kb", writable_mode, d_tracer,
> > tr, &buffer_subbuf_size_fops);
> >
> > - trace_create_file("syscall_user_buf_size", TRACE_MODE_WRITE, d_tracer,
> > - tr, &tracing_syscall_buf_fops);
> > -
> > create_trace_options_dir(tr);
> >
> > #ifdef CONFIG_TRACER_MAX_TRACE
> > - trace_create_maxlat_file(tr, d_tracer);
> > + if (!readonly)
> > + trace_create_maxlat_file(tr, d_tracer);
> > #endif
> >
> > - if (ftrace_create_function_files(tr, d_tracer))
> > + if (!readonly && ftrace_create_function_files(tr, d_tracer))
> > MEM_FAIL(1, "Could not allocate function filter files");
> >
> > if (tr->range_addr_start) {
> > trace_create_file("last_boot_info", TRACE_MODE_READ, d_tracer,
> > tr, &last_boot_fops);
> > #ifdef CONFIG_TRACER_SNAPSHOT
> > - } else {
> > + } else if (!readonly) {
> > trace_create_file("snapshot", TRACE_MODE_WRITE, d_tracer,
> > tr, &snapshot_fops);
> > #endif
> > }
> >
> > - trace_create_file("error_log", TRACE_MODE_WRITE, d_tracer,
> > - tr, &tracing_err_log_fops);
> > + if (!readonly)
> > + trace_create_file("error_log", TRACE_MODE_WRITE, d_tracer,
> > + tr, &tracing_err_log_fops);
>
> Why not move this up into the "if (!readonly) {" block above?
OK, I'll move it.
>
> >
> > for_each_tracing_cpu(cpu)
> > tracing_init_tracefs_percpu(tr, cpu);
> >
> > - ftrace_init_tracefs(tr, d_tracer);
> > + if (!readonly)
> > + ftrace_init_tracefs(tr, d_tracer);
>
> Same here. Or just move the readonly block down to the end of the function.
OK.
>
> > }
> >
> > #ifdef CONFIG_TRACEFS_AUTOMOUNT_DEPRECATED
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index 69e7defba6c6..0adc644084bf 100644
> > --- a/kernel/trace/trace.h
> > +++ b/kernel/trace/trace.h
> > @@ -33,6 +33,7 @@
> >
> > #define TRACE_MODE_WRITE 0640
> > #define TRACE_MODE_READ 0440
> > +#define TRACE_MODE_WRITE_MASK (TRACE_MODE_WRITE & ~TRACE_MODE_READ)
> >
> > enum trace_type {
> > __TRACE_FIRST_TYPE = 0,
> > @@ -483,6 +484,12 @@ extern bool trace_clock_in_ns(struct trace_array *tr);
> >
> > extern unsigned long trace_adjust_address(struct trace_array *tr, unsigned long addr);
> >
> > +static inline bool trace_array_is_readonly(struct trace_array *tr)
> > +{
> > + /* backup instance is read only. */
> > + return tr->flags & TRACE_ARRAY_FL_VMALLOC;
>
> Hmm, I wonder if we should create a RDONLY flag for the trace_array?
OK, let me add the RDONLY flag. (maybe it will be reused afterwards)
Thanks,
>
> -- Steve
>
> > +}
> > +
> > /*
> > * The global tracer (top) should be the first trace array added,
> > * but we check the flag anyway.
> > @@ -681,7 +688,6 @@ struct dentry *trace_create_file(const char *name,
> > void *data,
> > const struct file_operations *fops);
> >
> > -
> > /**
> > * tracer_tracing_is_on_cpu - show real state of ring buffer enabled on for a cpu
> > * @tr : the trace array to know if ring buffer is enabled
> > diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
> > index dbe29b4c6a7a..2ca2541c8a58 100644
> > --- a/kernel/trace/trace_boot.c
> > +++ b/kernel/trace/trace_boot.c
> > @@ -61,7 +61,8 @@ trace_boot_set_instance_options(struct trace_array *tr, struct xbc_node *node)
> > v = memparse(p, NULL);
> > if (v < PAGE_SIZE)
> > pr_err("Buffer size is too small: %s\n", p);
> > - if (tracing_resize_ring_buffer(tr, v, RING_BUFFER_ALL_CPUS) < 0)
> > + if (trace_array_is_readonly(tr) ||
> > + tracing_resize_ring_buffer(tr, v, RING_BUFFER_ALL_CPUS) < 0)
> > pr_err("Failed to resize trace buffer to %s\n", p);
> > }
> >
> > @@ -597,7 +598,7 @@ trace_boot_enable_tracer(struct trace_array *tr, struct xbc_node *node)
> >
> > p = xbc_node_find_value(node, "tracer", NULL);
> > if (p && *p != '\0') {
> > - if (tracing_set_tracer(tr, p) < 0)
> > + if (trace_array_is_readonly(tr) || tracing_set_tracer(tr, p) < 0)
> > pr_err("Failed to set given tracer: %s\n", p);
> > }
> >
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* [PATCH] trace/hwlat: prevent false sharing in get_sample()
From: Colin Lord @ 2026-01-30 9:09 UTC (permalink / raw)
To: linux-kernel, linux-trace-kernel
Cc: Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers, Colin Lord
The get_sample() function in the hwlat tracer assumes the caller holds
hwlat_data.lock, but this is not actually happening. The result is
unprotected data access to hwlat_data, and in per-cpu mode can result in
false sharing. The false sharing can cause false positive latency
events, since the sample_width member is involved and gets read as part
of the main latency detection loop.
Lock before accessing hwlat_data members, and prevent false sharing by
pulling sample_width into a local variable.
One system this was tested on was a dual socket server with 32 CPUs on
each numa node. With settings of 1us threshold, 1000us width, and
2000us window, this change reduced the number of latency events from
500 per second down to approximately 1 event per minute. Some machines
tested did not exhibit measurable latency from the false sharing.
Signed-off-by: Colin Lord <clord@mykolab.com>
---
Hello, while debugging some poor hwlat results on a server I found this
false sharing. I've tested the patch on multiple servers with many of
the configs suggested by the patch submission checklist. A notable
exception is I wasn't able to test with an SMP disabled build as
multiple tags, including unmodified v6.18, were unable to finish booting
with my config/hardware, however it did compile successfully. My
understanding is that SMP is on its way to being required so I didn't
spend more time on it, but I can do so if it's important. Thanks for
your time and any feedback!
kernel/trace/trace_hwlat.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/kernel/trace/trace_hwlat.c b/kernel/trace/trace_hwlat.c
index 2f7b94e98317..1a4b1409226b 100644
--- a/kernel/trace/trace_hwlat.c
+++ b/kernel/trace/trace_hwlat.c
@@ -193,8 +193,7 @@ void trace_hwlat_callback(bool enter)
* get_sample - sample the CPU TSC and look for likely hardware latencies
*
* Used to repeatedly capture the CPU TSC (or similar), looking for potential
- * hardware-induced latency. Called with interrupts disabled and with
- * hwlat_data.lock held.
+ * hardware-induced latency. Disables interrupts during measurement.
*/
static int get_sample(void)
{
@@ -204,6 +203,7 @@ static int get_sample(void)
time_type start, t1, t2, last_t2;
s64 diff, outer_diff, total, last_total = 0;
u64 sample = 0;
+ u64 sample_width;
u64 thresh = tracing_thresh;
u64 outer_sample = 0;
int ret = -1;
@@ -211,6 +211,12 @@ static int get_sample(void)
do_div(thresh, NSEC_PER_USEC); /* modifies interval value */
+ mutex_lock(&hwlat_data.lock);
+ sample_width = hwlat_data.sample_width;
+ mutex_unlock(&hwlat_data.lock);
+
+ local_irq_disable();
+
kdata->nmi_total_ts = 0;
kdata->nmi_count = 0;
/* Make sure NMIs see this first */
@@ -267,12 +273,14 @@ static int get_sample(void)
if (diff > sample)
sample = diff; /* only want highest value */
- } while (total <= hwlat_data.sample_width);
+ } while (total <= sample_width);
barrier(); /* finish the above in the view for NMIs */
trace_hwlat_callback_enabled = false;
barrier(); /* Make sure nmi_total_ts is no longer updated */
+ local_irq_enable();
+
ret = 0;
/* If we exceed the threshold value, we have found a hardware latency */
@@ -285,8 +293,11 @@ static int get_sample(void)
if (kdata->nmi_total_ts)
do_div(kdata->nmi_total_ts, NSEC_PER_USEC);
+ mutex_lock(&hwlat_data.lock);
hwlat_data.count++;
s.seqnum = hwlat_data.count;
+ mutex_unlock(&hwlat_data.lock);
+
s.duration = sample;
s.outer_duration = outer_sample;
s.nmi_total_ts = kdata->nmi_total_ts;
@@ -303,7 +314,10 @@ static int get_sample(void)
}
}
+ return ret;
+
out:
+ local_irq_enable();
return ret;
}
@@ -361,9 +375,7 @@ static int kthread_fn(void *data)
if (hwlat_data.thread_mode == MODE_ROUND_ROBIN)
move_to_next_cpu();
- local_irq_disable();
get_sample();
- local_irq_enable();
mutex_lock(&hwlat_data.lock);
interval = hwlat_data.sample_window - hwlat_data.sample_width;
base-commit: 24d479d26b25bce5faea3ddd9fa8f3a6c3129ea7
--
2.51.2
^ permalink raw reply related
* Re: [PATCH v5 2/4] tracing: Make the backup instance non-reusable
From: Masami Hiramatsu @ 2026-01-30 9:01 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Mathieu Desnoyers, linux-kernel, linux-trace-kernel
In-Reply-To: <20260129145333.002815e1@gandalf.local.home>
On Thu, 29 Jan 2026 14:53:33 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 28 Jan 2026 09:09:56 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
>
> > @@ -9388,12 +9405,16 @@ static void
> > tracing_init_tracefs_percpu(struct trace_array *tr, long cpu)
> > {
> > struct dentry *d_percpu = tracing_dentry_percpu(tr, cpu);
> > + umode_t writable_mode = TRACE_MODE_WRITE;
> > struct dentry *d_cpu;
> > char cpu_dir[30]; /* 30 characters should be more than enough */
> >
> > if (!d_percpu)
> > return;
> >
> > + if (trace_array_is_readonly(tr))
> > + writable_mode = TRACE_MODE_READ;
>
> Hmm, writable_mode is set but not used?
Oops, good catch! I missed to update it.
Thanks!
>
> Looks like you missed an update.
>
> -- Steve
>
> > +
> > snprintf(cpu_dir, 30, "cpu%ld", cpu);
> > d_cpu = tracefs_create_dir(cpu_dir, d_percpu);
> > if (!d_cpu) {
> > @@ -9616,7 +9637,6 @@ struct dentry *trace_create_file(const char *name,
> > return ret;
> > }
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox