[PATCH 1/4] arm64: ptdump: use seq_puts() in pt_dump_seq_puts() macro

[PATCH 1/4] arm64: 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).

All call sites pass constant strings without format specifiers, so
using seq_printf() adds unnecessary overhead for format string parsing.

This bug was introduced in commit ae5d1cf358a5 ("arm64: dump: Make the
page table dumping seq_file optional") in 2016, where seq_puts() was
replaced with a new pt_dump_seq_puts() macro that mistakenly used
seq_printf().

Fixes: ae5d1cf358a5 ("arm64: dump: Make the page table dumping seq_file optional")
Signed-off-by: Josephine Pfeiffer <hi@josie.lol>
---
 arch/arm64/mm/ptdump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/mm/ptdump.c b/arch/arm64/mm/ptdump.c
index ab9899ca1e5f..a35fcd62bf75 100644
--- a/arch/arm64/mm/ptdump.c
+++ b/arch/arm64/mm/ptdump.c
@@ -35,7 +35,7 @@
 #define pt_dump_seq_puts(m, fmt)	\
 ({					\
 	if (m)				\
-		seq_printf(m, fmt);	\
+		seq_puts(m, fmt);	\
 })
 
 static const struct ptdump_prot_bits pte_bits[] = {
-- 
2.51.1.dirty

Re: [PATCH 1/4] arm64: ptdump: use seq_puts() in pt_dump_seq_puts() macro

[Will Deacon]

On Sat, Oct 18, 2025 at 07:04:16PM +0200, 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).
> 
> All call sites pass constant strings without format specifiers, so
> using seq_printf() adds unnecessary overhead for format string parsing.
> 
> This bug was introduced in commit ae5d1cf358a5 ("arm64: dump: Make the
> page table dumping seq_file optional") in 2016, where seq_puts() was
> replaced with a new pt_dump_seq_puts() macro that mistakenly used
> seq_printf().
> 
> Fixes: ae5d1cf358a5 ("arm64: dump: Make the page table dumping seq_file optional")
> Signed-off-by: Josephine Pfeiffer <hi@josie.lol>
> ---
>  arch/arm64/mm/ptdump.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/mm/ptdump.c b/arch/arm64/mm/ptdump.c
> index ab9899ca1e5f..a35fcd62bf75 100644
> --- a/arch/arm64/mm/ptdump.c
> +++ b/arch/arm64/mm/ptdump.c
> @@ -35,7 +35,7 @@
>  #define pt_dump_seq_puts(m, fmt)	\
>  ({					\
>  	if (m)				\
> -		seq_printf(m, fmt);	\
> +		seq_puts(m, fmt);	\
>  })

This looks fine to me but I'm slightly confused as this patch is marked
as 1/4 but I can't find the other three anywhere. Is that just an error
or is this part of a bigger series?

Thanks,

Will

Re: [PATCH 1/4] arm64: ptdump: use seq_puts() in pt_dump_seq_puts() macro

[Josephine Pfeiffer]

Hi Will,

Thank you for reviewing! This is part of a 4-patch series that fixes the same issue across multiple architectures. The other three patches are:

[PATCH 2/4] ARM: ptdump: use seq_puts() in pt_dump_seq_puts() macro
https://lore.kernel.org/all/20251018170442.3355403-1-hi@josie.lol/

[PATCH 3/4] riscv: ptdump: use seq_puts() in pt_dump_seq_puts() macro
https://lore.kernel.org/all/20251018170451.3355496-1-hi@josie.lol/

[PATCH 4/4] s390: ptdump: use seq_puts() in pt_dump_seq_puts() macro
https://lore.kernel.org/all/20251018170521.3355738-1-hi@josie.lol/

Each patch was sent to the respective architecture maintainers.

Best,
Josie

Re: [PATCH 1/4] arm64: ptdump: use seq_puts() in pt_dump_seq_puts() macro

[Josephine Pfeiffer]

Hi Will,

Friendly ping on this patch. The s390 and RISC-V patches from this series
have been merged to master. Is there anything else needed for the arm64
version?

Thanks,
Josie

Re: [PATCH 1/4] arm64: ptdump: use seq_puts() in pt_dump_seq_puts() macro

[Anshuman Khandual]

On 18/10/25 10:34 PM, 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).
> 
> All call sites pass constant strings without format specifiers, so
> using seq_printf() adds unnecessary overhead for format string parsing.
> 
> This bug was introduced in commit ae5d1cf358a5 ("arm64: dump: Make the
> page table dumping seq_file optional") in 2016, where seq_puts() was
> replaced with a new pt_dump_seq_puts() macro that mistakenly used
> seq_printf().
> 
> Fixes: ae5d1cf358a5 ("arm64: dump: Make the page table dumping seq_file optional")
> Signed-off-by: Josephine Pfeiffer <hi@josie.lol>
> ---
>  arch/arm64/mm/ptdump.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/mm/ptdump.c b/arch/arm64/mm/ptdump.c
> index ab9899ca1e5f..a35fcd62bf75 100644
> --- a/arch/arm64/mm/ptdump.c
> +++ b/arch/arm64/mm/ptdump.c
> @@ -35,7 +35,7 @@
>  #define pt_dump_seq_puts(m, fmt)	\
>  ({					\
>  	if (m)				\
> -		seq_printf(m, fmt);	\
> +		seq_puts(m, fmt);	\
>  })
>  
>  static const struct ptdump_prot_bits pte_bits[] = {

LGTM and also did no see any problem while dumping via the
sysfs file /sys/kernel/debug/kernel_page_tables

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

Re: [PATCH 1/4] arm64: ptdump: use seq_puts() in pt_dump_seq_puts() macro

[Will Deacon]

On Sat, Oct 18, 2025 at 07:04:16PM +0200, 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).

What's conceptually wrong with using printf() to print an unformatted
string? There are loads of printk() calls that do that and I think it's
fine.

>  arch/arm64/mm/ptdump.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/mm/ptdump.c b/arch/arm64/mm/ptdump.c
> index ab9899ca1e5f..a35fcd62bf75 100644
> --- a/arch/arm64/mm/ptdump.c
> +++ b/arch/arm64/mm/ptdump.c
> @@ -35,7 +35,7 @@
>  #define pt_dump_seq_puts(m, fmt)	\
>  ({					\
>  	if (m)				\
> -		seq_printf(m, fmt);	\
> +		seq_puts(m, fmt);	\
>  })

Given that this macro has exactly one caller and it isn't a fast path,
wouldn't it be better to go the other way around and remove this helper
in favour of using pt_dump_seq_printf() everywhere?

i.e. something like the diff below

Will

--->8

diff --git a/arch/arm64/mm/ptdump.c b/arch/arm64/mm/ptdump.c
index ab9899ca1e5f..8a03b2c9f88b 100644
--- a/arch/arm64/mm/ptdump.c
+++ b/arch/arm64/mm/ptdump.c
@@ -32,12 +32,6 @@
                seq_printf(m, fmt, ##args);     \
 })
 
-#define pt_dump_seq_puts(m, fmt)       \
-({                                     \
-       if (m)                          \
-               seq_printf(m, fmt);     \
-})
-
 static const struct ptdump_prot_bits pte_bits[] = {
        {
                .mask   = PTE_VALID,
@@ -232,7 +226,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
                if (st->current_prot && pg_level[st->level].bits)
                        dump_prot(st, pg_level[st->level].bits,
                                  pg_level[st->level].num);
-               pt_dump_seq_puts(st->seq, "\n");
+               pt_dump_seq_printf(st->seq, "\n");
 
                if (addr >= st->marker[1].start_address) {
                        st->marker++;