* Re: [PATCH v13 2/3] mm: Fix OOM killer inaccuracy on large many-core systems
From: Mathieu Desnoyers @ 2026-01-12 18:46 UTC (permalink / raw)
To: Shakeel Butt
Cc: Andrew Morton, linux-kernel, Paul E. McKenney, Steven Rostedt,
Masami Hiramatsu, Dennis Zhou, Tejun Heo, Christoph Lameter,
Martin Liu, David Rientjes, christian.koenig, SeongJae Park,
Michal Hocko, Johannes Weiner, Sweet Tea Dorminy, Lorenzo Stoakes,
Liam R . Howlett, Mike Rapoport, Suren Baghdasaryan,
Vlastimil Babka, Christian Brauner, Wei Yang, David Hildenbrand,
Miaohe Lin, Al Viro, linux-mm, linux-trace-kernel, Yu Zhao,
Roman Gushchin, Mateusz Guzik, Matthew Wilcox, Baolin Wang,
Aboorva Devarajan
In-Reply-To: <tzgfnxf3tii4s65inlzghhrzgc5gkp2stugnuzuqnjraqbp57k@khqklb37lci2>
On 2026-01-12 12:29, Shakeel Butt wrote:
> On Sun, Jan 11, 2026 at 02:49:57PM -0500, Mathieu Desnoyers wrote:
> [...]
>> @@ -626,14 +631,12 @@ static void check_mm(struct mm_struct *mm)
>> "Please make sure 'struct resident_page_types[]' is updated as well");
>>
>> for (i = 0; i < NR_MM_COUNTERS; i++) {
>> - long x = percpu_counter_sum(&mm->rss_stat[i]);
>> -
>> - if (unlikely(x)) {
>> - pr_alert("BUG: Bad rss-counter state mm:%p type:%s val:%ld Comm:%s Pid:%d\n",
>> - mm, resident_page_types[i], x,
>> + if (unlikely(percpu_counter_tree_precise_compare_value(&mm->rss_stat[i], 0) != 0))
>> + pr_alert("BUG: Bad rss-counter state mm:%p type:%s val:%d Comm:%s Pid:%d\n",
>> + mm, resident_page_types[i],
>> + percpu_counter_tree_precise_sum(&mm->rss_stat[i]),
>
> I haven't gone through the whole series yet but this "val:%ld" to
> "val:%d" conversion seems like unintended. Next patch fixed it but it
> should be fixed here.
My mistake, will fix for v14.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
^ permalink raw reply
* Re: [PATCH v12 0/3] mm: Fix OOM killer inaccuracy on large many-core systems
From: Mathieu Desnoyers @ 2026-01-12 18:36 UTC (permalink / raw)
To: Steven Rostedt
Cc: Andrew Morton, linux-kernel, Paul E. McKenney, Masami Hiramatsu,
Dennis Zhou, Tejun Heo, Christoph Lameter, Martin Liu,
David Rientjes, christian.koenig, Shakeel Butt, SeongJae Park,
Michal Hocko, Johannes Weiner, Sweet Tea Dorminy, Lorenzo Stoakes,
Liam R . Howlett, Mike Rapoport, Suren Baghdasaryan,
Vlastimil Babka, Christian Brauner, Wei Yang, David Hildenbrand,
Miaohe Lin, Al Viro, linux-mm, linux-trace-kernel, Yu Zhao,
Roman Gushchin, Mateusz Guzik, Matthew Wilcox, Baolin Wang,
Aboorva Devarajan
In-Reply-To: <20260112100523.6ab884be@gandalf.local.home>
On 2026-01-12 10:05, Steven Rostedt wrote:
> On Sun, 11 Jan 2026 13:04:59 -0500
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>
>> As a patch author, I find this feedback really useful. Is there
>> an easy way to get this feedback privately before sending out my
>> patches ?
>
> Hehe, we all want to look good don't we ;-)
>
> For me. I let people see the good, the bad, and the ugly (probably more than I should!)
I really don't mind having healthy discussions publicly, but I try
to minimize the amount of disruption my work brings on others.
That being said, I think this AI review can lead to interesting
discussions where others can pitch in, so having it in the open
is worthwhile.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
^ permalink raw reply
* Re: [PATCH v13 2/3] mm: Fix OOM killer inaccuracy on large many-core systems
From: Shakeel Butt @ 2026-01-12 17:29 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Andrew Morton, linux-kernel, Paul E. McKenney, Steven Rostedt,
Masami Hiramatsu, Dennis Zhou, Tejun Heo, Christoph Lameter,
Martin Liu, David Rientjes, christian.koenig, SeongJae Park,
Michal Hocko, Johannes Weiner, Sweet Tea Dorminy, Lorenzo Stoakes,
Liam R . Howlett, Mike Rapoport, Suren Baghdasaryan,
Vlastimil Babka, Christian Brauner, Wei Yang, David Hildenbrand,
Miaohe Lin, Al Viro, linux-mm, linux-trace-kernel, Yu Zhao,
Roman Gushchin, Mateusz Guzik, Matthew Wilcox, Baolin Wang,
Aboorva Devarajan
In-Reply-To: <20260111194958.1231477-3-mathieu.desnoyers@efficios.com>
On Sun, Jan 11, 2026 at 02:49:57PM -0500, Mathieu Desnoyers wrote:
[...]
> @@ -626,14 +631,12 @@ static void check_mm(struct mm_struct *mm)
> "Please make sure 'struct resident_page_types[]' is updated as well");
>
> for (i = 0; i < NR_MM_COUNTERS; i++) {
> - long x = percpu_counter_sum(&mm->rss_stat[i]);
> -
> - if (unlikely(x)) {
> - pr_alert("BUG: Bad rss-counter state mm:%p type:%s val:%ld Comm:%s Pid:%d\n",
> - mm, resident_page_types[i], x,
> + if (unlikely(percpu_counter_tree_precise_compare_value(&mm->rss_stat[i], 0) != 0))
> + pr_alert("BUG: Bad rss-counter state mm:%p type:%s val:%d Comm:%s Pid:%d\n",
> + mm, resident_page_types[i],
> + percpu_counter_tree_precise_sum(&mm->rss_stat[i]),
I haven't gone through the whole series yet but this "val:%ld" to
"val:%d" conversion seems like unintended. Next patch fixed it but it
should be fixed here.
^ permalink raw reply
* Re: [PATCH v5] tracing: Guard __DECLARE_TRACE() use of __DO_TRACE_CALL() with SRCU-fast
From: Alexei Starovoitov @ 2026-01-12 17:19 UTC (permalink / raw)
To: Steven Rostedt
Cc: Mathieu Desnoyers, LKML, Linux trace kernel, bpf,
Masami Hiramatsu, Paul E. McKenney, Sebastian Andrzej Siewior,
Thomas Gleixner
In-Reply-To: <20260112085257.26bb7b5b@fedora>
On Mon, Jan 12, 2026 at 5:53 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Sun, 11 Jan 2026 15:38:38 -0800
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>
> > > Oh, so you are OK replacing the preempt_disable in the tracepoint
> > > callbacks with fast SRCU?
> >
> > yes, but..
> >
> > > Then I guess we can simply do that. Would it be fine to do that for
> > > both RT and non-RT? That will simplify the code quite a bit.
> >
> > Agree. perf needs preempt_disable in their callbacks (as this patch does)
> > and bpf side needs to add migrate_disable in __bpf_trace_run for now.
> > Though syscall tracepoints are sleepable we don't take advantage of
> > that on the bpf side. Eventually we will, and then rcu_lock
> > inside __bpf_trace_run will become srcu_fast_lock.
> >
> > The way to think about generic infrastructure like tracepoints is
> > to minimize their overhead no matter what out-of-tree and in-tree
> > users' assumptions are today, so why do we need preempt_disable
> > or srcu_fast there?
>
> Either preempt disable or srcu_fast is still needed.
>
> > I think today it's there because all callbacks (perf, ftrace, bpf)
> > expect preemption to be disabled, but can we just remove it from tp side?
> > and move preempt_disable to callbacks that actually need it?
>
> Yes if we are talking about switching from preempt_disable to src_fast.
> No if you mean to remove both as it still needs RCU protection. It's
> used for synchronizing changes in the tracepoint infrastructure itself.
> That __DO_TRACE_CALL() macro is the guts of the tracepoint callback
> code. It needs to handle races with additions and removals of callbacks
> there, as the callbacks also get data passed to them. If it gets out of
> sync, a callback could be called with another callback's data.
>
> That's why it has:
>
> it_func_ptr = \
> rcu_dereference_raw((&__tracepoint_##name)->funcs);
>
> >
> > I'm looking at release_probes(). It's fine as-is, no?
>
> That's just freeing, and as you see, there's RCU synchronization
> required.
>
> Updates to tracepoints require RCU protection. It started out with
> preempt_disable() for all tracepoints (which was synchronized with
> synchronized_sched() which later became just synchronize_rcu()).
I see.
> The issue that came about was that both ftrace and perf had an
> assumption that its tracepoint callbacks always have preempt disabled
> when being called. To move to srcu_fast() that is no longer the case.
> And we need to do that for PREEMPT_RT if BPF is running very long
> callbacks to the tracepoints.
>
> Ftrace has been fixed to not require it, but still needs to take into
> account if tracepoints disable preemption or not so that it can display
> the preempt_count() of when the tracepoint was called correctly.
>
> Perf is trivial to fix as it can simply add a preempt_disable() in its
> handler.
>
> But we were originally told that BPF had an issue because it had the
> assumption that tracepoint callbacks wouldn't migrate. That's where
> this patch came about.
>
> Now if you are saying that BPF will handle migrate_disable() on its own
> and not require the tracepoint infrastructure to do it for it, then
> this is perfect. And I can then simplify this code, and just use
> srcu_fast for both RT and !RT.
Agree. Just add migrate_disable to __bpf_trace_run,
or, better yet, use rcu_read_lock_dont_migrate() in there.
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index fe28d86f7c35..abbf0177ad20 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2062,7 +2062,7 @@ void __bpf_trace_run(struct bpf_raw_tp_link
*link, u64 *args)
struct bpf_run_ctx *old_run_ctx;
struct bpf_trace_run_ctx run_ctx;
- cant_sleep();
+ rcu_read_lock_dont_migrate();
if (unlikely(this_cpu_inc_return(*(prog->active)) != 1)) {
bpf_prog_inc_misses_counter(prog);
goto out;
@@ -2071,13 +2071,12 @@ void __bpf_trace_run(struct bpf_raw_tp_link
*link, u64 *args)
run_ctx.bpf_cookie = link->cookie;
old_run_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);
- rcu_read_lock();
(void) bpf_prog_run(prog, args);
- rcu_read_unlock();
bpf_reset_run_ctx(old_run_ctx);
out:
this_cpu_dec(*(prog->active));
+ rcu_read_unlock_migrate();
}
^ permalink raw reply related
* Re: [PATCH v12 0/3] mm: Fix OOM killer inaccuracy on large many-core systems
From: Roman Gushchin @ 2026-01-12 17:15 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Andrew Morton, linux-kernel, Paul E. McKenney, Steven Rostedt,
Masami Hiramatsu, Dennis Zhou, Tejun Heo, Christoph Lameter,
Martin Liu, David Rientjes, christian.koenig, Shakeel Butt,
SeongJae Park, Michal Hocko, Johannes Weiner, Sweet Tea Dorminy,
Lorenzo Stoakes, Liam R . Howlett, Mike Rapoport,
Suren Baghdasaryan, Vlastimil Babka, Christian Brauner, Wei Yang,
David Hildenbrand, Miaohe Lin, Al Viro, linux-mm,
linux-trace-kernel, Yu Zhao, Mateusz Guzik, Matthew Wilcox,
Baolin Wang, Aboorva Devarajan
In-Reply-To: <6fbb17fe-f2b1-4233-9834-5a5020cd87b3@efficios.com>
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:
> On 2026-01-11 12:48, Andrew Morton wrote:
>> On Sun, 11 Jan 2026 10:02:46 -0500 Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>>
>>> Notable changes for v12:
>>>
>>> - Reduce per-CPU counters memory allocation size to sizeof long
>>> (fixing mixup with sizeof intermediate cache line aligned items).
>>> - Use "long" counters types rather than "int".
>>> - get_mm_counter_sum() returns a precise sum.
>>> - Introduce and use functions to calculate the min/max possible precise
>>> sum values associated with an approximate sum.
>> May I ask, as an early adopter, what is your overall impression of
>> the Gemini reviewbot?
>
> The review comments were all spot-on. This is the level of review I
> would expect from a good reviewer who spends a significant amount of
> effort digging into the proposed change to make sure the type limits
> are OK for the intended purpose stated in the commit message and that
> the intent stated in comments match the code.
>
> As a patch author, I find this feedback really useful. Is there
> an easy way to get this feedback privately before sending out my
> patches ?
If you need to review a limited number of patches, the easiest way to
use gemini cli/claude code or similar tools with a consumer grade
subscription (most are $20/month these days).
I maintain a pre-configured environment for Gemini:
git@github.com:rgushchin/kengp.git , but it's not hard to hack something
similar for other tools.
Thanks!
^ permalink raw reply
* Re: [PATCH] MAINTAINERS: add Rust files to STATIC BRANCH/CALL and TRACING
From: Steven Rostedt @ 2026-01-12 16:54 UTC (permalink / raw)
To: Alice Ryhl
Cc: Miguel Ojeda, Masami Hiramatsu, Mathieu Desnoyers, Peter Zijlstra,
Josh Poimboeuf, Jason Baron, Ard Biesheuvel, linux-kernel,
linux-trace-kernel, rust-for-linux
In-Reply-To: <aWUO59edshSRit-Y@google.com>
On Mon, 12 Jan 2026 15:10:31 +0000
Alice Ryhl <aliceryhl@google.com> wrote:
> > And have the M of the other sections be R here?
>
> Sure, we can do that.
>
> Are you still willing to pick up the patches? I think that is simpler in
> case there are any series that touch both the C and Rust parts. (Such as
> the initial tracepoint series did.)
Yes. So I guess you can still add me with a 'M:'. But I wanted a separate
section so that all the Rust expertise is still included.
-- Steve
^ permalink raw reply
* Re: [PATCH] MAINTAINERS: add Rust files to STATIC BRANCH/CALL and TRACING
From: Steven Rostedt @ 2026-01-12 16:54 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Alice Ryhl, Masami Hiramatsu, Mathieu Desnoyers, Peter Zijlstra,
Josh Poimboeuf, Jason Baron, Ard Biesheuvel, linux-kernel,
linux-trace-kernel, rust-for-linux
In-Reply-To: <CANiq72nY2xUj0aG4q7YOKxt0EV29Jwos_kjAPD89eE3_irsr0g@mail.gmail.com>
On Mon, 12 Jan 2026 16:11:40 +0100
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> There are several ways subsystems have gone about it:
>
> - Sometimes they may just add the people to the existing entry.
>
> - Sometimes they create sub-entries like you suggest (usually with
> square brackets like " [RUST]" but there is also a " - RUST" used),
> e.g.
>
> BLOCK LAYER DEVICE DRIVER API [RUST]
> MEMORY MANAGEMENT - RUST
>
> - Sometimes they add them into the existing entry but with an added
> "(RUST)" after their names, e.g. Boqun in LOCKING PRIMITIVES:
>
> M: Boqun Feng <boqun.feng@gmail.com> (LOCKDEP & RUST)
OK, add "[RUST]" instead ;-)
-- Steve
^ permalink raw reply
* Re: [PATCH v2 1/6] uaccess: Add copy_from_user_nul helper
From: Alice Ryhl @ 2026-01-12 16:37 UTC (permalink / raw)
To: Steven Rostedt
Cc: Borislav Petkov, Fushuai Wang, akpm, brauner, cyphar, dave.hansen,
hpa, jack, kees, linux-kernel, linux-trace-kernel, luto,
mathieu.desnoyers, mhiramat, mingo, peterz, tglx, vmalik,
wangfushuai, x86, yury.norov
In-Reply-To: <20260112112842.5d5171c3@gandalf.local.home>
On Mon, Jan 12, 2026 at 5:28 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon, 12 Jan 2026 15:40:53 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > I'm getting the impression that strncpy_from_user() is the real problem
> > here. It should append a nul byte.
> >
> > That would also be a lot less error-prone than the suggested new API.
> > Requiring a length of N+1 is not the most immediately-obvious API.
>
> Should we make a strscpy_from_user() API that acts the same as strscpy()
> but from user space?
Sounds quite reasonable to me.
Alice
^ permalink raw reply
* Re: [PATCH v2 1/6] uaccess: Add copy_from_user_nul helper
From: Steven Rostedt @ 2026-01-12 16:28 UTC (permalink / raw)
To: Alice Ryhl
Cc: Borislav Petkov, Fushuai Wang, akpm, brauner, cyphar, dave.hansen,
hpa, jack, kees, linux-kernel, linux-trace-kernel, luto,
mathieu.desnoyers, mhiramat, mingo, peterz, tglx, vmalik,
wangfushuai, x86, yury.norov
In-Reply-To: <aWUWBfKhP8DvzhDl@google.com>
On Mon, 12 Jan 2026 15:40:53 +0000
Alice Ryhl <aliceryhl@google.com> wrote:
> I'm getting the impression that strncpy_from_user() is the real problem
> here. It should append a nul byte.
>
> That would also be a lot less error-prone than the suggested new API.
> Requiring a length of N+1 is not the most immediately-obvious API.
Should we make a strscpy_from_user() API that acts the same as strscpy()
but from user space?
-- Steve
^ permalink raw reply
* Re: [PATCH] arm64/mm: Fix annotated branch unbootable kernel
From: Steven Rostedt @ 2026-01-12 15:46 UTC (permalink / raw)
To: Breno Leitao
Cc: Will Deacon, Catalin Marinas, Mark Rutland, Laura Abbott,
linux-arm-kernel, linux-kernel, linux-trace-kernel,
Masami Hiramatsu, puranjay, usamaarif642, kernel-team, stable
In-Reply-To: <74f52o5tq2nodc3otsvknrsf2rpzphtaba7lxia5u3i7322vni@giqfw3ofnnyk>
On Mon, 12 Jan 2026 01:42:47 -0800
Breno Leitao <leitao@debian.org> wrote:
> > OK, so by adding -DDISABLE_BRANCH_PROFILING to the Makefile configs and for
> > the files that were audited, could be opt-in?
>
> How to do the audit in this case? I suppose we want to disable branch
> profiling for files that have any function that would eventually call
> noinstr functions, right?
IIUC, noinstr is mostly used for the transition between user space and the
kernel (interrupts, exceptions, syscalls, etc). There shouldn't be any
random calls to noinstr functions unless it's going into user space, right?
-- Steve
^ permalink raw reply
* Re: [PATCH v5] tracing: Guard __DECLARE_TRACE() use of __DO_TRACE_CALL() with SRCU-fast
From: Peter Zijlstra @ 2026-01-12 15:44 UTC (permalink / raw)
To: Steven Rostedt
Cc: Mathieu Desnoyers, Alexei Starovoitov, LKML, Linux trace kernel,
bpf, Masami Hiramatsu, Paul E. McKenney,
Sebastian Andrzej Siewior, Thomas Gleixner, Linus Torvalds
In-Reply-To: <20260112103612.41dd4f03@gandalf.local.home>
On Mon, Jan 12, 2026 at 10:36:12AM -0500, Steven Rostedt wrote:
> On Mon, 12 Jan 2026 16:31:28 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
>
> > > OUCH! So migrate disable/enable has a much larger overhead when executed in
> > > a module than in the kernel? This means all spin_locks() in modules
> > > converted to mutexes in PREEMPT_RT are taking this hit!
> >
> > Not so, the migrate_disable() for PREEMPT_RT is still in core code --
> > kernel/locking/spinlock_rt.c is very much not build as a module.
>
> True. But still, wouldn't it be cleaner to have that variable separate from
> the run queue and make the code a bit simpler?
I still don't like exporting variables either way around. Exporting
functions is much saner.
^ permalink raw reply
* Re: [PATCH v2 3/6] tracing: Use copy_from_user_nul() instead of copy_from_user()
From: Steven Rostedt @ 2026-01-12 15:43 UTC (permalink / raw)
To: Fushuai Wang
Cc: tglx, peterz, mathieu.desnoyers, akpm, aliceryhl, yury.norov,
vmalik, kees, dave.hansen, luto, mingo, bp, hpa, mhiramat,
brauner, jack, cyphar, linux-kernel, x86, linux-trace-kernel,
wangfushuai
In-Reply-To: <20260112073039.1185-4-fushuai.wang@linux.dev>
On Mon, 12 Jan 2026 15:30:36 +0800
Fushuai Wang <fushuai.wang@linux.dev> wrote:
> From: Fushuai Wang <wangfushuai@baidu.com>
>
> Use copy_from_user_nul() instead of copy_from_user() to simplify
> the code.
>
> No functional change.
>
> Signed-off-by: Fushuai Wang <wangfushuai@baidu.com>
Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>
-- Steve
> ---
> kernel/trace/trace.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index baec63134ab6..b6ffd006fcf9 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -11266,10 +11266,9 @@ ssize_t trace_parse_run_command(struct file *file, const char __user *buffer,
> if (size >= WRITE_BUFSIZE)
> size = WRITE_BUFSIZE - 1;
>
> - if (copy_from_user(kbuf, buffer + done, size))
> + if (copy_from_user_nul(kbuf, buffer + done, size))
> return -EFAULT;
>
> - kbuf[size] = '\0';
> buf = kbuf;
> do {
> tmp = strchr(buf, '\n');
^ permalink raw reply
* Re: [PATCH v2 1/6] uaccess: Add copy_from_user_nul helper
From: Alice Ryhl @ 2026-01-12 15:40 UTC (permalink / raw)
To: Borislav Petkov
Cc: Fushuai Wang, akpm, brauner, cyphar, dave.hansen, hpa, jack, kees,
linux-kernel, linux-trace-kernel, luto, mathieu.desnoyers,
mhiramat, mingo, peterz, rostedt, tglx, vmalik, wangfushuai, x86,
yury.norov
In-Reply-To: <20260112132238.GDaWT1ntPxY8VD1hlH@fat_crate.local>
On Mon, Jan 12, 2026 at 02:22:38PM +0100, Borislav Petkov wrote:
> On Mon, Jan 12, 2026 at 08:22:36PM +0800, Fushuai Wang wrote:
> > > strncpy_from_user() succeeds even if userspace data does not contain a
> > > nul. Then it reads length bytes.
> >
> > Yes, but if there is no NUL byte in the user buf, whether you use
> > strncpy_from_user() or copy_from_user(), you need to manually add
> > a '\0' in the kernel buf to ensure it is properly NUL-terminated.
>
> This looks like a bunch of churn to save a "= \0" line.
>
> The more important question, IMO, would be whether there are cases in the
> kernel which *miss* a NUL termination, audit them and fix them.
>
> That'll give you a better idea whether such a *_nul() helper is even needed.
>
> Because converting only a handful of obvious places in the face of thousands
> of copy_from_user() invocations in the kernel is not doing anything useful.
I'm getting the impression that strncpy_from_user() is the real problem
here. It should append a nul byte.
That would also be a lot less error-prone than the suggested new API.
Requiring a length of N+1 is not the most immediately-obvious API.
Alice
^ permalink raw reply
* Re: [PATCH v5] tracing: Guard __DECLARE_TRACE() use of __DO_TRACE_CALL() with SRCU-fast
From: Steven Rostedt @ 2026-01-12 15:36 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Mathieu Desnoyers, Alexei Starovoitov, LKML, Linux trace kernel,
bpf, Masami Hiramatsu, Paul E. McKenney,
Sebastian Andrzej Siewior, Thomas Gleixner, Linus Torvalds
In-Reply-To: <20260112153128.GO830755@noisy.programming.kicks-ass.net>
On Mon, 12 Jan 2026 16:31:28 +0100
Peter Zijlstra <peterz@infradead.org> wrote:
> > OUCH! So migrate disable/enable has a much larger overhead when executed in
> > a module than in the kernel? This means all spin_locks() in modules
> > converted to mutexes in PREEMPT_RT are taking this hit!
>
> Not so, the migrate_disable() for PREEMPT_RT is still in core code --
> kernel/locking/spinlock_rt.c is very much not build as a module.
True. But still, wouldn't it be cleaner to have that variable separate from
the run queue and make the code a bit simpler?
As now it doesn't look like it will even bother tracing, as it appears that
only BPF would need it. So this would just be a clean up.
-- Steve
^ permalink raw reply
* Re: [PATCH v5] tracing: Guard __DECLARE_TRACE() use of __DO_TRACE_CALL() with SRCU-fast
From: Peter Zijlstra @ 2026-01-12 15:31 UTC (permalink / raw)
To: Steven Rostedt
Cc: Mathieu Desnoyers, Alexei Starovoitov, LKML, Linux trace kernel,
bpf, Masami Hiramatsu, Paul E. McKenney,
Sebastian Andrzej Siewior, Thomas Gleixner, Linus Torvalds
In-Reply-To: <20260109160202.22975aa4@gandalf.local.home>
On Fri, Jan 09, 2026 at 04:02:02PM -0500, Steven Rostedt wrote:
> On Fri, 9 Jan 2026 15:21:19 -0500
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>
> > * preempt disable/enable pair: 1.1 ns
> > * srcu-fast lock/unlock: 1.5 ns
> >
> > CONFIG_RCU_REF_SCALE_TEST=y
> > * migrate disable/enable pair: 3.0 ns
> > * calls to migrate disable/enable pair within noinline functions: 17.0 ns
> >
> > CONFIG_RCU_REF_SCALE_TEST=m
> > * migrate disable/enable pair: 22.0 ns
>
> OUCH! So migrate disable/enable has a much larger overhead when executed in
> a module than in the kernel? This means all spin_locks() in modules
> converted to mutexes in PREEMPT_RT are taking this hit!
Not so, the migrate_disable() for PREEMPT_RT is still in core code --
kernel/locking/spinlock_rt.c is very much not build as a module.
^ permalink raw reply
* Re: [PATCH v3 3/3] PCI: dw-rockchip: Add pcie_ltssm_state_transition trace support
From: Steven Rostedt @ 2026-01-12 15:16 UTC (permalink / raw)
To: Shawn Lin
Cc: Manivannan Sadhasivam, Bjorn Helgaas, linux-rockchip, linux-pci,
linux-trace-kernel, linux-doc, Masami Hiramatsu
In-Reply-To: <1768180800-63364-4-git-send-email-shawn.lin@rock-chips.com>
On Mon, 12 Jan 2026 09:20:00 +0800
Shawn Lin <shawn.lin@rock-chips.com> wrote:
> Rockchip platforms provide a 64x4 bytes debug FIFO to trace the
> LTSSM history. Any LTSSM change will be recorded. It's userful
> for debug purpose, for example link failure, etc.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
> Changes in v3:
> - reorder variables(Mani)
> - rename loop to i; rename en to enable(Mani)
> - use FIELD_GET(Mani)
> - add comment about how the FIFO works(Mani)
>
> Changes in v2:
> - use tracepoint
>
> drivers/pci/controller/dwc/pcie-dw-rockchip.c | 104 ++++++++++++++++++++++++++
> 1 file changed, 104 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 352f513..344e0b9 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -22,6 +22,8 @@
> #include <linux/platform_device.h>
> #include <linux/regmap.h>
> #include <linux/reset.h>
> +#include <linux/workqueue.h>
> +#include <trace/events/pci_controller.h>
>
> #include "../../pci.h"
> #include "pcie-designware.h"
> @@ -73,6 +75,20 @@
> #define PCIE_CLIENT_CDM_RASDES_TBA_L1_1 BIT(4)
> #define PCIE_CLIENT_CDM_RASDES_TBA_L1_2 BIT(5)
>
> +/* Debug FIFO information */
> +#define PCIE_CLIENT_DBG_FIFO_MODE_CON 0x310
> +#define PCIE_CLIENT_DBG_EN 0xffff0007
> +#define PCIE_CLIENT_DBG_DIS 0xffff0000
> +#define PCIE_CLIENT_DBG_FIFO_PTN_HIT_D0 0x320
> +#define PCIE_CLIENT_DBG_FIFO_PTN_HIT_D1 0x324
> +#define PCIE_CLIENT_DBG_FIFO_TRN_HIT_D0 0x328
> +#define PCIE_CLIENT_DBG_FIFO_TRN_HIT_D1 0x32c
> +#define PCIE_CLIENT_DBG_TRANSITION_DATA 0xffff0000
> +#define PCIE_CLIENT_DBG_FIFO_STATUS 0x350
> +#define PCIE_DBG_FIFO_RATE_MASK GENMASK(22, 20)
> +#define PCIE_DBG_FIFO_L1SUB_MASK GENMASK(10, 8)
> +#define PCIE_DBG_LTSSM_HISTORY_CNT 64
> +
> /* Hot Reset Control Register */
> #define PCIE_CLIENT_HOT_RESET_CTRL 0x180
> #define PCIE_LTSSM_APP_DLY2_EN BIT(1)
> @@ -96,6 +112,7 @@ struct rockchip_pcie {
> struct irq_domain *irq_domain;
> const struct rockchip_pcie_of_data *data;
> bool supports_clkreq;
> + struct delayed_work trace_work;
> };
>
> struct rockchip_pcie_of_data {
> @@ -206,6 +223,89 @@ static enum dw_pcie_ltssm rockchip_pcie_get_ltssm(struct dw_pcie *pci)
> return rockchip_pcie_get_ltssm_reg(rockchip) & PCIE_LTSSM_STATUS_MASK;
> }
>
> +#ifdef CONFIG_TRACING
> +static void rockchip_pcie_ltssm_trace_work(struct work_struct *work)
> +{
> + struct rockchip_pcie *rockchip = container_of(work, struct rockchip_pcie,
> + trace_work.work);
> + struct dw_pcie *pci = &rockchip->pci;
> + enum dw_pcie_ltssm state;
> + u32 i, l1ss, prev_val = DW_PCIE_LTSSM_UNKNOWN, rate, val;
> +
> + for (i = 0; i < PCIE_DBG_LTSSM_HISTORY_CNT; i++) {
> + val = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_DBG_FIFO_STATUS);
> + rate = FIELD_GET(PCIE_DBG_FIFO_RATE_MASK, val);
> + l1ss = FIELD_GET(PCIE_DBG_FIFO_L1SUB_MASK, val);
> + val = FIELD_GET(PCIE_LTSSM_STATUS_MASK, val);
> +
> + /*
> + * Hardware Mechanism: The ring FIFO employs two tracking counters:
> + * - 'last-read-point': maintains the user's last read position
> + * - 'last-valid-point': tracks the hardware's last state update
> + *
> + * Software Handling: When two consecutive LTSSM states are identical,
> + * it indicates invalid subsequent data in the FIFO. In this case, we
> + * skip the remaining entries. The dual-counter design ensures that on
> + * the next state transition, reading can resume from the last user
> + * position.
> + */
> + if ((i > 0 && val == prev_val) || val > DW_PCIE_LTSSM_RCVRY_EQ3)
> + break;
> +
> + state = prev_val = val;
> + if (val == DW_PCIE_LTSSM_L1_IDLE) {
> + if (l1ss == 2)
> + state = DW_PCIE_LTSSM_L1_2;
> + else if (l1ss == 1)
> + state = DW_PCIE_LTSSM_L1_1;
> + }
> +
> + trace_pcie_ltssm_state_transition(dev_name(pci->dev),
> + dw_pcie_ltssm_status_string(state),
> + ((rate + 1) > pci->max_link_speed) ?
> + PCI_SPEED_UNKNOWN : PCIE_SPEED_2_5GT + rate);
> + }
Does it make sense to call this work function every 5 seconds when the
tracepoint isn't enabled?
You can add a function callback to when the tracepoint is enabled by defining:
TRACE_EVENT_FN(<name>
TP_PROTO(..)
TP_ARGS(..)
TP_STRUCT__entry(..)
TP_fast_assign(..)
TP_printk(..)
reg,
unreg)
reg() gets called when the tracepoint is first enabled. This could be where
you can start the work function. And unreg() would stop it.
You would likely need to also include state variables as I guess you don't
want to start it if the link is down. Also, if the tracepoint is enabled
when the link goes up you want to start the work queue.
I would recommend this so that you don't call this work function when it's
not doing anything useful.
-- Steve
> +
> + schedule_delayed_work(&rockchip->trace_work, msecs_to_jiffies(5000));
> +}
> +
^ permalink raw reply
* Re: [PATCH] MAINTAINERS: add Rust files to STATIC BRANCH/CALL and TRACING
From: Miguel Ojeda @ 2026-01-12 15:11 UTC (permalink / raw)
To: Steven Rostedt
Cc: Alice Ryhl, Masami Hiramatsu, Mathieu Desnoyers, Peter Zijlstra,
Josh Poimboeuf, Jason Baron, Ard Biesheuvel, linux-kernel,
linux-trace-kernel, rust-for-linux
In-Reply-To: <20260112100006.7db6d31d@gandalf.local.home>
On Mon, Jan 12, 2026 at 4:00 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> I guess the question is are those with expertise on the list of people to
> Cc?
>
> I think it may be better to have a separate section for RUST so that the
> rust maintainers can be included in the M: part too. Perhaps:
>
> STATIC BRANCH/CALL (RUST)
>
> and
>
> TRACING (RUST)
>
> And have the M of the other sections be R here?
There are several ways subsystems have gone about it:
- Sometimes they may just add the people to the existing entry.
- Sometimes they create sub-entries like you suggest (usually with
square brackets like " [RUST]" but there is also a " - RUST" used),
e.g.
BLOCK LAYER DEVICE DRIVER API [RUST]
MEMORY MANAGEMENT - RUST
- Sometimes they add them into the existing entry but with an added
"(RUST)" after their names, e.g. Boqun in LOCKING PRIMITIVES:
M: Boqun Feng <boqun.feng@gmail.com> (LOCKDEP & RUST)
Cheers,
Miguel
^ permalink raw reply
* Re: [PATCH] MAINTAINERS: add Rust files to STATIC BRANCH/CALL and TRACING
From: Alice Ryhl @ 2026-01-12 15:10 UTC (permalink / raw)
To: Steven Rostedt
Cc: Miguel Ojeda, Masami Hiramatsu, Mathieu Desnoyers, Peter Zijlstra,
Josh Poimboeuf, Jason Baron, Ard Biesheuvel, linux-kernel,
linux-trace-kernel, rust-for-linux
In-Reply-To: <20260112100006.7db6d31d@gandalf.local.home>
On Mon, Jan 12, 2026 at 10:00:06AM -0500, Steven Rostedt wrote:
> On Sun, 11 Jan 2026 19:36:29 +0100
> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>
> > On Wed, Jan 7, 2026 at 1:32 PM Alice Ryhl <aliceryhl@google.com> wrote:
> > >
> > > I'm proposing to include them under the existing entries, as that seems
> > > like the most obvious place to include them. However, there are multiple
> > > ways to do this. If you prefer to have them elsewhere, I would also be
> > > happy to create a new entry with me as maintaining them, or I'm also
> > > willing to be listed as R: under the entry. Regardless of how we list
> > > them in MAINTAINERS, I will be around if anything comes up. Let me know
> > > what you all prefer.
> >
> > I think this was meant to go after the `---`?
> >
> > Whatever Steven et al. are happy with, this looks fine, thanks!
> >
> > Acked-by: Miguel Ojeda <ojeda@kernel.org>
> >
>
> I guess the question is are those with expertise on the list of people to
> Cc?
I would be on cc either way as RUST includes F: rust/.
> I think it may be better to have a separate section for RUST so that the
> rust maintainers can be included in the M: part too. Perhaps:
>
> STATIC BRANCH/CALL (RUST)
>
> and
>
> TRACING (RUST)
>
> And have the M of the other sections be R here?
Sure, we can do that.
Are you still willing to pick up the patches? I think that is simpler in
case there are any series that touch both the C and Rust parts. (Such as
the initial tracepoint series did.)
Alice
^ permalink raw reply
* Re: [PATCH v12 0/3] mm: Fix OOM killer inaccuracy on large many-core systems
From: Steven Rostedt @ 2026-01-12 15:05 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Andrew Morton, linux-kernel, Paul E. McKenney, Masami Hiramatsu,
Dennis Zhou, Tejun Heo, Christoph Lameter, Martin Liu,
David Rientjes, christian.koenig, Shakeel Butt, SeongJae Park,
Michal Hocko, Johannes Weiner, Sweet Tea Dorminy, Lorenzo Stoakes,
Liam R . Howlett, Mike Rapoport, Suren Baghdasaryan,
Vlastimil Babka, Christian Brauner, Wei Yang, David Hildenbrand,
Miaohe Lin, Al Viro, linux-mm, linux-trace-kernel, Yu Zhao,
Roman Gushchin, Mateusz Guzik, Matthew Wilcox, Baolin Wang,
Aboorva Devarajan
In-Reply-To: <6fbb17fe-f2b1-4233-9834-5a5020cd87b3@efficios.com>
On Sun, 11 Jan 2026 13:04:59 -0500
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> As a patch author, I find this feedback really useful. Is there
> an easy way to get this feedback privately before sending out my
> patches ?
Hehe, we all want to look good don't we ;-)
For me. I let people see the good, the bad, and the ugly (probably more than I should!)
-- Steve
^ permalink raw reply
* Re: [PATCH] MAINTAINERS: add Rust files to STATIC BRANCH/CALL and TRACING
From: Steven Rostedt @ 2026-01-12 15:00 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Alice Ryhl, Masami Hiramatsu, Mathieu Desnoyers, Peter Zijlstra,
Josh Poimboeuf, Jason Baron, Ard Biesheuvel, linux-kernel,
linux-trace-kernel, rust-for-linux
In-Reply-To: <CANiq72nV0Z_UjdAwT=41TjjfmKnT6WL4hVNcxg_QCe7TAEYK_Q@mail.gmail.com>
On Sun, 11 Jan 2026 19:36:29 +0100
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> On Wed, Jan 7, 2026 at 1:32 PM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > I'm proposing to include them under the existing entries, as that seems
> > like the most obvious place to include them. However, there are multiple
> > ways to do this. If you prefer to have them elsewhere, I would also be
> > happy to create a new entry with me as maintaining them, or I'm also
> > willing to be listed as R: under the entry. Regardless of how we list
> > them in MAINTAINERS, I will be around if anything comes up. Let me know
> > what you all prefer.
>
> I think this was meant to go after the `---`?
>
> Whatever Steven et al. are happy with, this looks fine, thanks!
>
> Acked-by: Miguel Ojeda <ojeda@kernel.org>
>
I guess the question is are those with expertise on the list of people to
Cc?
I think it may be better to have a separate section for RUST so that the
rust maintainers can be included in the M: part too. Perhaps:
STATIC BRANCH/CALL (RUST)
and
TRACING (RUST)
And have the M of the other sections be R here?
-- Steve
^ permalink raw reply
* [PATCH AUTOSEL 6.18-5.10] ring-buffer: Avoid softlockup in ring_buffer_resize() during memory free
From: Sasha Levin @ 2026-01-12 14:58 UTC (permalink / raw)
To: patches, stable
Cc: Wupeng Ma, mathieu.desnoyers, Masami Hiramatsu (Google),
Steven Rostedt (Google), Sasha Levin, linux-kernel,
linux-trace-kernel
In-Reply-To: <20260112145840.724774-1-sashal@kernel.org>
From: Wupeng Ma <mawupeng1@huawei.com>
[ Upstream commit 6435ffd6c7fcba330dfa91c58dc30aed2df3d0bf ]
When user resize all trace ring buffer through file 'buffer_size_kb',
then in ring_buffer_resize(), kernel allocates buffer pages for each
cpu in a loop.
If the kernel preemption model is PREEMPT_NONE and there are many cpus
and there are many buffer pages to be freed, it may not give up cpu
for a long time and finally cause a softlockup.
To avoid it, call cond_resched() after each cpu buffer free as Commit
f6bd2c92488c ("ring-buffer: Avoid softlockup in ring_buffer_resize()")
does.
Detailed call trace as follow:
rcu: INFO: rcu_sched self-detected stall on CPU
rcu: 24-....: (14837 ticks this GP) idle=521c/1/0x4000000000000000 softirq=230597/230597 fqs=5329
rcu: (t=15004 jiffies g=26003221 q=211022 ncpus=96)
CPU: 24 UID: 0 PID: 11253 Comm: bash Kdump: loaded Tainted: G EL 6.18.2+ #278 NONE
pc : arch_local_irq_restore+0x8/0x20
arch_local_irq_restore+0x8/0x20 (P)
free_frozen_page_commit+0x28c/0x3b0
__free_frozen_pages+0x1c0/0x678
___free_pages+0xc0/0xe0
free_pages+0x3c/0x50
ring_buffer_resize.part.0+0x6a8/0x880
ring_buffer_resize+0x3c/0x58
__tracing_resize_ring_buffer.part.0+0x34/0xd8
tracing_resize_ring_buffer+0x8c/0xd0
tracing_entries_write+0x74/0xd8
vfs_write+0xcc/0x288
ksys_write+0x74/0x118
__arm64_sys_write+0x24/0x38
Cc: <mathieu.desnoyers@efficios.com>
Link: https://patch.msgid.link/20251228065008.2396573-1-mawupeng1@huawei.com
Signed-off-by: Wupeng Ma <mawupeng1@huawei.com>
Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
LLM Generated explanations, may be completely bogus:
## Analysis of Commit: ring-buffer: Avoid softlockup in
ring_buffer_resize() during memory free
### 1. COMMIT MESSAGE ANALYSIS
The commit message clearly describes:
- **Problem**: When resizing trace ring buffers on systems with many
CPUs and PREEMPT_NONE, the kernel can hold the CPU for too long while
freeing buffer pages, causing a softlockup (RCU stall)
- **Evidence**: Includes a complete stack trace showing the issue on a
96-CPU system with kernel 6.18.2+
- **Solution**: Add `cond_resched()` after each buffer page free,
matching what was done in commit f6bd2c92488c for a different code
path
- **Validation**: Acked by Masami Hiramatsu and signed off by Steven
Rostedt (the ring buffer subsystem maintainer)
Keywords present: "softlockup", "self-detected stall", "rcu_sched" - all
indicate a real, user-visible bug.
### 2. CODE CHANGE ANALYSIS
The change is minimal and surgical:
```c
list_for_each_entry_safe(bpage, tmp, &cpu_buffer->new_pages, list) {
list_del_init(&bpage->list);
free_buffer_page(bpage);
+
+ cond_resched();
}
```
This is in the `out_err:` error handling path of `ring_buffer_resize()`.
The loop iterates over all buffer pages to free them on error cleanup.
On systems with many CPUs and many pages, this loop can run for a very
long time without yielding.
**Technical mechanism**:
- `cond_resched()` checks if the scheduler needs to preempt the current
task
- On PREEMPT_NONE kernels, voluntary preemption points like this are the
only way to yield
- This is a standard, well-established kernel pattern for long-running
loops
### 3. CLASSIFICATION
- **Bug type**: Softlockup fix - prevents RCU stalls and potential
watchdog timeouts
- **Not a feature**: Does not add new functionality, just prevents a
hang
- **Not an exception category**: Standard bug fix, not device
ID/quirk/DT
### 4. SCOPE AND RISK ASSESSMENT
| Metric | Assessment |
|--------|------------|
| Lines changed | 2 (1 blank + 1 cond_resched) |
| Files touched | 1 |
| Complexity | Trivial |
| Risk | Very Low |
**Risk is minimal because**:
- `cond_resched()` is designed to be safe to call anywhere
- It's a no-op when no rescheduling is needed
- The same pattern already exists in this function (per the referenced
commit)
- Only affects the error path during cleanup
### 5. USER IMPACT
- **Affected users**: Systems with many CPUs, PREEMPT_NONE
configuration, using tracing
- **Severity**: High - softlockups can trigger watchdog resets and
system hangs
- **Trigger**: User-initiated action (writing to buffer_size_kb file)
- **Real-world**: Stack trace demonstrates this was hit on production
hardware (96-CPU ARM64 system)
This is particularly relevant for enterprise servers and embedded
systems which commonly use PREEMPT_NONE for performance.
### 6. STABILITY INDICATORS
- **Reviewed by maintainer**: Steven Rostedt (ring buffer maintainer)
signed off
- **Acked by**: Masami Hiramatsu (Google) - experienced kernel tracing
developer
- **Follows established pattern**: References commit f6bd2c92488c which
applied the same fix elsewhere
### 7. DEPENDENCY CHECK
- The `ring_buffer_resize()` function and its error path have existed
for years
- `cond_resched()` is a fundamental kernel API available in all stable
trees
- No dependencies on new code or recent changes
- The fix is self-contained and independent
### CONCLUSION
This commit is an **ideal stable backport candidate**:
1. **Obviously correct**: Adding `cond_resched()` to prevent softlockups
is a textbook kernel pattern
2. **Fixes a real bug**: Softlockups are serious user-visible issues
that can hang systems
3. **Small and contained**: 2-line change, single file, trivial
complexity
4. **No new features**: Pure bug fix with no behavioral changes
5. **No API changes**: No userspace-visible modifications
6. **Very low risk**: `cond_resched()` is designed to be safe; worst
case is slightly increased latency
7. **Maintainer approved**: Signed off by the subsystem maintainer
8. **Affects all stable trees**: The vulnerable code path exists in all
supported stable kernels
The fix follows the exact pattern already applied in the same function
by a previous commit, demonstrating it's a proven solution. The risk-to-
benefit ratio is excellent: near-zero risk for fixing a potentially
system-hanging bug.
**YES**
kernel/trace/ring_buffer.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index afcd3747264d2..3ba08fc1b7d05 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -3121,6 +3121,8 @@ int ring_buffer_resize(struct trace_buffer *buffer, unsigned long size,
list) {
list_del_init(&bpage->list);
free_buffer_page(bpage);
+
+ cond_resched();
}
}
out_err_unlock:
--
2.51.0
^ permalink raw reply related
* Re: [PATCH v1] tools/rtla: Fix parse_cpu_set() and add unit test
From: Tomas Glozar @ 2026-01-12 14:50 UTC (permalink / raw)
To: Costa Shulyupin
Cc: Steven Rostedt, Wander Lairson Costa, Ivan Pravdin, Tiezhu Yang,
linux-kernel, linux-trace-kernel
In-Reply-To: <20260112132140.156956-1-costa.shul@redhat.com>
po 12. 1. 2026 v 14:22 odesílatel Costa Shulyupin
<costa.shul@redhat.com> napsal:
>
> The patch "Replace atoi() with a robust strtoi()" introduced a bug
> in parse_cpu_set(), which relies on partial parsing of the input
> string.
>
> Restore the original use of atoi() in parse_cpu_set().
>
> Add a unit test to prevent accidental regressions.
>
> Fixes: 7e9dfccf8f11 ("rtla: Replace atoi() with a robust strtoi()")
>
> Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
The convention is not to insert a blank line between Fixes and Signed-off-by.
> ---
> tools/tracing/rtla/Makefile | 3 ++
> tools/tracing/rtla/src/utils.c | 10 ++--
> tools/tracing/rtla/tests/Makefile | 12 +++++
> tools/tracing/rtla/tests/test_utils.c | 74 +++++++++++++++++++++++++++
> 4 files changed, 93 insertions(+), 6 deletions(-)
> create mode 100644 tools/tracing/rtla/tests/Makefile
> create mode 100644 tools/tracing/rtla/tests/test_utils.c
>
Could you split this into three commits?
1. Fix the bug.
2. Add unit test framework.
3. Add the specific unit tests for utils.
IMHO, it would be easier to modify the test in tests/timerlat.t to
cover this case for now, and implement unit tests later in an entirely
separate patchset. (See comments for the unit tests below.)
> diff --git a/tools/tracing/rtla/Makefile b/tools/tracing/rtla/Makefile
> index 2701256abaf3..1805916c7dba 100644
> --- a/tools/tracing/rtla/Makefile
> +++ b/tools/tracing/rtla/Makefile
> @@ -109,7 +109,10 @@ clean: doc_clean fixdep-clean
> $(Q)rm -f rtla rtla-static fixdep FEATURE-DUMP rtla-*
> $(Q)rm -rf feature
> $(Q)rm -f src/timerlat.bpf.o src/timerlat.skel.h example/timerlat_bpf_action.o
> +
> check: $(RTLA) tests/bpf/bpf_action_map.o
> + make -C tests/ check
> RTLA=$(RTLA) BPFTOOL=$(SYSTEM_BPFTOOL) prove -o -f -v tests/
> +
> examples: example/timerlat_bpf_action.o
> .PHONY: FORCE clean check
The unit tests should be a separate target, so that you can run unit
tests and end-to-end tests separately. Also, the directory structure
should reflect that there are two types of tests. For example, in
rteval, Test:Harness-based end-to-end tests (inspired by and very
similar to RTLA tests) are in tests/e2e while unit tests are in tests/
directly; it might be even better to have them in tests/unit.
> diff --git a/tools/tracing/rtla/tests/test_utils.c b/tools/tracing/rtla/tests/test_utils.c
> new file mode 100644
> index 000000000000..92ed49d60d33
> --- /dev/null
> +++ b/tools/tracing/rtla/tests/test_utils.c
> @@ -0,0 +1,74 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Unit tests for src/utils.c parsing functions
> + */
> +
> +#define _GNU_SOURCE
> +#include <check.h>
> +#include <unistd.h>
> +
> +#include "../src/utils.h"
> +
Here, you add an external dependency on libcheck. That has to be
documented in both the commit message and README, and it should also
be added to tools/build dependency detection (in a separate commit)
first before being added to RTLA, so that RTLA can check for the
dependency and disable unit tests if it is not present.
> diff --git a/tools/tracing/rtla/tests/Makefile b/tools/tracing/rtla/tests/Makefile
> new file mode 100644
> index 000000000000..fe187306a404
> --- /dev/null
> +++ b/tools/tracing/rtla/tests/Makefile
> @@ -0,0 +1,12 @@
> +LIBS := -lcheck
> +
> +test_utils: test_utils.c ../src/utils.c
> + $(CC) $(CFLAGS) -o $@ $^ $(LIBS)
> +
> +check: test_utils
> + ./test_utils
> +
> +clean:
> + rm -f test_utils
> +
> +.PHONY: check clean
Is there a reason for doing a separate Makefile for the tests? That
will require explicitly passing variables to the child make during the
build. It appears to me that it would be more natural to just include
unit tests into the main Makefile (or a file included from it, e.g.
Makefile.test), which already covers end-to-end tests. That would also
allow it to use RTLA build system's dependency detection.
Thanks,
Tomas
^ permalink raw reply
* Re: [PATCH v5] tracing: Guard __DECLARE_TRACE() use of __DO_TRACE_CALL() with SRCU-fast
From: Steven Rostedt @ 2026-01-12 13:53 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mathieu Desnoyers, LKML, Linux trace kernel, bpf,
Masami Hiramatsu, Paul E. McKenney, Sebastian Andrzej Siewior,
Thomas Gleixner
In-Reply-To: <CAADnVQJiEhDrfYVEyV8eGUECE_XFt7PGG=PFJRKU4jRBn-TsvA@mail.gmail.com>
On Sun, 11 Jan 2026 15:38:38 -0800
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > Oh, so you are OK replacing the preempt_disable in the tracepoint
> > callbacks with fast SRCU?
>
> yes, but..
>
> > Then I guess we can simply do that. Would it be fine to do that for
> > both RT and non-RT? That will simplify the code quite a bit.
>
> Agree. perf needs preempt_disable in their callbacks (as this patch does)
> and bpf side needs to add migrate_disable in __bpf_trace_run for now.
> Though syscall tracepoints are sleepable we don't take advantage of
> that on the bpf side. Eventually we will, and then rcu_lock
> inside __bpf_trace_run will become srcu_fast_lock.
>
> The way to think about generic infrastructure like tracepoints is
> to minimize their overhead no matter what out-of-tree and in-tree
> users' assumptions are today, so why do we need preempt_disable
> or srcu_fast there?
Either preempt disable or srcu_fast is still needed.
> I think today it's there because all callbacks (perf, ftrace, bpf)
> expect preemption to be disabled, but can we just remove it from tp side?
> and move preempt_disable to callbacks that actually need it?
Yes if we are talking about switching from preempt_disable to src_fast.
No if you mean to remove both as it still needs RCU protection. It's
used for synchronizing changes in the tracepoint infrastructure itself.
That __DO_TRACE_CALL() macro is the guts of the tracepoint callback
code. It needs to handle races with additions and removals of callbacks
there, as the callbacks also get data passed to them. If it gets out of
sync, a callback could be called with another callback's data.
That's why it has:
it_func_ptr = \
rcu_dereference_raw((&__tracepoint_##name)->funcs);
>
> I'm looking at release_probes(). It's fine as-is, no?
That's just freeing, and as you see, there's RCU synchronization
required.
Updates to tracepoints require RCU protection. It started out with
preempt_disable() for all tracepoints (which was synchronized with
synchronized_sched() which later became just synchronize_rcu()).
The issue that came about was that both ftrace and perf had an
assumption that its tracepoint callbacks always have preempt disabled
when being called. To move to srcu_fast() that is no longer the case.
And we need to do that for PREEMPT_RT if BPF is running very long
callbacks to the tracepoints.
Ftrace has been fixed to not require it, but still needs to take into
account if tracepoints disable preemption or not so that it can display
the preempt_count() of when the tracepoint was called correctly.
Perf is trivial to fix as it can simply add a preempt_disable() in its
handler.
But we were originally told that BPF had an issue because it had the
assumption that tracepoint callbacks wouldn't migrate. That's where
this patch came about.
Now if you are saying that BPF will handle migrate_disable() on its own
and not require the tracepoint infrastructure to do it for it, then
this is perfect. And I can then simplify this code, and just use
srcu_fast for both RT and !RT.
-- Steve
^ permalink raw reply
* Re: [PATCH v2 1/6] uaccess: Add copy_from_user_nul helper
From: Borislav Petkov @ 2026-01-12 13:22 UTC (permalink / raw)
To: Fushuai Wang
Cc: aliceryhl, akpm, brauner, cyphar, dave.hansen, hpa, jack, kees,
linux-kernel, linux-trace-kernel, luto, mathieu.desnoyers,
mhiramat, mingo, peterz, rostedt, tglx, vmalik, wangfushuai, x86,
yury.norov
In-Reply-To: <20260112122236.22214-1-fushuai.wang@linux.dev>
On Mon, Jan 12, 2026 at 08:22:36PM +0800, Fushuai Wang wrote:
> > strncpy_from_user() succeeds even if userspace data does not contain a
> > nul. Then it reads length bytes.
>
> Yes, but if there is no NUL byte in the user buf, whether you use
> strncpy_from_user() or copy_from_user(), you need to manually add
> a '\0' in the kernel buf to ensure it is properly NUL-terminated.
This looks like a bunch of churn to save a "= \0" line.
The more important question, IMO, would be whether there are cases in the
kernel which *miss* a NUL termination, audit them and fix them.
That'll give you a better idea whether such a *_nul() helper is even needed.
Because converting only a handful of obvious places in the face of thousands
of copy_from_user() invocations in the kernel is not doing anything useful.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* [PATCH v1] tools/rtla: Fix parse_cpu_set() and add unit test
From: Costa Shulyupin @ 2026-01-12 13:21 UTC (permalink / raw)
To: Steven Rostedt, Tomas Glozar, Wander Lairson Costa,
Costa Shulyupin, Ivan Pravdin, Tiezhu Yang, linux-kernel,
linux-trace-kernel
The patch "Replace atoi() with a robust strtoi()" introduced a bug
in parse_cpu_set(), which relies on partial parsing of the input
string.
Restore the original use of atoi() in parse_cpu_set().
Add a unit test to prevent accidental regressions.
Fixes: 7e9dfccf8f11 ("rtla: Replace atoi() with a robust strtoi()")
Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
---
tools/tracing/rtla/Makefile | 3 ++
tools/tracing/rtla/src/utils.c | 10 ++--
tools/tracing/rtla/tests/Makefile | 12 +++++
tools/tracing/rtla/tests/test_utils.c | 74 +++++++++++++++++++++++++++
4 files changed, 93 insertions(+), 6 deletions(-)
create mode 100644 tools/tracing/rtla/tests/Makefile
create mode 100644 tools/tracing/rtla/tests/test_utils.c
diff --git a/tools/tracing/rtla/Makefile b/tools/tracing/rtla/Makefile
index 2701256abaf3..1805916c7dba 100644
--- a/tools/tracing/rtla/Makefile
+++ b/tools/tracing/rtla/Makefile
@@ -109,7 +109,10 @@ clean: doc_clean fixdep-clean
$(Q)rm -f rtla rtla-static fixdep FEATURE-DUMP rtla-*
$(Q)rm -rf feature
$(Q)rm -f src/timerlat.bpf.o src/timerlat.skel.h example/timerlat_bpf_action.o
+
check: $(RTLA) tests/bpf/bpf_action_map.o
+ make -C tests/ check
RTLA=$(RTLA) BPFTOOL=$(SYSTEM_BPFTOOL) prove -o -f -v tests/
+
examples: example/timerlat_bpf_action.o
.PHONY: FORCE clean check
diff --git a/tools/tracing/rtla/src/utils.c b/tools/tracing/rtla/src/utils.c
index 18986a5aed3c..0da3b2470c31 100644
--- a/tools/tracing/rtla/src/utils.c
+++ b/tools/tracing/rtla/src/utils.c
@@ -128,18 +128,16 @@ int parse_cpu_set(char *cpu_list, cpu_set_t *set)
nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
for (p = cpu_list; *p; ) {
- if (strtoi(p, &cpu))
- goto err;
- if (cpu < 0 || cpu >= nr_cpus)
+ cpu = atoi(p);
+ if (cpu < 0 || (!cpu && *p != '0') || cpu >= nr_cpus)
goto err;
while (isdigit(*p))
p++;
if (*p == '-') {
p++;
- if (strtoi(p, &end_cpu))
- goto err;
- if (end_cpu < cpu || end_cpu >= nr_cpus)
+ end_cpu = atoi(p);
+ if (end_cpu < cpu || (!end_cpu && *p != '0') || end_cpu >= nr_cpus)
goto err;
while (isdigit(*p))
p++;
diff --git a/tools/tracing/rtla/tests/Makefile b/tools/tracing/rtla/tests/Makefile
new file mode 100644
index 000000000000..fe187306a404
--- /dev/null
+++ b/tools/tracing/rtla/tests/Makefile
@@ -0,0 +1,12 @@
+LIBS := -lcheck
+
+test_utils: test_utils.c ../src/utils.c
+ $(CC) $(CFLAGS) -o $@ $^ $(LIBS)
+
+check: test_utils
+ ./test_utils
+
+clean:
+ rm -f test_utils
+
+.PHONY: check clean
diff --git a/tools/tracing/rtla/tests/test_utils.c b/tools/tracing/rtla/tests/test_utils.c
new file mode 100644
index 000000000000..92ed49d60d33
--- /dev/null
+++ b/tools/tracing/rtla/tests/test_utils.c
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Unit tests for src/utils.c parsing functions
+ */
+
+#define _GNU_SOURCE
+#include <check.h>
+#include <unistd.h>
+
+#include "../src/utils.h"
+
+START_TEST(test_parse_cpu_set)
+{
+ cpu_set_t set;
+ int nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
+
+ ck_assert_int_eq(parse_cpu_set("0", &set), 0);
+ ck_assert(CPU_ISSET(0, &set));
+ ck_assert(!CPU_ISSET(1, &set));
+
+ if (nr_cpus > 2) {
+ ck_assert_int_eq(parse_cpu_set("0,2", &set), 0);
+ ck_assert(CPU_ISSET(0, &set));
+ ck_assert(CPU_ISSET(2, &set));
+ }
+
+ if (nr_cpus > 3) {
+ ck_assert_int_eq(parse_cpu_set("0-3", &set), 0);
+ ck_assert(CPU_ISSET(0, &set));
+ ck_assert(CPU_ISSET(1, &set));
+ ck_assert(CPU_ISSET(2, &set));
+ ck_assert(CPU_ISSET(3, &set));
+ }
+
+ if (nr_cpus > 5) {
+ ck_assert_int_eq(parse_cpu_set("1-3,5", &set), 0);
+ ck_assert(!CPU_ISSET(0, &set));
+ ck_assert(CPU_ISSET(1, &set));
+ ck_assert(CPU_ISSET(2, &set));
+ ck_assert(CPU_ISSET(3, &set));
+ ck_assert(!CPU_ISSET(4, &set));
+ ck_assert(CPU_ISSET(5, &set));
+ }
+
+ ck_assert_int_ne(parse_cpu_set("-1", &set), 0);
+ ck_assert_int_ne(parse_cpu_set("abc", &set), 0);
+ ck_assert_int_ne(parse_cpu_set("9999", &set), 0);
+}
+END_TEST
+
+Suite *utils_suite(void)
+{
+ Suite *s = suite_create("utils");
+ TCase *tc = tcase_create("core");
+
+ tcase_add_test(tc, test_parse_cpu_set);
+
+ suite_add_tcase(s, tc);
+ return s;
+}
+
+int main(void)
+{
+ int num_failed;
+ SRunner *sr;
+
+ sr = srunner_create(utils_suite());
+ srunner_run_all(sr, CK_NORMAL);
+ num_failed = srunner_ntests_failed(sr);
+
+ srunner_free(sr);
+
+ return (num_failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
+}
--
2.52.0
^ permalink raw reply related
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