* Re: [PATCH 13/15] spi: Use trace_invoke_##name() at guarded tracepoint call sites
From: Mark Brown @ 2026-03-12 16:54 UTC (permalink / raw)
To: Steven Rostedt
Cc: Vineeth Pillai (Google), Peter Zijlstra, Michael Hennerich,
Nuno Sá, David Lechner, linux-spi, linux-kernel,
linux-trace-kernel
In-Reply-To: <20260312114254.6467c03d@gandalf.local.home>
[-- Attachment #1: Type: text/plain, Size: 521 bytes --]
On Thu, Mar 12, 2026 at 11:42:54AM -0400, Steven Rostedt wrote:
> Mark Brown <broonie@kernel.org> wrote:
> > though not loving the naming here, I'll have a hard time figuring out
> > what the weird call is about next time I look at that code
> Would:
> trace_call__foo()
> Be better?
> Or do you have another name we should use?
Possibly an _unchecked or something? Honestly the suggestion someone
had for _do seemed OK to me. Part of it is that I wouldn't think of
tracepoints as being something that I'd call.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 00/15] tracepoint: Avoid double static_branch evaluation at guarded call sites
From: Andrii Nakryiko @ 2026-03-12 16:54 UTC (permalink / raw)
To: Vineeth Remanan Pillai
Cc: Mathieu Desnoyers, Steven Rostedt, Peter Zijlstra,
Dmitry Ilvokhin, Masami Hiramatsu, Ingo Molnar, Jens Axboe,
io-uring, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Alexei Starovoitov, Daniel Borkmann,
Marcelo Ricardo Leitner, Xin Long, Jon Maloy, Aaron Conole,
Eelco Chaudron, Ilya Maximets, netdev, bpf, linux-sctp,
tipc-discussion, dev, Oded Gabbay, Koby Elbaz, dri-devel,
Rafael J. Wysocki, Viresh Kumar, Gautham R. Shenoy, Huang Rui,
Mario Limonciello, Len Brown, Srinivas Pandruvada, linux-pm,
MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Christian König,
Sumit Semwal, linaro-mm-sig, Eddie James, Andrew Jeffery,
Joel Stanley, linux-fsi, David Airlie, Simona Vetter,
Alex Deucher, Danilo Krummrich, Matthew Brost, Philipp Stanner,
Harry Wentland, Leo Li, amd-gfx, Jiri Kosina, Benjamin Tissoires,
linux-input, Wolfram Sang, linux-i2c, Mark Brown,
Michael Hennerich, Nuno Sá, linux-spi, James E.J. Bottomley,
Martin K. Petersen, linux-scsi, Chris Mason, David Sterba,
linux-btrfs, linux-trace-kernel, linux-kernel
In-Reply-To: <CAO7JXPjnnruhM5oC6xMgnYaQ9efzYFqMCFiJLNM3HCQ+ZeCiJw@mail.gmail.com>
On Thu, Mar 12, 2026 at 9:15 AM Vineeth Remanan Pillai
<vineeth@bitbyteword.org> wrote:
>
> On Thu, Mar 12, 2026 at 11:49 AM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
> >
> > On 2026-03-12 11:40, Steven Rostedt wrote:
> > > On Thu, 12 Mar 2026 11:28:07 -0400
> > > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> > >
> > >>> Note, Vineeth came up with the naming. I would have done "do" but when I
> > >>> saw "invoke" I thought it sounded better.
> > >>
> > >> It works as long as you don't have a tracing subsystem called
> > >> "invoke", then you get into identifier clash territory.
> > >
> > > True. Perhaps we should do the double underscore trick.
> > >
> > > Instead of: trace_invoke_foo()
> > >
> > > use: trace_invoke__foo()
> > >
> > >
> > > Which will make it more visible to what the trace event is.
> > >
> > > Hmm, we probably should have used: trace__foo() for all tracepoints, as
> > > there's still functions that are called trace_foo() that are not
> > > tracepoints :-p
> >
> > One certain way to eliminate identifier clash would be to go for a
> > prefix to "trace_", e.g.
> >
> > do_trace_foo()
> > call_trace_foo()
>
> This was the initial idea, but it had conflict in the existing source:
> call_trace_sched_update_nr_running. do_trace_##name also had
> collisions when I checked. So, went with trace_invoke_##name. Did not
> check rest of the suggestions here though.
>
> Thanks,
> Vineeth
>
> > emit_trace_foo()
> > __trace_foo()
this seems like the best approach, IMO. double-underscored variants
are usually used for some specialized/internal version of a function
when we know that some conditions are correct (e.g., lock is already
taken, or something like that). Which fits here: trace_xxx() will
check if tracepoint is enabled, while __trace_xxx() will not check and
just invoke the tracepoint? It's short, it's distinct, and it says "I
know what I am doing".
> > invoke_trace_foo()
> > dispatch_trace_foo()
> >
> > Thanks,
> >
> > Mathieu
> >
> >
> >
> > --
> > Mathieu Desnoyers
> > EfficiOS Inc.
> > https://www.efficios.com
>
^ permalink raw reply
* Re: [PATCH 00/61] treewide: Use IS_ERR_OR_NULL over manual NULL check - refactor
From: Jason Gunthorpe @ 2026-03-12 16:54 UTC (permalink / raw)
To: James Bottomley
Cc: Kuan-Wei Chiu, Philipp Hahn, amd-gfx, apparmor, bpf, ceph-devel,
cocci, dm-devel, dri-devel, gfs2, intel-gfx, intel-wired-lan,
iommu, kvm, linux-arm-kernel, linux-block, linux-bluetooth,
linux-btrfs, linux-cifs, linux-clk, linux-erofs, linux-ext4,
linux-fsdevel, linux-gpio, linux-hyperv, linux-input,
linux-kernel, linux-leds, linux-media, linux-mips, linux-mm,
linux-modules, linux-mtd, linux-nfs, linux-omap, linux-phy,
linux-pm, linux-rockchip, linux-s390, linux-scsi, linux-sctp,
linux-security-module, linux-sh, linux-sound, linux-stm32,
linux-trace-kernel, linux-usb, linux-wireless, netdev, ntfs3,
samba-technical, sched-ext, target-devel, tipc-discussion, v9fs
In-Reply-To: <f5688b895eaebabae6545a0d9baf8f1404e8454e.camel@HansenPartnership.com>
On Thu, Mar 12, 2026 at 11:32:37AM -0400, James Bottomley wrote:
> On Thu, 2026-03-12 at 09:57 -0300, Jason Gunthorpe wrote:
> > On Wed, Mar 11, 2026 at 02:40:36AM +0800, Kuan-Wei Chiu wrote:
> >
> > > IMHO, the necessity of IS_ERR_OR_NULL() often highlights a
> > > confusing or flawed API design. It usually implies that the caller
> > > is unsure whether a failure results in an error pointer or a NULL
> > > pointer.
> >
> > +1
> >
> > IS_ERR_OR_NULL() should always be looked on with suspicion. Very
> > little should be returning some tri-state 'ERR' 'NULL' 'SUCCESS'
> > pointer. What does the middle condition even mean? IS_ERR_OR_NULL()
> > implies ERR and NULL are semanticly the same, so fix the things to
> > always use ERR.
>
> Not in any way supporting the original patch. However, the pattern
> ERR, NULL, PTR is used extensively in the dentry code of filesystems.
> See the try_lookup..() set of functions in fs/namei.c
>
> The meaning is
>
> PTR - I found it
> NULL - It definitely doesn't exist
> ERR - something went wrong during the lookup.
>
> So I don't think you can blanket say this pattern is wrong.
Lots of places also would return ENOENT, I'd argue that is easier to
use..
But yes, I did use the word "suspicion" not blanket wrong :)
Jason
^ permalink raw reply
* Re: [PATCH 13/15] spi: Use trace_invoke_##name() at guarded tracepoint call sites
From: Steven Rostedt @ 2026-03-12 17:00 UTC (permalink / raw)
To: Mark Brown
Cc: Vineeth Pillai (Google), Peter Zijlstra, Michael Hennerich,
Nuno Sá, David Lechner, linux-spi, linux-kernel,
linux-trace-kernel
In-Reply-To: <06438818-2e3e-426f-aec5-ea6344e4b057@sirena.org.uk>
On Thu, 12 Mar 2026 16:54:00 +0000
Mark Brown <broonie@kernel.org> wrote:
> Possibly an _unchecked or something? Honestly the suggestion someone
> had for _do seemed OK to me. Part of it is that I wouldn't think of
> tracepoints as being something that I'd call.
The "__do_trace.." is an internal function I don't want to expose.
I'm thinking of: call_trace_foo(), as that should be pretty obvious to what
it is.
I want to avoid the do_trace_foo() because that's usually the name of the
wrapper code that is done in header files. Where the header calls:
do_trace_foo()
and the C file has:
void do_trace_foo(..)
{
trace_foo(..);
}
Which could be:
void do_trace_foo(..)
{
call_trace(..);
}
And remove the static branch there.
-- Steve
^ permalink raw reply
* Re: [PATCH 00/15] tracepoint: Avoid double static_branch evaluation at guarded call sites
From: Steven Rostedt @ 2026-03-12 17:02 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Vineeth Remanan Pillai, Mathieu Desnoyers, Peter Zijlstra,
Dmitry Ilvokhin, Masami Hiramatsu, Ingo Molnar, Jens Axboe,
io-uring, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Alexei Starovoitov, Daniel Borkmann,
Marcelo Ricardo Leitner, Xin Long, Jon Maloy, Aaron Conole,
Eelco Chaudron, Ilya Maximets, netdev, bpf, linux-sctp,
tipc-discussion, dev, Oded Gabbay, Koby Elbaz, dri-devel,
Rafael J. Wysocki, Viresh Kumar, Gautham R. Shenoy, Huang Rui,
Mario Limonciello, Len Brown, Srinivas Pandruvada, linux-pm,
MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Christian König,
Sumit Semwal, linaro-mm-sig, Eddie James, Andrew Jeffery,
Joel Stanley, linux-fsi, David Airlie, Simona Vetter,
Alex Deucher, Danilo Krummrich, Matthew Brost, Philipp Stanner,
Harry Wentland, Leo Li, amd-gfx, Jiri Kosina, Benjamin Tissoires,
linux-input, Wolfram Sang, linux-i2c, Mark Brown,
Michael Hennerich, Nuno Sá, linux-spi, James E.J. Bottomley,
Martin K. Petersen, linux-scsi, Chris Mason, David Sterba,
linux-btrfs, linux-trace-kernel, linux-kernel
In-Reply-To: <CAEf4BzbnfyhCqp0ne=2gRnVxp-mdGmuZwDeFRyhRYH+eDcz2-w@mail.gmail.com>
On Thu, 12 Mar 2026 09:54:29 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> > > emit_trace_foo()
> > > __trace_foo()
>
> this seems like the best approach, IMO. double-underscored variants
> are usually used for some specialized/internal version of a function
> when we know that some conditions are correct (e.g., lock is already
> taken, or something like that). Which fits here: trace_xxx() will
> check if tracepoint is enabled, while __trace_xxx() will not check and
> just invoke the tracepoint? It's short, it's distinct, and it says "I
> know what I am doing".
Honestly, I consider double underscore as internal only and not something
anyone but the subsystem maintainers use.
This, is a normal function where it's just saying: If you have it already
enabled, then you can use this. Thus, I don't think it qualifies as a "you
know what you are doing".
Perhaps: call_trace_foo() ?
-- Steve
^ permalink raw reply
* Re: [PATCH v3 4/4] trace/preemptirq: Implement trace_irqflags hooks
From: Wander Lairson Costa @ 2026-03-12 17:09 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann,
Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider,
Masami Hiramatsu, Mathieu Desnoyers, Andrew Morton, open list,
open list:TRACING, acme, williams, gmonaco
In-Reply-To: <20260311194305.GT606826@noisy.programming.kicks-ass.net>
On Wed, Mar 11, 2026 at 08:43:05PM +0100, Peter Zijlstra wrote:
> On Wed, Mar 11, 2026 at 09:50:18AM -0300, Wander Lairson Costa wrote:
> > +#define local_irq_enable() \
> > + do { \
> > + if (tracepoint_enabled(irq_enable)) \
> > + trace_local_irq_enable(); \
>
> I'm thinking you didn't even look at the assembly generated :/
Yes, I did. But I was more concerned with the hot path, making sure it
only adds the NOP instruction generated by the static_key machinery.
>
> Otherwise you would have written this like:
>
> if (tracepoint_enabled(irq_enable))
> __do_trace_local_irq_enable();
>
> > + raw_local_irq_enable(); \
> > + } while (0)
>
Steve already opposed to using internal functions. When trace invoke
public API is available, we can always come back and replace the call.
> Again, this was one instruction, and you clearly didn't bother looking
> at the mess you've generated :/
>
I might have missed something (as was the case for preempt_enable), but
I spent quite some time looking at the assembly code. I did my best, but
there lots of people far more skilled than me. And patch submission is
the way to connect to these people.
^ permalink raw reply
* Re: [PATCH v3 1/4] tracing/preemptirq: Optimize preempt_disable/enable() tracepoint overhead
From: Wander Lairson Costa @ 2026-03-12 17:19 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann,
Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider,
Masami Hiramatsu, Mathieu Desnoyers, Andrew Morton,
open list:SCHEDULER, open list:TRACING, acme, williams, gmonaco
In-Reply-To: <20260311193503.GS606826@noisy.programming.kicks-ass.net>
On Wed, Mar 11, 2026 at 08:35:03PM +0100, Peter Zijlstra wrote:
> On Wed, Mar 11, 2026 at 09:50:15AM -0300, Wander Lairson Costa wrote:
>
> > +extern void __trace_preempt_on(void);
> > +extern void __trace_preempt_off(void);
> > +
> > +DECLARE_TRACEPOINT(preempt_enable);
> > +DECLARE_TRACEPOINT(preempt_disable);
> > +
> > +#define __preempt_trace_enabled(type, val) \
> > + (tracepoint_enabled(preempt_##type) && preempt_count() == (val))
> > +
> > +static __always_inline void preempt_count_add(int val)
> > +{
> > + __preempt_count_add(val);
> > +
> > + if (__preempt_trace_enabled(disable, val))
> > + __trace_preempt_off();
> > +}
> > +
> > +static __always_inline void preempt_count_sub(int val)
> > +{
> > + if (__preempt_trace_enabled(enable, val))
> > + __trace_preempt_on();
> > +
> > + __preempt_count_sub(val);
> > +}
> > #else
> > #define preempt_count_add(val) __preempt_count_add(val)
> > #define preempt_count_sub(val) __preempt_count_sub(val)
> > #define preempt_count_dec_and_test() __preempt_count_dec_and_test()
> > #endif
> >
> > +#if defined(CONFIG_DEBUG_PREEMPT) || defined(CONFIG_TRACE_PREEMPT_TOGGLE)
> > +#define preempt_count_dec_and_test() \
> > + ({ preempt_count_sub(1); should_resched(0); })
> > +#endif
>
> Why!?!
>
> Why can't you simply have:
>
> static __always_inline bool preempt_count_dec_and_test(void)
> {
> if (__preempt_trace_enabled(enable, 1))
> __trace_preempt_on();
>
> return __preempt_count_dec_and_test();
> }
Indeed it improved the generated code. Thanks a lot.
>
> Also, given how !x86 architectures were just complaining about how
> terrible their preempt_emable() is, I'm really not liking this much at
> all.
>
I will look deeper in arm64 and riscv generated code. If there are other
specific architectures that concerns you, please let me know.
> Currently the x86 preempt_disable() is _1_ instruction and
> preempt_enable() is all of 3. Adding in these tracepoints will bloat
> every single such site by at least another 4-5.
>
Yes, there is a tradeoff. As I said before, I am looking at the hot path
(tracepoint no activated). Furthermore, when CONFIG_TRACE_PREEMPT_TOGGLE
and CONFIG_TRACE_IRQFLAGS_TOGGLE are off (default config), the code
generated is the same, byte by byte, of the stock kernel.
> That's significant bloat, for really very little gain. Realistically
> nobody is going to need these.
>
Of course, I can't speak for others, but more than once I debugged issues
that those tracepoints had made my life far easier. Those cases convinced
me that such a feature would be worth it. But if you don't see
value and will reject the patches no matter what, nothing can be done,
and I will have to accept defeat.
^ permalink raw reply
* [PATCH] tracing: Generate undef symbols allowlist for simple_ring_buffer
From: Vincent Donnefort @ 2026-03-12 18:20 UTC (permalink / raw)
To: maz
Cc: rostedt, arnd, nathan, linux-trace-kernel, kvmarm, kernel-team,
Vincent Donnefort
Compiler and tooling-generated symbols are difficult to maintain
across all supported architectures. Make the allowlist more robust by
replacing the harcoded list with a mechanism that automatically detects
these symbols.
This mechanism generates a C function designed to trigger common
compiler-inserted symbols.
Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
index beb15936829d..3b427b76434a 100644
--- a/kernel/trace/Makefile
+++ b/kernel/trace/Makefile
@@ -136,17 +136,37 @@ obj-$(CONFIG_TRACE_REMOTE_TEST) += remote_test.o
# simple_ring_buffer is used by the pKVM hypervisor which does not have access
# to all kernel symbols. Fail the build if forbidden symbols are found.
#
-UNDEFINED_ALLOWLIST := memset alt_cb_patch_nops __x86 __ubsan __asan __kasan __gcov __aeabi_unwind
-UNDEFINED_ALLOWLIST += __stack_chk_fail stackleak_track_stack __ref_stack __sanitizer llvm_gcda llvm_gcov
-UNDEFINED_ALLOWLIST += .TOC\. __clear_pages_unrolled __memmove copy_page warn_slowpath_fmt
-UNDEFINED_ALLOWLIST += ftrace_likely_update __hwasan_load __hwasan_store __hwasan_tag_memory
-UNDEFINED_ALLOWLIST += warn_bogus_irq_restore __stack_chk_guard
-UNDEFINED_ALLOWLIST := $(addprefix -e , $(UNDEFINED_ALLOWLIST))
+# undefsyms_base generates a set of compiler and tooling-generated symbols that can
+# safely be ignored for simple_ring_buffer.
+#
+$(obj)/undefsyms_base.c: FORCE
+ $(Q)echo '#include <asm/page.h>' > $@
+ $(Q)echo '#include <asm/local.h>' >> $@
+ $(Q)echo 'static char page[PAGE_SIZE] __aligned(PAGE_SIZE);' >> $@
+ $(Q)echo 'void undefsyms_base(int n);' >> $@
+ $(Q)echo 'void undefsyms_base(int n) {' >> $@
+ $(Q)echo ' char buffer[256] = { 0 };' >> $@
+ $(Q)echo ' u32 u = 0;' >> $@
+ $(Q)echo ' memset((char * volatile)page, 8, PAGE_SIZE);' >> $@
+ $(Q)echo ' memset((char * volatile)buffer, 8, sizeof(buffer));' >> $@
+ $(Q)echo ' cmpxchg((u32 * volatile)&u, 0, 8);' >> $@
+ $(Q)echo ' WARN_ON(n == 0xdeadbeef);' >> $@
+ $(Q)echo '}' >> $@
+
+clean-files += undefsyms_base.c
+targets += undefsyms_base.c
+
+$(obj)/undefsyms_base.o: $(obj)/undefsyms_base.c
+
+extra-y += undefsyms_base.o
+
+UNDEFINED_ALLOWLIST = __asan __gcov __kasan __kcsan __hwasan __sanitizer __tsan __ubsan __x86_indirect_thunk \
+ $(shell $(NM) -u $(obj)/undefsyms_base.o 2>/dev/null | awk '{print $$2}')
quiet_cmd_check_undefined = NM $<
- cmd_check_undefined = test -z "`$(NM) -u $< | grep -v $(UNDEFINED_ALLOWLIST)`"
+ cmd_check_undefined = test -z "`$(NM) -u $< | grep -v $(addprefix -e ,$(UNDEFINED_ALLOWLIST))`"
-$(obj)/%.o.checked: $(obj)/%.o FORCE
+$(obj)/%.o.checked: $(obj)/%.o $(obj)/undefsyms_base.o FORCE
$(call if_changed,check_undefined)
always-$(CONFIG_SIMPLE_RING_BUFFER) += simple_ring_buffer.o.checked
base-commit: 33f2e266515717c4b2df585dadefa0525557726c
--
2.53.0.851.ga537e3e6e9-goog
^ permalink raw reply related
* Re: [PATCH] tracing: Generate undef symbols allowlist for simple_ring_buffer
From: Steven Rostedt @ 2026-03-12 18:43 UTC (permalink / raw)
To: Vincent Donnefort
Cc: maz, arnd, nathan, linux-trace-kernel, kvmarm, kernel-team
In-Reply-To: <20260312182010.111013-1-vdonnefort@google.com>
On Thu, 12 Mar 2026 18:20:10 +0000
Vincent Donnefort <vdonnefort@google.com> wrote:
> Compiler and tooling-generated symbols are difficult to maintain
> across all supported architectures. Make the allowlist more robust by
> replacing the harcoded list with a mechanism that automatically detects
> these symbols.
>
> This mechanism generates a C function designed to trigger common
> compiler-inserted symbols.
>
> Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
>
> diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
> index beb15936829d..3b427b76434a 100644
> --- a/kernel/trace/Makefile
> +++ b/kernel/trace/Makefile
> @@ -136,17 +136,37 @@ obj-$(CONFIG_TRACE_REMOTE_TEST) += remote_test.o
> # simple_ring_buffer is used by the pKVM hypervisor which does not have access
> # to all kernel symbols. Fail the build if forbidden symbols are found.
> #
> -UNDEFINED_ALLOWLIST := memset alt_cb_patch_nops __x86 __ubsan __asan __kasan __gcov __aeabi_unwind
> -UNDEFINED_ALLOWLIST += __stack_chk_fail stackleak_track_stack __ref_stack __sanitizer llvm_gcda llvm_gcov
> -UNDEFINED_ALLOWLIST += .TOC\. __clear_pages_unrolled __memmove copy_page warn_slowpath_fmt
> -UNDEFINED_ALLOWLIST += ftrace_likely_update __hwasan_load __hwasan_store __hwasan_tag_memory
> -UNDEFINED_ALLOWLIST += warn_bogus_irq_restore __stack_chk_guard
> -UNDEFINED_ALLOWLIST := $(addprefix -e , $(UNDEFINED_ALLOWLIST))
> +# undefsyms_base generates a set of compiler and tooling-generated symbols that can
> +# safely be ignored for simple_ring_buffer.
> +#
> +$(obj)/undefsyms_base.c: FORCE
> + $(Q)echo '#include <asm/page.h>' > $@
> + $(Q)echo '#include <asm/local.h>' >> $@
> + $(Q)echo 'static char page[PAGE_SIZE] __aligned(PAGE_SIZE);' >> $@
> + $(Q)echo 'void undefsyms_base(int n);' >> $@
> + $(Q)echo 'void undefsyms_base(int n) {' >> $@
> + $(Q)echo ' char buffer[256] = { 0 };' >> $@
> + $(Q)echo ' u32 u = 0;' >> $@
> + $(Q)echo ' memset((char * volatile)page, 8, PAGE_SIZE);' >> $@
> + $(Q)echo ' memset((char * volatile)buffer, 8, sizeof(buffer));' >> $@
> + $(Q)echo ' cmpxchg((u32 * volatile)&u, 0, 8);' >> $@
> + $(Q)echo ' WARN_ON(n == 0xdeadbeef);' >> $@
> + $(Q)echo '}' >> $@
Looking at the scripts/Makefile.* files, it looks like the proper way to do
C files is to make it a single command:
Perhaps something like:
$(obj)/undefsyms_base.c: FORCE
$(Q)( \
echo '#include <asm/page.h>'; \
echo '#include <asm/local.h>'; \
echo 'static char page[PAGE_SIZE] __aligned(PAGE_SIZE);'; \
echo 'void undefsyms_base(int n);'; \
echo 'void undefsyms_base(int n) {'; \
echo ' char buffer[256] = { 0 };'; \
echo ' u32 u = 0;'; \
echo ' memset((char * volatile)page, 8, PAGE_SIZE);'; \
echo ' memset((char * volatile)buffer, 8, sizeof(buffer));'; \
echo ' cmpxchg((u32 * volatile)&u, 0, 8);'; \
echo ' WARN_ON(n == 0xdeadbeef);'; \
echo '}' ) > $@
-- Steve
> +
> +clean-files += undefsyms_base.c
> +targets += undefsyms_base.c
> +
> +$(obj)/undefsyms_base.o: $(obj)/undefsyms_base.c
> +
> +extra-y += undefsyms_base.o
> +
> +UNDEFINED_ALLOWLIST = __asan __gcov __kasan __kcsan __hwasan __sanitizer __tsan __ubsan __x86_indirect_thunk \
> + $(shell $(NM) -u $(obj)/undefsyms_base.o 2>/dev/null | awk '{print $$2}')
>
> quiet_cmd_check_undefined = NM $<
> - cmd_check_undefined = test -z "`$(NM) -u $< | grep -v $(UNDEFINED_ALLOWLIST)`"
> + cmd_check_undefined = test -z "`$(NM) -u $< | grep -v $(addprefix -e ,$(UNDEFINED_ALLOWLIST))`"
>
> -$(obj)/%.o.checked: $(obj)/%.o FORCE
> +$(obj)/%.o.checked: $(obj)/%.o $(obj)/undefsyms_base.o FORCE
> $(call if_changed,check_undefined)
>
> always-$(CONFIG_SIMPLE_RING_BUFFER) += simple_ring_buffer.o.checked
>
> base-commit: 33f2e266515717c4b2df585dadefa0525557726c
^ permalink raw reply
* [PATCH 1/3] lib/bootconfig: fix off-by-one in xbc_verify_tree() unclosed brace error
From: Josh Law @ 2026-03-12 18:45 UTC (permalink / raw)
To: Masami Hiramatsu, Andrew Morton
Cc: Josh Law, linux-kernel, linux-trace-kernel
__xbc_open_brace() pushes entries with post-increment
(open_brace[brace_index++]), so brace_index always points one past
the last valid entry. xbc_verify_tree() reads open_brace[brace_index]
to report which brace is unclosed, but this is one past the last
pushed entry and contains stale/zero data, causing the error message
to reference the wrong node.
Use open_brace[brace_index - 1] to correctly identify the unclosed
brace. brace_index is known to be > 0 here since we are inside the
if (brace_index) guard.
Signed-off-by: Josh Law <objecting@objecting.org>
---
lib/bootconfig.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 2bcd5c2aa87e..a1e6a2e14b01 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -802,7 +802,7 @@ static int __init xbc_verify_tree(void)
/* Brace closing */
if (brace_index) {
- n = &xbc_nodes[open_brace[brace_index]];
+ n = &xbc_nodes[open_brace[brace_index - 1]];
return xbc_parse_error("Brace is not closed",
xbc_node_get_data(n));
}
--
2.34.1
^ permalink raw reply related
* [PATCH 2/3] lib/bootconfig: check bounds before writing in __xbc_open_brace()
From: Josh Law @ 2026-03-12 18:45 UTC (permalink / raw)
To: Masami Hiramatsu, Andrew Morton
Cc: Josh Law, linux-kernel, linux-trace-kernel
In-Reply-To: <20260312184520.24399-1-objecting@objecting.org>
The bounds check for brace_index happens after the array write.
While the current call pattern prevents an actual out-of-bounds
access (the previous call would have returned an error), the
write-before-check pattern is fragile and would become a real
out-of-bounds write if the error return were ever not propagated.
Move the bounds check before the array write so the function is
self-contained and safe regardless of caller behavior.
Signed-off-by: Josh Law <objecting@objecting.org>
---
lib/bootconfig.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index a1e6a2e14b01..62b4ed7a0ba6 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -532,9 +532,9 @@ static char *skip_spaces_until_newline(char *p)
static int __init __xbc_open_brace(char *p)
{
/* Push the last key as open brace */
- open_brace[brace_index++] = xbc_node_index(last_parent);
if (brace_index >= XBC_DEPTH_MAX)
return xbc_parse_error("Exceed max depth of braces", p);
+ open_brace[brace_index++] = xbc_node_index(last_parent);
return 0;
}
--
2.34.1
^ permalink raw reply related
* [PATCH 3/3] lib/bootconfig: fix snprintf truncation check in xbc_node_compose_key_after()
From: Josh Law @ 2026-03-12 18:45 UTC (permalink / raw)
To: Masami Hiramatsu, Andrew Morton
Cc: Josh Law, linux-kernel, linux-trace-kernel
In-Reply-To: <20260312184520.24399-1-objecting@objecting.org>
snprintf() returns the number of characters that would have been
written excluding the NUL terminator. Output is truncated when the
return value is >= the buffer size, not just > the buffer size.
When ret == size, the current code takes the non-truncated path,
advancing buf by ret and reducing size to 0. This is wrong because
the output was actually truncated (the last character was replaced by
NUL). Fix by using >= so the truncation path is taken correctly.
Signed-off-by: Josh Law <objecting@objecting.org>
---
lib/bootconfig.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 62b4ed7a0ba6..b0ef1e74e98a 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -316,7 +316,7 @@ int __init xbc_node_compose_key_after(struct xbc_node *root,
depth ? "." : "");
if (ret < 0)
return ret;
- if (ret > size) {
+ if (ret >= size) {
size = 0;
} else {
size -= ret;
--
2.34.1
^ permalink raw reply related
* Re: [PATCH 06/15] cpufreq: Use trace_invoke_##name() at guarded tracepoint call sites
From: Rafael J. Wysocki @ 2026-03-12 18:58 UTC (permalink / raw)
To: vineeth
Cc: Steven Rostedt, Peter Zijlstra, Huang Rui, Gautham R. Shenoy,
Mario Limonciello, Perry Yuan, Rafael J. Wysocki, Viresh Kumar,
Srinivas Pandruvada, Len Brown, linux-pm, linux-kernel,
linux-trace-kernel
In-Reply-To: <20260312150523.2054552-7-vineeth@bitbyteword.org>
On Thu, Mar 12, 2026 at 4:05 PM Vineeth Pillai (Google)
<vineeth@bitbyteword.org> wrote:
>
> Replace trace_foo() with the new trace_invoke_foo() at sites already
> guarded by trace_foo_enabled(), avoiding a redundant
> static_branch_unlikely() re-evaluation inside the tracepoint.
> trace_invoke_foo() calls the tracepoint callbacks directly without
> utilizing the static branch again.
>
> Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Vineeth Pillai (Google) <vineeth@bitbyteword.org>
> Assisted-by: Claude:claude-sonnet-4-6
Fine with me, so
Acked-by: Rafael J. Wysocki (Intel) <rafael@kernel.org> # cpufreq
> ---
> drivers/cpufreq/amd-pstate.c | 10 +++++-----
> drivers/cpufreq/cpufreq.c | 2 +-
> drivers/cpufreq/intel_pstate.c | 2 +-
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 5aa9fcd80cf51..3fa40a32ef6b5 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -247,7 +247,7 @@ static int msr_update_perf(struct cpufreq_policy *policy, u8 min_perf,
> if (trace_amd_pstate_epp_perf_enabled()) {
> union perf_cached perf = READ_ONCE(cpudata->perf);
>
> - trace_amd_pstate_epp_perf(cpudata->cpu,
> + trace_invoke_amd_pstate_epp_perf(cpudata->cpu,
> perf.highest_perf,
> epp,
> min_perf,
> @@ -298,7 +298,7 @@ static int msr_set_epp(struct cpufreq_policy *policy, u8 epp)
> if (trace_amd_pstate_epp_perf_enabled()) {
> union perf_cached perf = cpudata->perf;
>
> - trace_amd_pstate_epp_perf(cpudata->cpu, perf.highest_perf,
> + trace_invoke_amd_pstate_epp_perf(cpudata->cpu, perf.highest_perf,
> epp,
> FIELD_GET(AMD_CPPC_MIN_PERF_MASK,
> cpudata->cppc_req_cached),
> @@ -343,7 +343,7 @@ static int shmem_set_epp(struct cpufreq_policy *policy, u8 epp)
> if (trace_amd_pstate_epp_perf_enabled()) {
> union perf_cached perf = cpudata->perf;
>
> - trace_amd_pstate_epp_perf(cpudata->cpu, perf.highest_perf,
> + trace_invoke_amd_pstate_epp_perf(cpudata->cpu, perf.highest_perf,
> epp,
> FIELD_GET(AMD_CPPC_MIN_PERF_MASK,
> cpudata->cppc_req_cached),
> @@ -507,7 +507,7 @@ static int shmem_update_perf(struct cpufreq_policy *policy, u8 min_perf,
> if (trace_amd_pstate_epp_perf_enabled()) {
> union perf_cached perf = READ_ONCE(cpudata->perf);
>
> - trace_amd_pstate_epp_perf(cpudata->cpu,
> + trace_invoke_amd_pstate_epp_perf(cpudata->cpu,
> perf.highest_perf,
> epp,
> min_perf,
> @@ -588,7 +588,7 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
> }
>
> if (trace_amd_pstate_perf_enabled() && amd_pstate_sample(cpudata)) {
> - trace_amd_pstate_perf(min_perf, des_perf, max_perf, cpudata->freq,
> + trace_invoke_amd_pstate_perf(min_perf, des_perf, max_perf, cpudata->freq,
> cpudata->cur.mperf, cpudata->cur.aperf, cpudata->cur.tsc,
> cpudata->cpu, fast_switch);
> }
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 277884d91913c..cf57aeb503790 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2222,7 +2222,7 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
>
> if (trace_cpu_frequency_enabled()) {
> for_each_cpu(cpu, policy->cpus)
> - trace_cpu_frequency(freq, cpu);
> + trace_invoke_cpu_frequency(freq, cpu);
> }
>
> return freq;
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 11c58af419006..a0da9b31c4ffe 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -3132,7 +3132,7 @@ static void intel_cpufreq_trace(struct cpudata *cpu, unsigned int trace_type, in
> return;
>
> sample = &cpu->sample;
> - trace_pstate_sample(trace_type,
> + trace_invoke_pstate_sample(trace_type,
> 0,
> old_pstate,
> cpu->pstate.current_pstate,
> --
> 2.53.0
>
^ permalink raw reply
* [PATCH v2 0/3] lib/bootconfig: three bug fixes
From: Josh Law @ 2026-03-12 19:11 UTC (permalink / raw)
To: Masami Hiramatsu, Andrew Morton
Cc: Josh Law, linux-kernel, linux-trace-kernel
Three fixes for lib/bootconfig.c:
1. Fix off-by-one in xbc_verify_tree() unclosed brace error reporting.
2. Check bounds before writing in __xbc_open_brace() to prevent
potential out-of-bounds writes.
3. Fix snprintf truncation check in xbc_node_compose_key_after().
Changes since v1:
- Added explicit From: lines at top of changelog.
Josh Law (3):
lib/bootconfig: fix off-by-one in xbc_verify_tree() unclosed brace
error
lib/bootconfig: check bounds before writing in __xbc_open_brace()
lib/bootconfig: fix snprintf truncation check in
xbc_node_compose_key_after()
lib/bootconfig.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--
2.34.1
^ permalink raw reply
* [PATCH v2 1/3] lib/bootconfig: fix off-by-one in xbc_verify_tree() unclosed brace error
From: Josh Law @ 2026-03-12 19:11 UTC (permalink / raw)
To: Masami Hiramatsu, Andrew Morton
Cc: Josh Law, linux-kernel, linux-trace-kernel
In-Reply-To: <20260312191143.28719-1-objecting@objecting.org>
From: Josh Law <objecting@objecting.org>
__xbc_open_brace() pushes entries with post-increment
(open_brace[brace_index++]), so brace_index always points one past
the last valid entry. xbc_verify_tree() reads open_brace[brace_index]
to report which brace is unclosed, but this is one past the last
pushed entry and contains stale/zero data, causing the error message
to reference the wrong node.
Use open_brace[brace_index - 1] to correctly identify the unclosed
brace. brace_index is known to be > 0 here since we are inside the
if (brace_index) guard.
Signed-off-by: Josh Law <objecting@objecting.org>
---
lib/bootconfig.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 2bcd5c2aa87e..a1e6a2e14b01 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -802,7 +802,7 @@ static int __init xbc_verify_tree(void)
/* Brace closing */
if (brace_index) {
- n = &xbc_nodes[open_brace[brace_index]];
+ n = &xbc_nodes[open_brace[brace_index - 1]];
return xbc_parse_error("Brace is not closed",
xbc_node_get_data(n));
}
--
2.34.1
^ permalink raw reply related
* [PATCH v2 2/3] lib/bootconfig: check bounds before writing in __xbc_open_brace()
From: Josh Law @ 2026-03-12 19:11 UTC (permalink / raw)
To: Masami Hiramatsu, Andrew Morton
Cc: Josh Law, linux-kernel, linux-trace-kernel
In-Reply-To: <20260312191143.28719-1-objecting@objecting.org>
From: Josh Law <objecting@objecting.org>
The bounds check for brace_index happens after the array write.
While the current call pattern prevents an actual out-of-bounds
access (the previous call would have returned an error), the
write-before-check pattern is fragile and would become a real
out-of-bounds write if the error return were ever not propagated.
Move the bounds check before the array write so the function is
self-contained and safe regardless of caller behavior.
Signed-off-by: Josh Law <objecting@objecting.org>
---
lib/bootconfig.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index a1e6a2e14b01..62b4ed7a0ba6 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -532,9 +532,9 @@ static char *skip_spaces_until_newline(char *p)
static int __init __xbc_open_brace(char *p)
{
/* Push the last key as open brace */
- open_brace[brace_index++] = xbc_node_index(last_parent);
if (brace_index >= XBC_DEPTH_MAX)
return xbc_parse_error("Exceed max depth of braces", p);
+ open_brace[brace_index++] = xbc_node_index(last_parent);
return 0;
}
--
2.34.1
^ permalink raw reply related
* [PATCH v2 3/3] lib/bootconfig: fix snprintf truncation check in xbc_node_compose_key_after()
From: Josh Law @ 2026-03-12 19:11 UTC (permalink / raw)
To: Masami Hiramatsu, Andrew Morton
Cc: Josh Law, linux-kernel, linux-trace-kernel
In-Reply-To: <20260312191143.28719-1-objecting@objecting.org>
From: Josh Law <objecting@objecting.org>
snprintf() returns the number of characters that would have been
written excluding the NUL terminator. Output is truncated when the
return value is >= the buffer size, not just > the buffer size.
When ret == size, the current code takes the non-truncated path,
advancing buf by ret and reducing size to 0. This is wrong because
the output was actually truncated (the last character was replaced by
NUL). Fix by using >= so the truncation path is taken correctly.
Signed-off-by: Josh Law <objecting@objecting.org>
---
lib/bootconfig.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 62b4ed7a0ba6..b0ef1e74e98a 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -316,7 +316,7 @@ int __init xbc_node_compose_key_after(struct xbc_node *root,
depth ? "." : "");
if (ret < 0)
return ret;
- if (ret > size) {
+ if (ret >= size) {
size = 0;
} else {
size -= ret;
--
2.34.1
^ permalink raw reply related
* Re: [PATCH 48/61] mtd: Prefer IS_ERR_OR_NULL over manual NULL check
From: Richard Weinberger @ 2026-03-12 19:33 UTC (permalink / raw)
To: Philipp Hahn
Cc: amd-gfx, apparmor, bpf, ceph-devel, cocci, dm-devel,
DRI mailing list, gfs2, intel-gfx, intel-wired-lan, iommu, kvm,
linux-arm-kernel, linux-block, linux-bluetooth, linux-btrfs,
linux-cifs, linux-clk, linux-erofs, linux-ext4, linux-fsdevel,
linux-gpio, linux-hyperv, linux-input, linux-kernel, linux-leds,
linux-media, linux-mips, linux-mm, linux-modules, linux-mtd,
linux-nfs, linux-omap, linux-phy, linux-pm, linux-rockchip,
linux-s390, linux-scsi, linux-sctp, LSM, linux-sh, linux-sound,
linux-stm32, linux-trace-kernel, linux-usb, linux-wireless,
netdev, ntfs3, samba-technical, sched-ext, target-devel,
tipc-discussion, v9fs, Miquel Raynal, Vignesh Raghavendra
In-Reply-To: <20260310-b4-is_err_or_null-v1-48-bd63b656022d@avm.de>
----- Ursprüngliche Mail -----
> Von: "Philipp Hahn" <phahn-oss@avm.de>
> - if (gpiomtd->nwp && !IS_ERR(gpiomtd->nwp))
> + if (!IS_ERR_OR_NULL(gpiomtd->nwp))
No, please don't.
This makes reading the code not easier.
Thanks,
//richard
^ permalink raw reply
* Re: [PATCH mm-unstable v15 01/13] mm/khugepaged: generalize hugepage_vma_revalidate for mTHP support
From: David Hildenbrand (Arm) @ 2026-03-12 20:00 UTC (permalink / raw)
To: Nico Pache, linux-doc, linux-kernel, linux-mm, linux-trace-kernel
Cc: aarcange, akpm, anshuman.khandual, apopple, baohua, baolin.wang,
byungchul, catalin.marinas, cl, corbet, dave.hansen, dev.jain,
gourry, hannes, hughd, jack, jackmanb, jannh, jglisse,
joshua.hahnjy, kas, lance.yang, Liam.Howlett, lorenzo.stoakes,
mathieu.desnoyers, matthew.brost, mhiramat, mhocko, peterx,
pfalcato, rakie.kim, raquini, rdunlap, richard.weiyang, rientjes,
rostedt, rppt, ryan.roberts, shivankg, sunnanyong, surenb,
thomas.hellstrom, tiwai, usamaarif642, vbabka, vishal.moola,
wangkefeng.wang, will, willy, yang, ying.huang, ziy, zokeefe
In-Reply-To: <20260226032217.232353-1-npache@redhat.com>
On 2/26/26 04:22, Nico Pache wrote:
> For khugepaged to support different mTHP orders, we must generalize this
> to check if the PMD is not shared by another VMA and that the order is
> enabled.
>
> No functional change in this patch. Also correct a comment about the
> functionality of the revalidation.
>
> Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
> Reviewed-by: Lance Yang <lance.yang@linux.dev>
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Reviewed-by: Zi Yan <ziy@nvidia.com>
> Co-developed-by: Dev Jain <dev.jain@arm.com>
> Signed-off-by: Dev Jain <dev.jain@arm.com>
> Signed-off-by: Nico Pache <npache@redhat.com>
Acked-by: David Hildenbrand (Arm) <david@kernel.org>
--
Cheers,
David
^ permalink raw reply
* Re: [PATCH mm-unstable v15 02/13] mm/khugepaged: generalize alloc_charge_folio()
From: David Hildenbrand (Arm) @ 2026-03-12 20:05 UTC (permalink / raw)
To: Nico Pache, linux-doc, linux-kernel, linux-mm, linux-trace-kernel
Cc: aarcange, akpm, anshuman.khandual, apopple, baohua, baolin.wang,
byungchul, catalin.marinas, cl, corbet, dave.hansen, dev.jain,
gourry, hannes, hughd, jack, jackmanb, jannh, jglisse,
joshua.hahnjy, kas, lance.yang, Liam.Howlett, lorenzo.stoakes,
mathieu.desnoyers, matthew.brost, mhiramat, mhocko, peterx,
pfalcato, rakie.kim, raquini, rdunlap, richard.weiyang, rientjes,
rostedt, rppt, ryan.roberts, shivankg, sunnanyong, surenb,
thomas.hellstrom, tiwai, usamaarif642, vbabka, vishal.moola,
wangkefeng.wang, will, willy, yang, ying.huang, ziy, zokeefe
In-Reply-To: <20260226032326.232770-1-npache@redhat.com>
On 2/26/26 04:23, Nico Pache wrote:
> From: Dev Jain <dev.jain@arm.com>
>
> Pass order to alloc_charge_folio() and update mTHP statistics.
>
> Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
> Reviewed-by: Lance Yang <lance.yang@linux.dev>
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Reviewed-by: Zi Yan <ziy@nvidia.com>
> Co-developed-by: Nico Pache <npache@redhat.com>
> Signed-off-by: Nico Pache <npache@redhat.com>
> Signed-off-by: Dev Jain <dev.jain@arm.com>
> ---
Acked-by: David Hildenbrand (Arm) <david@kernel.org>
--
Cheers,
David
^ permalink raw reply
* Re: [PATCH 13/15] spi: Use trace_invoke_##name() at guarded tracepoint call sites
From: Mark Brown @ 2026-03-12 20:05 UTC (permalink / raw)
To: Steven Rostedt
Cc: Vineeth Pillai (Google), Peter Zijlstra, Michael Hennerich,
Nuno Sá, David Lechner, linux-spi, linux-kernel,
linux-trace-kernel
In-Reply-To: <20260312130022.669e7de4@gandalf.local.home>
[-- Attachment #1: Type: text/plain, Size: 631 bytes --]
On Thu, Mar 12, 2026 at 01:00:22PM -0400, Steven Rostedt wrote:
> Mark Brown <broonie@kernel.org> wrote:
> > Possibly an _unchecked or something? Honestly the suggestion someone
> > had for _do seemed OK to me. Part of it is that I wouldn't think of
> > tracepoints as being something that I'd call.
> The "__do_trace.." is an internal function I don't want to expose.
> I'm thinking of: call_trace_foo(), as that should be pretty obvious to what
> it is.
Yeah, like I day it was the concept of there being a call in a
tracepoint that was a bit of a surprise. It was more a grumble than
anything else, I did ack the change.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH mm-unstable v15 03/13] mm/khugepaged: generalize __collapse_huge_page_* for mTHP support
From: David Hildenbrand (Arm) @ 2026-03-12 20:32 UTC (permalink / raw)
To: Nico Pache, linux-doc, linux-kernel, linux-mm, linux-trace-kernel
Cc: aarcange, akpm, anshuman.khandual, apopple, baohua, baolin.wang,
byungchul, catalin.marinas, cl, corbet, dave.hansen, dev.jain,
gourry, hannes, hughd, jack, jackmanb, jannh, jglisse,
joshua.hahnjy, kas, lance.yang, Liam.Howlett, lorenzo.stoakes,
mathieu.desnoyers, matthew.brost, mhiramat, mhocko, peterx,
pfalcato, rakie.kim, raquini, rdunlap, richard.weiyang, rientjes,
rostedt, rppt, ryan.roberts, shivankg, sunnanyong, surenb,
thomas.hellstrom, tiwai, usamaarif642, vbabka, vishal.moola,
wangkefeng.wang, will, willy, yang, ying.huang, ziy, zokeefe
In-Reply-To: <20260226032347.232939-1-npache@redhat.com>
On 2/26/26 04:23, Nico Pache wrote:
> generalize the order of the __collapse_huge_page_* functions
> to support future mTHP collapse.
>
> mTHP collapse will not honor the khugepaged_max_ptes_shared or
> khugepaged_max_ptes_swap parameters, and will fail if it encounters a
> shared or swapped entry.
>
> No functional changes in this patch.
>
> Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
> Reviewed-by: Lance Yang <lance.yang@linux.dev>
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> Co-developed-by: Dev Jain <dev.jain@arm.com>
> Signed-off-by: Dev Jain <dev.jain@arm.com>
> Signed-off-by: Nico Pache <npache@redhat.com>
> ---
> mm/khugepaged.c | 73 +++++++++++++++++++++++++++++++------------------
> 1 file changed, 47 insertions(+), 26 deletions(-)
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index a9b645402b7f..ecdbbf6a01a6 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -535,7 +535,7 @@ static void release_pte_pages(pte_t *pte, pte_t *_pte,
>
> static enum scan_result __collapse_huge_page_isolate(struct vm_area_struct *vma,
> unsigned long start_addr, pte_t *pte, struct collapse_control *cc,
> - struct list_head *compound_pagelist)
> + unsigned int order, struct list_head *compound_pagelist)
> {
> struct page *page = NULL;
> struct folio *folio = NULL;
> @@ -543,15 +543,17 @@ static enum scan_result __collapse_huge_page_isolate(struct vm_area_struct *vma,
> pte_t *_pte;
> int none_or_zero = 0, shared = 0, referenced = 0;
> enum scan_result result = SCAN_FAIL;
> + const unsigned long nr_pages = 1UL << order;
> + int max_ptes_none = khugepaged_max_ptes_none >> (HPAGE_PMD_ORDER - order);
It might be a bit more readable to move "const unsigned long
nr_pages = 1UL << order;" all the way to the top.
Then, have here
int max_ptes_none = 0;
and do at the beginning of the function:
/* For MADV_COLLAPSE, we always collapse ... */
if (!cc->is_khugepaged)
max_ptes_none = HPAGE_PMD_NR;
/* ... except if userfaultf relies on MISSING faults. */
if (!userfaultfd_armed(vma))
max_ptes_none = khugepaged_max_ptes_none >> (HPAGE_PMD_ORDER - order);
(but see below regarding helper function)
then the code below becomes ...
>
> - for (_pte = pte; _pte < pte + HPAGE_PMD_NR;
> + for (_pte = pte; _pte < pte + nr_pages;
> _pte++, addr += PAGE_SIZE) {
> pte_t pteval = ptep_get(_pte);
> if (pte_none_or_zero(pteval)) {
> ++none_or_zero;
> if (!userfaultfd_armed(vma) &&
> (!cc->is_khugepaged ||
> - none_or_zero <= khugepaged_max_ptes_none)) {
> + none_or_zero <= max_ptes_none)) {
...
if (none_or_zero <= max_ptes_none) {
I see that you do something like that (but slightly different) in the next
patch. You could easily extend the above by it.
Or go one step further and move all of that conditional into collapse_max_ptes_none(), whereby
you simply also pass the cc and the vma.
Then this all gets cleaned up and you'd end up above with
max_ptes_none = collapse_max_ptes_none(cc, vma, order);
if (max_ptes_none < 0)
return result;
I'd do all that in this patch here, getting rid of #4.
> continue;
> } else {
> result = SCAN_EXCEED_NONE_PTE;
> @@ -585,8 +587,14 @@ static enum scan_result __collapse_huge_page_isolate(struct vm_area_struct *vma,
> /* See collapse_scan_pmd(). */
> if (folio_maybe_mapped_shared(folio)) {
> ++shared;
> - if (cc->is_khugepaged &&
> - shared > khugepaged_max_ptes_shared) {
> + /*
> + * TODO: Support shared pages without leading to further
> + * mTHP collapses. Currently bringing in new pages via
> + * shared may cause a future higher order collapse on a
> + * rescan of the same range.
> + */
> + if (!is_pmd_order(order) || (cc->is_khugepaged &&
> + shared > khugepaged_max_ptes_shared)) {
That's not how we indent within a nested ().
To make this easier to read, what about similarly having at the beginning
of the function:
int max_ptes_shared = 0;
/* For MADV_COLLAPSE, we always collapse. */
if (cc->is_khugepaged)
max_ptes_none = HPAGE_PMD_NR;
/* TODO ... */
if (is_pmd_order(order))
max_ptes_none = khugepaged_max_ptes_shared;
to turn this code into a
if (shared > khugepaged_max_ptes_shared)
Also, here, might make sense to have a collapse_max_ptes_swap(cc, order)
to do that and clean it up.
> result = SCAN_EXCEED_SHARED_PTE;
> count_vm_event(THP_SCAN_EXCEED_SHARED_PTE);
> goto out;
> @@ -679,18 +687,18 @@ static enum scan_result __collapse_huge_page_isolate(struct vm_area_struct *vma,
> }
>
> static void __collapse_huge_page_copy_succeeded(pte_t *pte,
> - struct vm_area_struct *vma,
> - unsigned long address,
> - spinlock_t *ptl,
> - struct list_head *compound_pagelist)
> + struct vm_area_struct *vma, unsigned long address,
> + spinlock_t *ptl, unsigned int order,
> + struct list_head *compound_pagelist)
> {
> - unsigned long end = address + HPAGE_PMD_SIZE;
> + unsigned long end = address + (PAGE_SIZE << order);
> struct folio *src, *tmp;
> pte_t pteval;
> pte_t *_pte;
> unsigned int nr_ptes;
> + const unsigned long nr_pages = 1UL << order;
Move it further to the top.
>
> - for (_pte = pte; _pte < pte + HPAGE_PMD_NR; _pte += nr_ptes,
> + for (_pte = pte; _pte < pte + nr_pages; _pte += nr_ptes,
> address += nr_ptes * PAGE_SIZE) {
> nr_ptes = 1;
> pteval = ptep_get(_pte);
> @@ -743,13 +751,11 @@ static void __collapse_huge_page_copy_succeeded(pte_t *pte,
> }
>
> static void __collapse_huge_page_copy_failed(pte_t *pte,
> - pmd_t *pmd,
> - pmd_t orig_pmd,
> - struct vm_area_struct *vma,
> - struct list_head *compound_pagelist)
> + pmd_t *pmd, pmd_t orig_pmd, struct vm_area_struct *vma,
> + unsigned int order, struct list_head *compound_pagelist)
> {
> spinlock_t *pmd_ptl;
> -
> + const unsigned long nr_pages = 1UL << order;
> /*
> * Re-establish the PMD to point to the original page table
> * entry. Restoring PMD needs to be done prior to releasing
> @@ -763,7 +769,7 @@ static void __collapse_huge_page_copy_failed(pte_t *pte,
> * Release both raw and compound pages isolated
> * in __collapse_huge_page_isolate.
> */
> - release_pte_pages(pte, pte + HPAGE_PMD_NR, compound_pagelist);
> + release_pte_pages(pte, pte + nr_pages, compound_pagelist);
> }
>
> /*
> @@ -783,16 +789,16 @@ static void __collapse_huge_page_copy_failed(pte_t *pte,
> */
> static enum scan_result __collapse_huge_page_copy(pte_t *pte, struct folio *folio,
> pmd_t *pmd, pmd_t orig_pmd, struct vm_area_struct *vma,
> - unsigned long address, spinlock_t *ptl,
> + unsigned long address, spinlock_t *ptl, unsigned int order,
> struct list_head *compound_pagelist)
> {
> unsigned int i;
> enum scan_result result = SCAN_SUCCEED;
> -
> + const unsigned long nr_pages = 1UL << order;
Same here, all the way to the top.
> /*
> * Copying pages' contents is subject to memory poison at any iteration.
> */
> - for (i = 0; i < HPAGE_PMD_NR; i++) {
> + for (i = 0; i < nr_pages; i++) {
> pte_t pteval = ptep_get(pte + i);
> struct page *page = folio_page(folio, i);
> unsigned long src_addr = address + i * PAGE_SIZE;
> @@ -811,10 +817,10 @@ static enum scan_result __collapse_huge_page_copy(pte_t *pte, struct folio *foli
>
> if (likely(result == SCAN_SUCCEED))
> __collapse_huge_page_copy_succeeded(pte, vma, address, ptl,
> - compound_pagelist);
> + order, compound_pagelist);
> else
> __collapse_huge_page_copy_failed(pte, pmd, orig_pmd, vma,
> - compound_pagelist);
> + order, compound_pagelist);
>
> return result;
> }
> @@ -985,12 +991,12 @@ static enum scan_result check_pmd_still_valid(struct mm_struct *mm,
> * Returns result: if not SCAN_SUCCEED, mmap_lock has been released.
> */
> static enum scan_result __collapse_huge_page_swapin(struct mm_struct *mm,
> - struct vm_area_struct *vma, unsigned long start_addr, pmd_t *pmd,
> - int referenced)
> + struct vm_area_struct *vma, unsigned long start_addr,
> + pmd_t *pmd, int referenced, unsigned int order)
> {
> int swapped_in = 0;
> vm_fault_t ret = 0;
> - unsigned long addr, end = start_addr + (HPAGE_PMD_NR * PAGE_SIZE);
> + unsigned long addr, end = start_addr + (PAGE_SIZE << order);
> enum scan_result result;
> pte_t *pte = NULL;
> spinlock_t *ptl;
> @@ -1022,6 +1028,19 @@ static enum scan_result __collapse_huge_page_swapin(struct mm_struct *mm,
> pte_present(vmf.orig_pte))
> continue;
>
> + /*
> + * TODO: Support swapin without leading to further mTHP
> + * collapses. Currently bringing in new pages via swapin may
> + * cause a future higher order collapse on a rescan of the same
> + * range.
> + */
> + if (!is_pmd_order(order)) {
> + pte_unmap(pte);
> + mmap_read_unlock(mm);
> + result = SCAN_EXCEED_SWAP_PTE;
> + goto out;
> + }
> +
Interesting, we just swapin everything we find :)
But do we really need this check here? I mean, we just found it to be present.
In the rare event that there was a race, do we really care? It was just
present, now it's swapped. Bad luck. Just swap it in.
--
Cheers,
David
^ permalink raw reply
* Re: [PATCH mm-unstable v15 03/13] mm/khugepaged: generalize __collapse_huge_page_* for mTHP support
From: David Hildenbrand (Arm) @ 2026-03-12 20:36 UTC (permalink / raw)
To: Nico Pache, linux-doc, linux-kernel, linux-mm, linux-trace-kernel
Cc: aarcange, akpm, anshuman.khandual, apopple, baohua, baolin.wang,
byungchul, catalin.marinas, cl, corbet, dave.hansen, dev.jain,
gourry, hannes, hughd, jack, jackmanb, jannh, jglisse,
joshua.hahnjy, kas, lance.yang, Liam.Howlett, lorenzo.stoakes,
mathieu.desnoyers, matthew.brost, mhiramat, mhocko, peterx,
pfalcato, rakie.kim, raquini, rdunlap, richard.weiyang, rientjes,
rostedt, rppt, ryan.roberts, shivankg, sunnanyong, surenb,
thomas.hellstrom, tiwai, usamaarif642, vbabka, vishal.moola,
wangkefeng.wang, will, willy, yang, ying.huang, ziy, zokeefe
In-Reply-To: <8a4568de-e0f9-471b-bc94-1062d4af3938@kernel.org>
On 3/12/26 21:32, David Hildenbrand (Arm) wrote:
> On 2/26/26 04:23, Nico Pache wrote:
>> generalize the order of the __collapse_huge_page_* functions
>> to support future mTHP collapse.
>>
>> mTHP collapse will not honor the khugepaged_max_ptes_shared or
>> khugepaged_max_ptes_swap parameters, and will fail if it encounters a
>> shared or swapped entry.
>>
>> No functional changes in this patch.
>>
>> Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
>> Reviewed-by: Lance Yang <lance.yang@linux.dev>
>> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> Co-developed-by: Dev Jain <dev.jain@arm.com>
>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>> Signed-off-by: Nico Pache <npache@redhat.com>
>> ---
>> mm/khugepaged.c | 73 +++++++++++++++++++++++++++++++------------------
>> 1 file changed, 47 insertions(+), 26 deletions(-)
>>
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index a9b645402b7f..ecdbbf6a01a6 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -535,7 +535,7 @@ static void release_pte_pages(pte_t *pte, pte_t *_pte,
>>
>> static enum scan_result __collapse_huge_page_isolate(struct vm_area_struct *vma,
>> unsigned long start_addr, pte_t *pte, struct collapse_control *cc,
>> - struct list_head *compound_pagelist)
>> + unsigned int order, struct list_head *compound_pagelist)
>> {
>> struct page *page = NULL;
>> struct folio *folio = NULL;
>> @@ -543,15 +543,17 @@ static enum scan_result __collapse_huge_page_isolate(struct vm_area_struct *vma,
>> pte_t *_pte;
>> int none_or_zero = 0, shared = 0, referenced = 0;
>> enum scan_result result = SCAN_FAIL;
>> + const unsigned long nr_pages = 1UL << order;
>> + int max_ptes_none = khugepaged_max_ptes_none >> (HPAGE_PMD_ORDER - order);
>
> It might be a bit more readable to move "const unsigned long
> nr_pages = 1UL << order;" all the way to the top.
>
> Then, have here
>
> int max_ptes_none = 0;
>
> and do at the beginning of the function:
>
> /* For MADV_COLLAPSE, we always collapse ... */
> if (!cc->is_khugepaged)
> max_ptes_none = HPAGE_PMD_NR;
> /* ... except if userfaultf relies on MISSING faults. */
> if (!userfaultfd_armed(vma))
> max_ptes_none = khugepaged_max_ptes_none >> (HPAGE_PMD_ORDER - order);
>
> (but see below regarding helper function)
>
> then the code below becomes ...
>
>>
>> - for (_pte = pte; _pte < pte + HPAGE_PMD_NR;
>> + for (_pte = pte; _pte < pte + nr_pages;
>> _pte++, addr += PAGE_SIZE) {
>> pte_t pteval = ptep_get(_pte);
>> if (pte_none_or_zero(pteval)) {
>> ++none_or_zero;
>> if (!userfaultfd_armed(vma) &&
>> (!cc->is_khugepaged ||
>> - none_or_zero <= khugepaged_max_ptes_none)) {
>> + none_or_zero <= max_ptes_none)) {
>
> ...
>
> if (none_or_zero <= max_ptes_none) {
>
>
> I see that you do something like that (but slightly different) in the next
> patch. You could easily extend the above by it.
>
> Or go one step further and move all of that conditional into collapse_max_ptes_none(), whereby
> you simply also pass the cc and the vma.
>
> Then this all gets cleaned up and you'd end up above with
>
> max_ptes_none = collapse_max_ptes_none(cc, vma, order);
> if (max_ptes_none < 0)
> return result;
>
> I'd do all that in this patch here, getting rid of #4.
>
>
>> continue;
>> } else {
>> result = SCAN_EXCEED_NONE_PTE;
>> @@ -585,8 +587,14 @@ static enum scan_result __collapse_huge_page_isolate(struct vm_area_struct *vma,
>> /* See collapse_scan_pmd(). */
>> if (folio_maybe_mapped_shared(folio)) {
>> ++shared;
>> - if (cc->is_khugepaged &&
>> - shared > khugepaged_max_ptes_shared) {
>> + /*
>> + * TODO: Support shared pages without leading to further
>> + * mTHP collapses. Currently bringing in new pages via
>> + * shared may cause a future higher order collapse on a
>> + * rescan of the same range.
>> + */
>> + if (!is_pmd_order(order) || (cc->is_khugepaged &&
>> + shared > khugepaged_max_ptes_shared)) {
>
> That's not how we indent within a nested ().
>
> To make this easier to read, what about similarly having at the beginning
> of the function:
>
> int max_ptes_shared = 0;
>
> /* For MADV_COLLAPSE, we always collapse. */
> if (cc->is_khugepaged)
> max_ptes_none = HPAGE_PMD_NR;
> /* TODO ... */
> if (is_pmd_order(order))
> max_ptes_none = khugepaged_max_ptes_shared;
>
> to turn this code into a
>
> if (shared > khugepaged_max_ptes_shared)
>
> Also, here, might make sense to have a collapse_max_ptes_swap(cc, order)
> to do that and clean it up.
>
>
>> result = SCAN_EXCEED_SHARED_PTE;
>> count_vm_event(THP_SCAN_EXCEED_SHARED_PTE);
>> goto out;
>> @@ -679,18 +687,18 @@ static enum scan_result __collapse_huge_page_isolate(struct vm_area_struct *vma,
>> }
>>
>> static void __collapse_huge_page_copy_succeeded(pte_t *pte,
>> - struct vm_area_struct *vma,
>> - unsigned long address,
>> - spinlock_t *ptl,
>> - struct list_head *compound_pagelist)
>> + struct vm_area_struct *vma, unsigned long address,
>> + spinlock_t *ptl, unsigned int order,
>> + struct list_head *compound_pagelist)
>> {
>> - unsigned long end = address + HPAGE_PMD_SIZE;
>> + unsigned long end = address + (PAGE_SIZE << order);
>> struct folio *src, *tmp;
>> pte_t pteval;
>> pte_t *_pte;
>> unsigned int nr_ptes;
>> + const unsigned long nr_pages = 1UL << order;
>
> Move it further to the top.
>
>>
>> - for (_pte = pte; _pte < pte + HPAGE_PMD_NR; _pte += nr_ptes,
>> + for (_pte = pte; _pte < pte + nr_pages; _pte += nr_ptes,
>> address += nr_ptes * PAGE_SIZE) {
>> nr_ptes = 1;
>> pteval = ptep_get(_pte);
>> @@ -743,13 +751,11 @@ static void __collapse_huge_page_copy_succeeded(pte_t *pte,
>> }
>>
>> static void __collapse_huge_page_copy_failed(pte_t *pte,
>> - pmd_t *pmd,
>> - pmd_t orig_pmd,
>> - struct vm_area_struct *vma,
>> - struct list_head *compound_pagelist)
>> + pmd_t *pmd, pmd_t orig_pmd, struct vm_area_struct *vma,
>> + unsigned int order, struct list_head *compound_pagelist)
>> {
>> spinlock_t *pmd_ptl;
>> -
>> + const unsigned long nr_pages = 1UL << order;
>> /*
>> * Re-establish the PMD to point to the original page table
>> * entry. Restoring PMD needs to be done prior to releasing
>> @@ -763,7 +769,7 @@ static void __collapse_huge_page_copy_failed(pte_t *pte,
>> * Release both raw and compound pages isolated
>> * in __collapse_huge_page_isolate.
>> */
>> - release_pte_pages(pte, pte + HPAGE_PMD_NR, compound_pagelist);
>> + release_pte_pages(pte, pte + nr_pages, compound_pagelist);
>> }
>>
>> /*
>> @@ -783,16 +789,16 @@ static void __collapse_huge_page_copy_failed(pte_t *pte,
>> */
>> static enum scan_result __collapse_huge_page_copy(pte_t *pte, struct folio *folio,
>> pmd_t *pmd, pmd_t orig_pmd, struct vm_area_struct *vma,
>> - unsigned long address, spinlock_t *ptl,
>> + unsigned long address, spinlock_t *ptl, unsigned int order,
>> struct list_head *compound_pagelist)
>> {
>> unsigned int i;
>> enum scan_result result = SCAN_SUCCEED;
>> -
>> + const unsigned long nr_pages = 1UL << order;
>
> Same here, all the way to the top.
>
>> /*
>> * Copying pages' contents is subject to memory poison at any iteration.
>> */
>> - for (i = 0; i < HPAGE_PMD_NR; i++) {
>> + for (i = 0; i < nr_pages; i++) {
>> pte_t pteval = ptep_get(pte + i);
>> struct page *page = folio_page(folio, i);
>> unsigned long src_addr = address + i * PAGE_SIZE;
>> @@ -811,10 +817,10 @@ static enum scan_result __collapse_huge_page_copy(pte_t *pte, struct folio *foli
>>
>> if (likely(result == SCAN_SUCCEED))
>> __collapse_huge_page_copy_succeeded(pte, vma, address, ptl,
>> - compound_pagelist);
>> + order, compound_pagelist);
>> else
>> __collapse_huge_page_copy_failed(pte, pmd, orig_pmd, vma,
>> - compound_pagelist);
>> + order, compound_pagelist);
>>
>> return result;
>> }
>> @@ -985,12 +991,12 @@ static enum scan_result check_pmd_still_valid(struct mm_struct *mm,
>> * Returns result: if not SCAN_SUCCEED, mmap_lock has been released.
>> */
>> static enum scan_result __collapse_huge_page_swapin(struct mm_struct *mm,
>> - struct vm_area_struct *vma, unsigned long start_addr, pmd_t *pmd,
>> - int referenced)
>> + struct vm_area_struct *vma, unsigned long start_addr,
>> + pmd_t *pmd, int referenced, unsigned int order)
>> {
>> int swapped_in = 0;
>> vm_fault_t ret = 0;
>> - unsigned long addr, end = start_addr + (HPAGE_PMD_NR * PAGE_SIZE);
>> + unsigned long addr, end = start_addr + (PAGE_SIZE << order);
>> enum scan_result result;
>> pte_t *pte = NULL;
>> spinlock_t *ptl;
>> @@ -1022,6 +1028,19 @@ static enum scan_result __collapse_huge_page_swapin(struct mm_struct *mm,
>> pte_present(vmf.orig_pte))
>> continue;
>>
>> + /*
>> + * TODO: Support swapin without leading to further mTHP
>> + * collapses. Currently bringing in new pages via swapin may
>> + * cause a future higher order collapse on a rescan of the same
>> + * range.
>> + */
>> + if (!is_pmd_order(order)) {
>> + pte_unmap(pte);
>> + mmap_read_unlock(mm);
>> + result = SCAN_EXCEED_SWAP_PTE;
>> + goto out;
>> + }
>> +
>
> Interesting, we just swapin everything we find :)
>
> But do we really need this check here? I mean, we just found it to be present.
>
> In the rare event that there was a race, do we really care? It was just
> present, now it's swapped. Bad luck. Just swap it in.
>
Okay, now I am confused. Why are you not taking care of
collapse_scan_pmd() in the same context?
Because if you make sure that we properly check against a max_ptes_swap
similar as in the style above, we'd rule out swapin right from the start?
Also, I would expect that all other parameters in there are similarly
handled?
--
Cheers,
David
^ permalink raw reply
* Re: [PATCH v14 18/30] tracing: Check for undefined symbols in simple_ring_buffer
From: Nathan Chancellor @ 2026-03-12 20:40 UTC (permalink / raw)
To: Vincent Donnefort
Cc: rostedt, mhiramat, mathieu.desnoyers, linux-trace-kernel, maz,
oliver.upton, joey.gouly, suzuki.poulose, yuzenghui, kvmarm,
linux-arm-kernel, jstultz, qperret, will, aneesh.kumar,
kernel-team, linux-kernel
In-Reply-To: <abLIslqIcVscZRFg@google.com>
On Thu, Mar 12, 2026 at 02:07:46PM +0000, Vincent Donnefort wrote:
> In the end to unblock linux-next I have already sent an updated list of symbols.
> However feel free to send the logging bit, that is surely useful.
Thanks. I will review and test your "tracing: Generate undef symbols
allowlist for simple_ring_buffer" shortly (as I found more errors other
than the ones I described here) then I will base the logging patch on
top of that.
Cheers,
Nathan
^ permalink raw reply
* Re: [PATCH mm-unstable v15 03/13] mm/khugepaged: generalize __collapse_huge_page_* for mTHP support
From: David Hildenbrand (Arm) @ 2026-03-12 20:56 UTC (permalink / raw)
To: Nico Pache, linux-doc, linux-kernel, linux-mm, linux-trace-kernel
Cc: aarcange, akpm, anshuman.khandual, apopple, baohua, baolin.wang,
byungchul, catalin.marinas, cl, corbet, dave.hansen, dev.jain,
gourry, hannes, hughd, jack, jackmanb, jannh, jglisse,
joshua.hahnjy, kas, lance.yang, Liam.Howlett, lorenzo.stoakes,
mathieu.desnoyers, matthew.brost, mhiramat, mhocko, peterx,
pfalcato, rakie.kim, raquini, rdunlap, richard.weiyang, rientjes,
rostedt, rppt, ryan.roberts, shivankg, sunnanyong, surenb,
thomas.hellstrom, tiwai, usamaarif642, vbabka, vishal.moola,
wangkefeng.wang, will, willy, yang, ying.huang, ziy, zokeefe
In-Reply-To: <ee39e605-0d9f-433b-9dfa-f70fd92edfac@kernel.org>
On 3/12/26 21:36, David Hildenbrand (Arm) wrote:
> On 3/12/26 21:32, David Hildenbrand (Arm) wrote:
>> On 2/26/26 04:23, Nico Pache wrote:
>>> generalize the order of the __collapse_huge_page_* functions
>>> to support future mTHP collapse.
>>>
>>> mTHP collapse will not honor the khugepaged_max_ptes_shared or
>>> khugepaged_max_ptes_swap parameters, and will fail if it encounters a
>>> shared or swapped entry.
>>>
>>> No functional changes in this patch.
>>>
>>> Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
>>> Reviewed-by: Lance Yang <lance.yang@linux.dev>
>>> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>>> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>>> Co-developed-by: Dev Jain <dev.jain@arm.com>
>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>> Signed-off-by: Nico Pache <npache@redhat.com>
>>> ---
>>> mm/khugepaged.c | 73 +++++++++++++++++++++++++++++++------------------
>>> 1 file changed, 47 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>>> index a9b645402b7f..ecdbbf6a01a6 100644
>>> --- a/mm/khugepaged.c
>>> +++ b/mm/khugepaged.c
>>> @@ -535,7 +535,7 @@ static void release_pte_pages(pte_t *pte, pte_t *_pte,
>>>
>>> static enum scan_result __collapse_huge_page_isolate(struct vm_area_struct *vma,
>>> unsigned long start_addr, pte_t *pte, struct collapse_control *cc,
>>> - struct list_head *compound_pagelist)
>>> + unsigned int order, struct list_head *compound_pagelist)
>>> {
>>> struct page *page = NULL;
>>> struct folio *folio = NULL;
>>> @@ -543,15 +543,17 @@ static enum scan_result __collapse_huge_page_isolate(struct vm_area_struct *vma,
>>> pte_t *_pte;
>>> int none_or_zero = 0, shared = 0, referenced = 0;
>>> enum scan_result result = SCAN_FAIL;
>>> + const unsigned long nr_pages = 1UL << order;
>>> + int max_ptes_none = khugepaged_max_ptes_none >> (HPAGE_PMD_ORDER - order);
>>
>> It might be a bit more readable to move "const unsigned long
>> nr_pages = 1UL << order;" all the way to the top.
>>
>> Then, have here
>>
>> int max_ptes_none = 0;
>>
>> and do at the beginning of the function:
>>
>> /* For MADV_COLLAPSE, we always collapse ... */
>> if (!cc->is_khugepaged)
>> max_ptes_none = HPAGE_PMD_NR;
>> /* ... except if userfaultf relies on MISSING faults. */
>> if (!userfaultfd_armed(vma))
>> max_ptes_none = khugepaged_max_ptes_none >> (HPAGE_PMD_ORDER - order);
>>
>> (but see below regarding helper function)
>>
>> then the code below becomes ...
>>
>>>
>>> - for (_pte = pte; _pte < pte + HPAGE_PMD_NR;
>>> + for (_pte = pte; _pte < pte + nr_pages;
>>> _pte++, addr += PAGE_SIZE) {
>>> pte_t pteval = ptep_get(_pte);
>>> if (pte_none_or_zero(pteval)) {
>>> ++none_or_zero;
>>> if (!userfaultfd_armed(vma) &&
>>> (!cc->is_khugepaged ||
>>> - none_or_zero <= khugepaged_max_ptes_none)) {
>>> + none_or_zero <= max_ptes_none)) {
>>
>> ...
>>
>> if (none_or_zero <= max_ptes_none) {
>>
>>
>> I see that you do something like that (but slightly different) in the next
>> patch. You could easily extend the above by it.
>>
>> Or go one step further and move all of that conditional into collapse_max_ptes_none(), whereby
>> you simply also pass the cc and the vma.
>>
>> Then this all gets cleaned up and you'd end up above with
>>
>> max_ptes_none = collapse_max_ptes_none(cc, vma, order);
>> if (max_ptes_none < 0)
>> return result;
>>
>> I'd do all that in this patch here, getting rid of #4.
>>
>>
>>> continue;
>>> } else {
>>> result = SCAN_EXCEED_NONE_PTE;
>>> @@ -585,8 +587,14 @@ static enum scan_result __collapse_huge_page_isolate(struct vm_area_struct *vma,
>>> /* See collapse_scan_pmd(). */
>>> if (folio_maybe_mapped_shared(folio)) {
>>> ++shared;
>>> - if (cc->is_khugepaged &&
>>> - shared > khugepaged_max_ptes_shared) {
>>> + /*
>>> + * TODO: Support shared pages without leading to further
>>> + * mTHP collapses. Currently bringing in new pages via
>>> + * shared may cause a future higher order collapse on a
>>> + * rescan of the same range.
>>> + */
>>> + if (!is_pmd_order(order) || (cc->is_khugepaged &&
>>> + shared > khugepaged_max_ptes_shared)) {
>>
>> That's not how we indent within a nested ().
>>
>> To make this easier to read, what about similarly having at the beginning
>> of the function:
>>
>> int max_ptes_shared = 0;
>>
>> /* For MADV_COLLAPSE, we always collapse. */
>> if (cc->is_khugepaged)
>> max_ptes_none = HPAGE_PMD_NR;
>> /* TODO ... */
>> if (is_pmd_order(order))
>> max_ptes_none = khugepaged_max_ptes_shared;
>>
>> to turn this code into a
>>
>> if (shared > khugepaged_max_ptes_shared)
>>
>> Also, here, might make sense to have a collapse_max_ptes_swap(cc, order)
>> to do that and clean it up.
>>
>>
>>> result = SCAN_EXCEED_SHARED_PTE;
>>> count_vm_event(THP_SCAN_EXCEED_SHARED_PTE);
>>> goto out;
>>> @@ -679,18 +687,18 @@ static enum scan_result __collapse_huge_page_isolate(struct vm_area_struct *vma,
>>> }
>>>
>>> static void __collapse_huge_page_copy_succeeded(pte_t *pte,
>>> - struct vm_area_struct *vma,
>>> - unsigned long address,
>>> - spinlock_t *ptl,
>>> - struct list_head *compound_pagelist)
>>> + struct vm_area_struct *vma, unsigned long address,
>>> + spinlock_t *ptl, unsigned int order,
>>> + struct list_head *compound_pagelist)
>>> {
>>> - unsigned long end = address + HPAGE_PMD_SIZE;
>>> + unsigned long end = address + (PAGE_SIZE << order);
>>> struct folio *src, *tmp;
>>> pte_t pteval;
>>> pte_t *_pte;
>>> unsigned int nr_ptes;
>>> + const unsigned long nr_pages = 1UL << order;
>>
>> Move it further to the top.
>>
>>>
>>> - for (_pte = pte; _pte < pte + HPAGE_PMD_NR; _pte += nr_ptes,
>>> + for (_pte = pte; _pte < pte + nr_pages; _pte += nr_ptes,
>>> address += nr_ptes * PAGE_SIZE) {
>>> nr_ptes = 1;
>>> pteval = ptep_get(_pte);
>>> @@ -743,13 +751,11 @@ static void __collapse_huge_page_copy_succeeded(pte_t *pte,
>>> }
>>>
>>> static void __collapse_huge_page_copy_failed(pte_t *pte,
>>> - pmd_t *pmd,
>>> - pmd_t orig_pmd,
>>> - struct vm_area_struct *vma,
>>> - struct list_head *compound_pagelist)
>>> + pmd_t *pmd, pmd_t orig_pmd, struct vm_area_struct *vma,
>>> + unsigned int order, struct list_head *compound_pagelist)
>>> {
>>> spinlock_t *pmd_ptl;
>>> -
>>> + const unsigned long nr_pages = 1UL << order;
>>> /*
>>> * Re-establish the PMD to point to the original page table
>>> * entry. Restoring PMD needs to be done prior to releasing
>>> @@ -763,7 +769,7 @@ static void __collapse_huge_page_copy_failed(pte_t *pte,
>>> * Release both raw and compound pages isolated
>>> * in __collapse_huge_page_isolate.
>>> */
>>> - release_pte_pages(pte, pte + HPAGE_PMD_NR, compound_pagelist);
>>> + release_pte_pages(pte, pte + nr_pages, compound_pagelist);
>>> }
>>>
>>> /*
>>> @@ -783,16 +789,16 @@ static void __collapse_huge_page_copy_failed(pte_t *pte,
>>> */
>>> static enum scan_result __collapse_huge_page_copy(pte_t *pte, struct folio *folio,
>>> pmd_t *pmd, pmd_t orig_pmd, struct vm_area_struct *vma,
>>> - unsigned long address, spinlock_t *ptl,
>>> + unsigned long address, spinlock_t *ptl, unsigned int order,
>>> struct list_head *compound_pagelist)
>>> {
>>> unsigned int i;
>>> enum scan_result result = SCAN_SUCCEED;
>>> -
>>> + const unsigned long nr_pages = 1UL << order;
>>
>> Same here, all the way to the top.
>>
>>> /*
>>> * Copying pages' contents is subject to memory poison at any iteration.
>>> */
>>> - for (i = 0; i < HPAGE_PMD_NR; i++) {
>>> + for (i = 0; i < nr_pages; i++) {
>>> pte_t pteval = ptep_get(pte + i);
>>> struct page *page = folio_page(folio, i);
>>> unsigned long src_addr = address + i * PAGE_SIZE;
>>> @@ -811,10 +817,10 @@ static enum scan_result __collapse_huge_page_copy(pte_t *pte, struct folio *foli
>>>
>>> if (likely(result == SCAN_SUCCEED))
>>> __collapse_huge_page_copy_succeeded(pte, vma, address, ptl,
>>> - compound_pagelist);
>>> + order, compound_pagelist);
>>> else
>>> __collapse_huge_page_copy_failed(pte, pmd, orig_pmd, vma,
>>> - compound_pagelist);
>>> + order, compound_pagelist);
>>>
>>> return result;
>>> }
>>> @@ -985,12 +991,12 @@ static enum scan_result check_pmd_still_valid(struct mm_struct *mm,
>>> * Returns result: if not SCAN_SUCCEED, mmap_lock has been released.
>>> */
>>> static enum scan_result __collapse_huge_page_swapin(struct mm_struct *mm,
>>> - struct vm_area_struct *vma, unsigned long start_addr, pmd_t *pmd,
>>> - int referenced)
>>> + struct vm_area_struct *vma, unsigned long start_addr,
>>> + pmd_t *pmd, int referenced, unsigned int order)
>>> {
>>> int swapped_in = 0;
>>> vm_fault_t ret = 0;
>>> - unsigned long addr, end = start_addr + (HPAGE_PMD_NR * PAGE_SIZE);
>>> + unsigned long addr, end = start_addr + (PAGE_SIZE << order);
>>> enum scan_result result;
>>> pte_t *pte = NULL;
>>> spinlock_t *ptl;
>>> @@ -1022,6 +1028,19 @@ static enum scan_result __collapse_huge_page_swapin(struct mm_struct *mm,
>>> pte_present(vmf.orig_pte))
>>> continue;
>>>
>>> + /*
>>> + * TODO: Support swapin without leading to further mTHP
>>> + * collapses. Currently bringing in new pages via swapin may
>>> + * cause a future higher order collapse on a rescan of the same
>>> + * range.
>>> + */
>>> + if (!is_pmd_order(order)) {
>>> + pte_unmap(pte);
>>> + mmap_read_unlock(mm);
>>> + result = SCAN_EXCEED_SWAP_PTE;
>>> + goto out;
>>> + }
>>> +
>>
>> Interesting, we just swapin everything we find :)
>>
>> But do we really need this check here? I mean, we just found it to be present.
>>
>> In the rare event that there was a race, do we really care? It was just
>> present, now it's swapped. Bad luck. Just swap it in.
>>
>
> Okay, now I am confused. Why are you not taking care of
> collapse_scan_pmd() in the same context?
>
> Because if you make sure that we properly check against a max_ptes_swap
> similar as in the style above, we'd rule out swapin right from the start?
>
> Also, I would expect that all other parameters in there are similarly
> handled?
>
Okay, I think you should add the following:
From 17bce81ab93f3b16e044ac2f4f62be19aac38180 Mon Sep 17 00:00:00 2001
From: "David Hildenbrand (Arm)" <david@kernel.org>
Date: Thu, 12 Mar 2026 21:54:22 +0100
Subject: [PATCH] tmp
Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>
---
mm/khugepaged.c | 89 +++++++++++++++++++++++++++++--------------------
1 file changed, 53 insertions(+), 36 deletions(-)
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index b7b4680d27ab..6a3773bfa0a2 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -318,6 +318,34 @@ static ssize_t max_ptes_shared_store(struct kobject *kobj,
return count;
}
+static int collapse_max_ptes_none(struct collapse_control *cc,
+ struct vm_area_struct *vma)
+{
+ /* We don't mess with MISSING faults. */
+ if (vma && userfaultfd_armed(vma))
+ return 0;
+ /* MADV_COLLAPSE always collapses. */
+ if (!cc->is_khugepaged)
+ return HPAGE_PMD_NR;
+ return khugepaged_max_ptes_none;
+}
+
+static int collapse_max_ptes_shared(struct collapse_control *cc)
+{
+ /* MADV_COLLAPSE always collapses. */
+ if (!cc->is_khugepaged)
+ return HPAGE_PMD_NR;
+ return khugepaged_max_ptes_shared;
+}
+
+static int collapse_max_ptes_swap(struct collapse_control *cc)
+{
+ /* MADV_COLLAPSE always collapses. */
+ if (!cc->is_khugepaged)
+ return HPAGE_PMD_NR;
+ return khugepaged_max_ptes_swap;
+}
+
static struct kobj_attribute khugepaged_max_ptes_shared_attr =
__ATTR_RW(max_ptes_shared);
@@ -539,6 +567,8 @@ static enum scan_result __collapse_huge_page_isolate(struct vm_area_struct *vma,
unsigned long start_addr, pte_t *pte, struct collapse_control *cc,
struct list_head *compound_pagelist)
{
+ const int max_ptes_none = collapse_max_ptes_none(cc, vma);
+ const int max_ptes_shared = collapse_max_ptes_shared(cc);
struct page *page = NULL;
struct folio *folio = NULL;
unsigned long addr = start_addr;
@@ -550,16 +580,12 @@ static enum scan_result __collapse_huge_page_isolate(struct vm_area_struct *vma,
_pte++, addr += PAGE_SIZE) {
pte_t pteval = ptep_get(_pte);
if (pte_none_or_zero(pteval)) {
- ++none_or_zero;
- if (!userfaultfd_armed(vma) &&
- (!cc->is_khugepaged ||
- none_or_zero <= khugepaged_max_ptes_none)) {
- continue;
- } else {
+ if (++none_or_zero > max_ptes_none) {
result = SCAN_EXCEED_NONE_PTE;
count_vm_event(THP_SCAN_EXCEED_NONE_PTE);
goto out;
}
+ continue;
}
if (!pte_present(pteval)) {
result = SCAN_PTE_NON_PRESENT;
@@ -586,9 +612,7 @@ static enum scan_result __collapse_huge_page_isolate(struct vm_area_struct *vma,
/* See hpage_collapse_scan_pmd(). */
if (folio_maybe_mapped_shared(folio)) {
- ++shared;
- if (cc->is_khugepaged &&
- shared > khugepaged_max_ptes_shared) {
+ if (++shared > max_ptes_shared) {
result = SCAN_EXCEED_SHARED_PTE;
count_vm_event(THP_SCAN_EXCEED_SHARED_PTE);
goto out;
@@ -1247,6 +1271,9 @@ static enum scan_result hpage_collapse_scan_pmd(struct mm_struct *mm,
struct vm_area_struct *vma, unsigned long start_addr,
bool *mmap_locked, struct collapse_control *cc)
{
+ const int max_ptes_none = collapse_max_ptes_none(cc, vma);
+ const int max_ptes_swap = collapse_max_ptes_swap(cc);
+ const int max_ptes_shared = collapse_max_ptes_shared(cc);
pmd_t *pmd;
pte_t *pte, *_pte;
int none_or_zero = 0, shared = 0, referenced = 0;
@@ -1280,36 +1307,28 @@ static enum scan_result hpage_collapse_scan_pmd(struct mm_struct *mm,
pte_t pteval = ptep_get(_pte);
if (pte_none_or_zero(pteval)) {
- ++none_or_zero;
- if (!userfaultfd_armed(vma) &&
- (!cc->is_khugepaged ||
- none_or_zero <= khugepaged_max_ptes_none)) {
- continue;
- } else {
+ if (++none_or_zero > max_ptes_none) {
result = SCAN_EXCEED_NONE_PTE;
count_vm_event(THP_SCAN_EXCEED_NONE_PTE);
goto out_unmap;
}
+ continue;
}
if (!pte_present(pteval)) {
- ++unmapped;
- if (!cc->is_khugepaged ||
- unmapped <= khugepaged_max_ptes_swap) {
- /*
- * Always be strict with uffd-wp
- * enabled swap entries. Please see
- * comment below for pte_uffd_wp().
- */
- if (pte_swp_uffd_wp_any(pteval)) {
- result = SCAN_PTE_UFFD_WP;
- goto out_unmap;
- }
- continue;
- } else {
+ if (++unmapped > max_ptes_swap) {
result = SCAN_EXCEED_SWAP_PTE;
count_vm_event(THP_SCAN_EXCEED_SWAP_PTE);
goto out_unmap;
}
+ /*
+ * Always be strict with uffd-wp enabled swap entries.
+ * See the comment below for pte_uffd_wp().
+ */
+ if (pte_swp_uffd_wp_any(pteval)) {
+ result = SCAN_PTE_UFFD_WP;
+ goto out_unmap;
+ }
+ continue;
}
if (pte_uffd_wp(pteval)) {
/*
@@ -1348,9 +1367,7 @@ static enum scan_result hpage_collapse_scan_pmd(struct mm_struct *mm,
* is shared.
*/
if (folio_maybe_mapped_shared(folio)) {
- ++shared;
- if (cc->is_khugepaged &&
- shared > khugepaged_max_ptes_shared) {
+ if (++shared > max_ptes_shared) {
result = SCAN_EXCEED_SHARED_PTE;
count_vm_event(THP_SCAN_EXCEED_SHARED_PTE);
goto out_unmap;
@@ -2305,6 +2322,8 @@ static enum scan_result hpage_collapse_scan_file(struct mm_struct *mm,
unsigned long addr, struct file *file, pgoff_t start,
struct collapse_control *cc)
{
+ const int max_ptes_none = collapse_max_ptes_none(cc, NULL);
+ const int max_ptes_swap = collapse_max_ptes_swap(cc);
struct folio *folio = NULL;
struct address_space *mapping = file->f_mapping;
XA_STATE(xas, &mapping->i_pages, start);
@@ -2323,8 +2342,7 @@ static enum scan_result hpage_collapse_scan_file(struct mm_struct *mm,
if (xa_is_value(folio)) {
swap += 1 << xas_get_order(&xas);
- if (cc->is_khugepaged &&
- swap > khugepaged_max_ptes_swap) {
+ if (swap > max_ptes_swap) {
result = SCAN_EXCEED_SWAP_PTE;
count_vm_event(THP_SCAN_EXCEED_SWAP_PTE);
break;
@@ -2395,8 +2413,7 @@ static enum scan_result hpage_collapse_scan_file(struct mm_struct *mm,
cc->progress += HPAGE_PMD_NR;
if (result == SCAN_SUCCEED) {
- if (cc->is_khugepaged &&
- present < HPAGE_PMD_NR - khugepaged_max_ptes_none) {
+ if (present < HPAGE_PMD_NR - max_ptes_none) {
result = SCAN_EXCEED_NONE_PTE;
count_vm_event(THP_SCAN_EXCEED_NONE_PTE);
} else {
--
2.43.0
Then extend it by passing an order + return value check in this patch here. You can
directly squash changes from patch #4 in here then.
--
Cheers,
David
^ 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