[PATCH 3/4] riscv: ptdump: use seq_puts() in pt_dump_seq_puts() macro

[PATCH 3/4] riscv: ptdump: use seq_puts() in pt_dump_seq_puts() macro

[Josephine Pfeiffer]

The pt_dump_seq_puts() macro incorrectly uses seq_printf() instead of
seq_puts(). This is both a performance issue and conceptually wrong,
as the macro name suggests plain string output (puts) but the
implementation uses formatted output (printf).

The macro is used in ptdump.c:301 to output a newline character. Using
seq_printf() adds unnecessary overhead for format string parsing when
outputting this constant string.

This bug was introduced in commit 59c4da8640cc ("riscv: Add support to
dump the kernel page tables") in 2020, which copied the implementation
pattern from other architectures that had the same bug.

Fixes: 59c4da8640cc ("riscv: Add support to dump the kernel page tables")
Signed-off-by: Josephine Pfeiffer <hi@josie.lol>
---
 arch/riscv/mm/ptdump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/mm/ptdump.c b/arch/riscv/mm/ptdump.c
index 3b51690cc876..34299c2b231f 100644
--- a/arch/riscv/mm/ptdump.c
+++ b/arch/riscv/mm/ptdump.c
@@ -21,7 +21,7 @@
 #define pt_dump_seq_puts(m, fmt)	\
 ({					\
 	if (m)				\
-		seq_printf(m, fmt);	\
+		seq_puts(m, fmt);	\
 })
 
 /*
-- 
2.51.1.dirty

Re: [PATCH 3/4] riscv: ptdump: use seq_puts() in pt_dump_seq_puts() macro

[Paul Walmsley]

On Sat, 18 Oct 2025, Josephine Pfeiffer wrote:

> The pt_dump_seq_puts() macro incorrectly uses seq_printf() instead of
> seq_puts(). This is both a performance issue and conceptually wrong,
> as the macro name suggests plain string output (puts) but the
> implementation uses formatted output (printf).
> 
> The macro is used in ptdump.c:301 to output a newline character. Using
> seq_printf() adds unnecessary overhead for format string parsing when
> outputting this constant string.

Hard to accept that it's a performance issue.  But I think you're right 
that generating a newline should be done with seq_puts().

> This bug was introduced in commit 59c4da8640cc ("riscv: Add support to
> dump the kernel page tables") in 2020, which copied the implementation
> pattern from other architectures that had the same bug.
> 
> Fixes: 59c4da8640cc ("riscv: Add support to dump the kernel page tables")
> Signed-off-by: Josephine Pfeiffer <hi@josie.lol>

A better fix would seem to be to just get rid of pt_dump_seq_puts().  It's 
only used once in arch/riscv.

Taking a broader view, both pt_dump_seq_puts() and pt_dump_seq_printf() 
look completely pointless.  Is there any argument for keeping them?


- Paul

Re: [PATCH 3/4] riscv: ptdump: use seq_puts() in pt_dump_seq_puts() macro

[Josephine Pfeiffer]

On Sat, 18 Oct 2025, Paul Walmsley wrote:

> Hard to accept that it's a performance issue.  But I think you're right
> that generating a newline should be done with seq_puts().

Fair point. I'll drop that from the commit message.

> A better fix would seem to be to just get rid of pt_dump_seq_puts().  It's
> only used once in arch/riscv.
>
> Taking a broader view, both pt_dump_seq_puts() and pt_dump_seq_printf()
> look completely pointless.  Is there any argument for keeping them?

Good question. I investigated the git history and current usage:

The macros were introduced in commit ae5d1cf358a5 ("arm64: dump: Make the
page table dumping seq_file optional") to support passing NULL for the
seq_file parameter. This is used by ptdump_check_wx() for CONFIG_DEBUG_WX,
where the kernel walks page tables to check for writable+executable pages
without outputting anything to userspace.

All four architectures use this pattern in ptdump_check_wx():

  arch/arm64/mm/ptdump.c:341:         .seq = NULL,
  arch/arm/mm/dump.c:456:             .seq = NULL,
  arch/riscv/mm/ptdump.c:378:         .seq = NULL,
  arch/s390/mm/dump_pagetables.c:197: .seq = NULL,

However, you're right that the utility of these macros varies:

Usage of pt_dump_seq_puts():
- arm64: 1 use
- ARM: 0 uses
- riscv: 1 use
- s390: 3 uses

Note: ARM defines pt_dump_seq_puts() but never uses it - that macro
could be removed entirely.

Usage of pt_dump_seq_printf():
- arm64: 6 uses
- ARM: 7 uses
- riscv: 6 uses
- s390: 5 uses

For RISC-V specifically, I agree the single use of pt_dump_seq_puts()
could be replaced with an inline conditional. For pt_dump_seq_printf(),
the macro does save some repetition (6 uses vs 1 macro definition).

pt_dump_seq_printf() could also be questioned - removing it means 20+
inline conditionals across all architectures. I focused on the minimal
fix, but happy to tackle the larger refactor if preferred.

Would you prefer:

Option A) Remove pt_dump_seq_puts() entirely from riscv and replace the
single use with:
  if (st->seq)
    seq_puts(st->seq, "\n");

Option B) Keep the macro for consistency with other architectures, but
fix the bug

I'm happy to send a v2 with either approach. If Option A, I could also
propose similar cleanups for arm64 (1 use) as a follow-up.

Thanks for the review!

Re: [PATCH 3/4] riscv: ptdump: use seq_puts() in pt_dump_seq_puts() macro

[Paul Walmsley]

On Sun, 19 Oct 2025, Josephine Pfeiffer wrote:

> On Sat, 18 Oct 2025, Paul Walmsley wrote:
> 
> > Hard to accept that it's a performance issue.  But I think you're right
> > that generating a newline should be done with seq_puts().
> 
> Fair point. I'll drop that from the commit message.
> 
> > A better fix would seem to be to just get rid of pt_dump_seq_puts().  It's
> > only used once in arch/riscv.
> >
> > Taking a broader view, both pt_dump_seq_puts() and pt_dump_seq_printf()
> > look completely pointless.  Is there any argument for keeping them?
> 
> Good question. I investigated the git history and current usage:
> 
> The macros were introduced in commit ae5d1cf358a5 ("arm64: dump: Make the
> page table dumping seq_file optional") to support passing NULL for the
> seq_file parameter. This is used by ptdump_check_wx() for CONFIG_DEBUG_WX,
> where the kernel walks page tables to check for writable+executable pages
> without outputting anything to userspace.
> 
> All four architectures use this pattern in ptdump_check_wx():
> 
>   arch/arm64/mm/ptdump.c:341:         .seq = NULL,
>   arch/arm/mm/dump.c:456:             .seq = NULL,
>   arch/riscv/mm/ptdump.c:378:         .seq = NULL,
>   arch/s390/mm/dump_pagetables.c:197: .seq = NULL,
> 
> However, you're right that the utility of these macros varies:
> 
> Usage of pt_dump_seq_puts():
> - arm64: 1 use
> - ARM: 0 uses
> - riscv: 1 use
> - s390: 3 uses
> 
> Note: ARM defines pt_dump_seq_puts() but never uses it - that macro
> could be removed entirely.
> 
> Usage of pt_dump_seq_printf():
> - arm64: 6 uses
> - ARM: 7 uses
> - riscv: 6 uses
> - s390: 5 uses
> 
> For RISC-V specifically, I agree the single use of pt_dump_seq_puts()
> could be replaced with an inline conditional. For pt_dump_seq_printf(),
> the macro does save some repetition (6 uses vs 1 macro definition).
> 
> pt_dump_seq_printf() could also be questioned - removing it means 20+
> inline conditionals across all architectures. I focused on the minimal
> fix, but happy to tackle the larger refactor if preferred.
> 
> Would you prefer:
> 
> Option A) Remove pt_dump_seq_puts() entirely from riscv and replace the
> single use with:
>   if (st->seq)
>     seq_puts(st->seq, "\n");
> 
> Option B) Keep the macro for consistency with other architectures, but
> fix the bug
> 
> I'm happy to send a v2 with either approach. If Option A, I could also
> propose similar cleanups for arm64 (1 use) as a follow-up.

Thanks for investigating further.  I just queued your original patch; I 
think that's the simplest way forward.


- Paul