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

[Paul Walmsley <pjw@kernel.org>]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv