Re: [PATCH 01/13] kernel/irq/proc: use seq_put_decimal_ull_width() for decimal values

Re: [PATCH 01/13] kernel/irq/proc: use seq_put_decimal_ull_width() for decimal values

[Geert Uytterhoeven]

 	Hi David,

On Sat, 9 Nov 2024, David Wang wrote:
> seq_printf() is costy, on a system with m interrupts and n CPUs, there
> would be m*n decimal values yield via seq_printf() when reading
> /proc/interrupts, the cost parsing format strings grows with number of
> CPU. Profiling on a x86 8-core system indicates seq_printf() takes ~47%
> samples of show_interrupts(), and replace seq_printf() with
> seq_put_decimal_ull_width() could have near 30% performance gain.
>
> The improvement has pratical significance, considering many monitoring
> tools would read /proc/interrupts periodically.
>
> Signed-off-by: David Wang <00107082@163.com>

Thanks for your patch, which is now commit f9ed1f7c2e26fcd1
("genirq/proc: Use seq_put_decimal_ull_width() for decimal values")
in irqchip/irq/core.

This removes a space after the last CPU column, causing the values in
this column to be concatenated to the values in the next column.

E.g. on Koelsch (R-Car M-W), the output changes from:

 	       CPU0       CPU1
      27:       1871       2017 GIC-0  27 Level     arch_timer
      29:        646          0 GIC-0 205 Level     e60b0000.i2c
      30:          0          0 GIC-0 174 Level     ffca0000.timer
      31:          0          0 GIC-0  36 Level     e6050000.gpio
      32:          0          0 GIC-0  37 Level     e6051000.gpio
      [...]

to

 	       CPU0       CPU1
      27:       1966       1900GIC-0  27 Level     arch_timer
      29:        580          0GIC-0 205 Level     e60b0000.i2c
      30:          0          0GIC-0 174 Level     ffca0000.timer
      31:          0          0GIC-0  36 Level     e6050000.gpio
      32:          0          0GIC-0  37 Level     e6051000.gpio
      [...]

making the output hard to read, and probably breaking scripts that parse
its contents.

Reverting the commit fixes the issue for me.

> --- a/kernel/irq/proc.c
> +++ b/kernel/irq/proc.c
> @@ -494,9 +494,11 @@ int show_interrupts(struct seq_file *p, void *v)
> 	if (!desc->action || irq_desc_is_chained(desc) || !desc->kstat_irqs)
> 		goto outsparse;
>
> -	seq_printf(p, "%*d: ", prec, i);
> +	seq_printf(p, "%*d:", prec, i);
> 	for_each_online_cpu(j)
> -		seq_printf(p, "%10u ", desc->kstat_irqs ? per_cpu(desc->kstat_irqs->cnt, j) : 0);
> +		seq_put_decimal_ull_width(p, " ",
> +					  desc->kstat_irqs ? per_cpu(desc->kstat_irqs->cnt, j) : 0,
> +					  10);
>
> 	raw_spin_lock_irqsave(&desc->lock, flags);
> 	if (desc->irq_data.chip) {

Gr{oetje,eeting}s,

 						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 							    -- Linus Torvalds

Re: [PATCH 01/13] kernel/irq/proc: use seq_put_decimal_ull_width() for decimal values

[Thomas Gleixner]

On Tue, Nov 19 2024 at 20:55, Geert Uytterhoeven wrote:
> E.g. on Koelsch (R-Car M-W), the output changes from:
>
>  	       CPU0       CPU1
>       27:       1871       2017 GIC-0  27 Level     arch_timer
>       29:        646          0 GIC-0 205 Level     e60b0000.i2c
>       30:          0          0 GIC-0 174 Level     ffca0000.timer
>       31:          0          0 GIC-0  36 Level     e6050000.gpio
>       32:          0          0 GIC-0  37 Level     e6051000.gpio
>       [...]
>
> to
>
>  	       CPU0       CPU1
>       27:       1966       1900GIC-0  27 Level     arch_timer
>       29:        580          0GIC-0 205 Level     e60b0000.i2c
>       30:          0          0GIC-0 174 Level     ffca0000.timer
>       31:          0          0GIC-0  36 Level     e6050000.gpio
>       32:          0          0GIC-0  37 Level     e6051000.gpio
>       [...]
>
> making the output hard to read, and probably breaking scripts that parse
> its contents.
>
> Reverting the commit fixes the issue for me.

Interestingly enough the generic version and quite some of the chip
specific print functions have a leading space, but GIC does not.

The below should restore the original state.

Thanks,

        tglx
---
diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index f36c33bd2da4..9b715ce8cf2e 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -501,6 +501,7 @@ int show_interrupts(struct seq_file *p, void *v)
 
 		seq_put_decimal_ull_width(p, " ", cnt, 10);
 	}
+	seq_putc(p, ' ');
 
 	raw_spin_lock_irqsave(&desc->lock, flags);
 	if (desc->irq_data.chip) {

Re: [PATCH 01/13] kernel/irq/proc: use seq_put_decimal_ull_width() for decimal values

[David Wang]

At 2024-11-20 09:20:59, "Thomas Gleixner" <tglx@linutronix.de> wrote:
>On Tue, Nov 19 2024 at 20:55, Geert Uytterhoeven wrote:
>> E.g. on Koelsch (R-Car M-W), the output changes from:
>>
>>  	       CPU0       CPU1
>>       27:       1871       2017 GIC-0  27 Level     arch_timer
>>       29:        646          0 GIC-0 205 Level     e60b0000.i2c
>>       30:          0          0 GIC-0 174 Level     ffca0000.timer
>>       31:          0          0 GIC-0  36 Level     e6050000.gpio
>>       32:          0          0 GIC-0  37 Level     e6051000.gpio
>>       [...]
>>
>> to
>>
>>  	       CPU0       CPU1
>>       27:       1966       1900GIC-0  27 Level     arch_timer
>>       29:        580          0GIC-0 205 Level     e60b0000.i2c
>>       30:          0          0GIC-0 174 Level     ffca0000.timer
>>       31:          0          0GIC-0  36 Level     e6050000.gpio
>>       32:          0          0GIC-0  37 Level     e6051000.gpio
>>       [...]
>>
>> making the output hard to read, and probably breaking scripts that parse
>> its contents.
>>
>> Reverting the commit fixes the issue for me.
>
>Interestingly enough the generic version and quite some of the chip
>specific print functions have a leading space, but GIC does not.
>
>The below should restore the original state.
>
>Thanks,
>
>        tglx
>---
>diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
>index f36c33bd2da4..9b715ce8cf2e 100644
>--- a/kernel/irq/proc.c
>+++ b/kernel/irq/proc.c
>@@ -501,6 +501,7 @@ int show_interrupts(struct seq_file *p, void *v)
> 
> 		seq_put_decimal_ull_width(p, " ", cnt, 10);
> 	}
>+	seq_putc(p, ' ');
> 
> 	raw_spin_lock_irqsave(&desc->lock, flags);
> 	if (desc->irq_data.chip) {

LGTM.

Acked-by:  David Wang <00107082@163.com>


Thanks
David

Re: [PATCH 01/13] kernel/irq/proc: use seq_put_decimal_ull_width() for decimal values

[David Wang]

At 2024-11-20 09:20:59, "Thomas Gleixner" <tglx@linutronix.de> wrote:
>On Tue, Nov 19 2024 at 20:55, Geert Uytterhoeven wrote:
>> E.g. on Koelsch (R-Car M-W), the output changes from:
>>
>>  	       CPU0       CPU1
>>       27:       1871       2017 GIC-0  27 Level     arch_timer
>>       29:        646          0 GIC-0 205 Level     e60b0000.i2c
>>       30:          0          0 GIC-0 174 Level     ffca0000.timer
>>       31:          0          0 GIC-0  36 Level     e6050000.gpio
>>       32:          0          0 GIC-0  37 Level     e6051000.gpio
>>       [...]
>>
>> to
>>
>>  	       CPU0       CPU1
>>       27:       1966       1900GIC-0  27 Level     arch_timer
>>       29:        580          0GIC-0 205 Level     e60b0000.i2c
>>       30:          0          0GIC-0 174 Level     ffca0000.timer
>>       31:          0          0GIC-0  36 Level     e6050000.gpio
>>       32:          0          0GIC-0  37 Level     e6051000.gpio
>>       [...]
>>
>> making the output hard to read, and probably breaking scripts that parse
>> its contents.
>>
>> Reverting the commit fixes the issue for me.
>
>Interestingly enough the generic version and quite some of the chip
>specific print functions have a leading space, but GIC does not.
>
>The below should restore the original state.
>
>Thanks,
>
>        tglx
>---
>diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
>index f36c33bd2da4..9b715ce8cf2e 100644
>--- a/kernel/irq/proc.c
>+++ b/kernel/irq/proc.c
>@@ -501,6 +501,7 @@ int show_interrupts(struct seq_file *p, void *v)
> 
> 		seq_put_decimal_ull_width(p, " ", cnt, 10);
> 	}
>+	seq_putc(p, ' ');
> 
> 	raw_spin_lock_irqsave(&desc->lock, flags);
> 	if (desc->irq_data.chip) {

On second thought,   considering other paths have already had a leading space, 
maybe it is more clean to just add a leading space before irq_print_chip:

        raw_spin_lock_irqsave(&desc->lock, flags);
        if (desc->irq_data.chip) {
-               if (desc->irq_data.chip->irq_print_chip)
+               if (desc->irq_data.chip->irq_print_chip) {
+                       seq_putc(p, ' ');
                        desc->irq_data.chip->irq_print_chip(&desc->irq_data, p);
-               else if (desc->irq_data.chip->name)
+               } else if (desc->irq_data.chip->name)
                        seq_printf(p, " %8s", desc->irq_data.chip->name);
                else
                        seq_printf(p, " %8s", "-");



David

Re: [PATCH 01/13] kernel/irq/proc: use seq_put_decimal_ull_width() for decimal values

[Thomas Gleixner]

On Wed, Nov 20 2024 at 12:24, David Wang wrote:
> At 2024-11-20 09:20:59, "Thomas Gleixner" <tglx@linutronix.de> wrote:
>>diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
>>index f36c33bd2da4..9b715ce8cf2e 100644
>>--- a/kernel/irq/proc.c
>>+++ b/kernel/irq/proc.c
>>@@ -501,6 +501,7 @@ int show_interrupts(struct seq_file *p, void *v)
>> 
>> 		seq_put_decimal_ull_width(p, " ", cnt, 10);
>> 	}
>>+	seq_putc(p, ' ');
>> 
>> 	raw_spin_lock_irqsave(&desc->lock, flags);
>> 	if (desc->irq_data.chip) {
>
> On second thought,   considering other paths have already had a leading space, 
> maybe it is more clean to just add a leading space before irq_print_chip:
>
>         raw_spin_lock_irqsave(&desc->lock, flags);
>         if (desc->irq_data.chip) {
> -               if (desc->irq_data.chip->irq_print_chip)
> +               if (desc->irq_data.chip->irq_print_chip) {
> +                       seq_putc(p, ' ');
>                         desc->irq_data.chip->irq_print_chip(&desc->irq_data, p);
> -               else if (desc->irq_data.chip->name)
> +               } else if (desc->irq_data.chip->name)
>                         seq_printf(p, " %8s", desc->irq_data.chip->name);
>                 else
>                         seq_printf(p, " %8s", "-");

I rather keep the seq_putc() and remove the trailing space from the
other prints.

Re: [PATCH 01/13] kernel/irq/proc: use seq_put_decimal_ull_width() for decimal values

[David Wang]

At 2024-11-27 01:40:29, "Thomas Gleixner" <tglx@linutronix.de> wrote:
>On Wed, Nov 20 2024 at 12:24, David Wang wrote:
>> At 2024-11-20 09:20:59, "Thomas Gleixner" <tglx@linutronix.de> wrote:
>>>diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
>>>index f36c33bd2da4..9b715ce8cf2e 100644
>>>--- a/kernel/irq/proc.c
>>>+++ b/kernel/irq/proc.c
>>>@@ -501,6 +501,7 @@ int show_interrupts(struct seq_file *p, void *v)
>>> 
>>> 		seq_put_decimal_ull_width(p, " ", cnt, 10);
>>> 	}
>>>+	seq_putc(p, ' ');
>>> 
>>> 	raw_spin_lock_irqsave(&desc->lock, flags);
>>> 	if (desc->irq_data.chip) {
>>
>> On second thought,   considering other paths have already had a leading space, 
>> maybe it is more clean to just add a leading space before irq_print_chip:
>>
>>         raw_spin_lock_irqsave(&desc->lock, flags);
>>         if (desc->irq_data.chip) {
>> -               if (desc->irq_data.chip->irq_print_chip)
>> +               if (desc->irq_data.chip->irq_print_chip) {
>> +                       seq_putc(p, ' ');
>>                         desc->irq_data.chip->irq_print_chip(&desc->irq_data, p);
>> -               else if (desc->irq_data.chip->name)
>> +               } else if (desc->irq_data.chip->name)
>>                         seq_printf(p, " %8s", desc->irq_data.chip->name);
>>                 else
>>                         seq_printf(p, " %8s", "-");
>
>I rather keep the seq_putc() and remove the trailing space from the
>other prints.

Agree,  code would align better this way and separating spaces will be consistent.

David

Re: [PATCH 01/13] kernel/irq/proc: use seq_put_decimal_ull_width() for decimal values

[Geert Uytterhoeven]

Hi Thomas,

On Wed, Nov 20, 2024 at 2:21 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> On Tue, Nov 19 2024 at 20:55, Geert Uytterhoeven wrote:
> > E.g. on Koelsch (R-Car M-W), the output changes from:
> >
> >              CPU0       CPU1
> >       27:       1871       2017 GIC-0  27 Level     arch_timer
> >       29:        646          0 GIC-0 205 Level     e60b0000.i2c
> >       30:          0          0 GIC-0 174 Level     ffca0000.timer
> >       31:          0          0 GIC-0  36 Level     e6050000.gpio
> >       32:          0          0 GIC-0  37 Level     e6051000.gpio
> >       [...]
> >
> > to
> >
> >              CPU0       CPU1
> >       27:       1966       1900GIC-0  27 Level     arch_timer
> >       29:        580          0GIC-0 205 Level     e60b0000.i2c
> >       30:          0          0GIC-0 174 Level     ffca0000.timer
> >       31:          0          0GIC-0  36 Level     e6050000.gpio
> >       32:          0          0GIC-0  37 Level     e6051000.gpio
> >       [...]
> >
> > making the output hard to read, and probably breaking scripts that parse
> > its contents.
> >
> > Reverting the commit fixes the issue for me.
>
> Interestingly enough the generic version and quite some of the chip
> specific print functions have a leading space, but GIC does not.
>
> The below should restore the original state.

> --- a/kernel/irq/proc.c
> +++ b/kernel/irq/proc.c
> @@ -501,6 +501,7 @@ int show_interrupts(struct seq_file *p, void *v)
>
>                 seq_put_decimal_ull_width(p, " ", cnt, 10);
>         }
> +       seq_putc(p, ' ');
>
>         raw_spin_lock_irqsave(&desc->lock, flags);
>         if (desc->irq_data.chip) {

Thanks, that does the trick!
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH] genirq/proc: Add missing space separator back

[Thomas Gleixner]

The recent conversion of show_interrupts() to seq_put_decimal_ull_width()
caused a formatting regression as it drops a previosuly existing space
separator.

Add it back by unconditionally inserting a space after the interrupt
counts and removing the extra leading space from the chip name prints.

Fixes: f9ed1f7c2e26 ("genirq/proc: Use seq_put_decimal_ull_width() for decimal values")
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Closes: https://lore.kernel.org/all/4ce18851-6e9f-bbe-8319-cc5e69fb45c@linux-m68k.org
---
 kernel/irq/proc.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -501,17 +501,18 @@ int show_interrupts(struct seq_file *p,
 
 		seq_put_decimal_ull_width(p, " ", cnt, 10);
 	}
+	seq_putc(p, ' ');
 
 	raw_spin_lock_irqsave(&desc->lock, flags);
 	if (desc->irq_data.chip) {
 		if (desc->irq_data.chip->irq_print_chip)
 			desc->irq_data.chip->irq_print_chip(&desc->irq_data, p);
 		else if (desc->irq_data.chip->name)
-			seq_printf(p, " %8s", desc->irq_data.chip->name);
+			seq_printf(p, "%8s", desc->irq_data.chip->name);
 		else
-			seq_printf(p, " %8s", "-");
+			seq_printf(p, "%8s", "-");
 	} else {
-		seq_printf(p, " %8s", "None");
+		seq_printf(p, "%8s", "None");
 	}
 	if (desc->irq_data.domain)
 		seq_printf(p, " %*lu", prec, desc->irq_data.hwirq);

Re:[PATCH] genirq/proc: Add missing space separator back

[David Wang]

At 2024-12-03 18:40:43, "Thomas Gleixner" <tglx@linutronix.de> wrote:
>The recent conversion of show_interrupts() to seq_put_decimal_ull_width()
>caused a formatting regression as it drops a previosuly existing space
>separator.
>
>Add it back by unconditionally inserting a space after the interrupt
>counts and removing the extra leading space from the chip name prints.
>
>Fixes: f9ed1f7c2e26 ("genirq/proc: Use seq_put_decimal_ull_width() for decimal values")
>Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
>Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>
>Closes: https://lore.kernel.org/all/4ce18851-6e9f-bbe-8319-cc5e69fb45c@linux-m68k.org
>---
> kernel/irq/proc.c |    7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
>--- a/kernel/irq/proc.c
>+++ b/kernel/irq/proc.c
>@@ -501,17 +501,18 @@ int show_interrupts(struct seq_file *p,
> 
> 		seq_put_decimal_ull_width(p, " ", cnt, 10);
> 	}
>+	seq_putc(p, ' ');
> 
> 	raw_spin_lock_irqsave(&desc->lock, flags);
> 	if (desc->irq_data.chip) {
> 		if (desc->irq_data.chip->irq_print_chip)
> 			desc->irq_data.chip->irq_print_chip(&desc->irq_data, p);
> 		else if (desc->irq_data.chip->name)
>-			seq_printf(p, " %8s", desc->irq_data.chip->name);
>+			seq_printf(p, "%8s", desc->irq_data.chip->name);
> 		else
>-			seq_printf(p, " %8s", "-");
>+			seq_printf(p, "%8s", "-");
> 	} else {
>-		seq_printf(p, " %8s", "None");
>+		seq_printf(p, "%8s", "None");
> 	}
> 	if (desc->irq_data.domain)
> 		seq_printf(p, " %*lu", prec, desc->irq_data.hwirq);


Reviewed-by:    David Wang <00107082@163.com>

And again, sorry for the regression...

Thanks
David

Re: [PATCH] genirq/proc: Add missing space separator back

[Geert Uytterhoeven]

Hi Thomas,

On Tue, Dec 3, 2024 at 11:40 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> The recent conversion of show_interrupts() to seq_put_decimal_ull_width()
> caused a formatting regression as it drops a previosuly existing space
> separator.
>
> Add it back by unconditionally inserting a space after the interrupt
> counts and removing the extra leading space from the chip name prints.
>
> Fixes: f9ed1f7c2e26 ("genirq/proc: Use seq_put_decimal_ull_width() for decimal values")
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Closes: https://lore.kernel.org/all/4ce18851-6e9f-bbe-8319-cc5e69fb45c@linux-m68k.org

Thanks for your patch!

> --- a/kernel/irq/proc.c
> +++ b/kernel/irq/proc.c
> @@ -501,17 +501,18 @@ int show_interrupts(struct seq_file *p,
>
>                 seq_put_decimal_ull_width(p, " ", cnt, 10);
>         }
> +       seq_putc(p, ' ');
>
>         raw_spin_lock_irqsave(&desc->lock, flags);
>         if (desc->irq_data.chip) {
>                 if (desc->irq_data.chip->irq_print_chip)
>                         desc->irq_data.chip->irq_print_chip(&desc->irq_data, p);

You should also remove the leading space from the few .irq_print_chip()
callbacks that print such a space. According to

    git grep -lw irq_print_chip | xargs git grep -W "seq_.*\" "

and filterng out the false positives:

    arch/powerpc/sysdev/fsl_msi.c:  seq_printf(p, " fsl-msi-%d", cascade_virq);
    drivers/bus/moxtet.c:   seq_printf(p, " moxtet-%s.%i#%i",
mox_module_name(id), pos->idx,
    drivers/irqchip/irq-partition-percpu.c: seq_printf(p, " %5s-%lu",
chip->name, data->hwirq);
    drivers/soc/qcom/smp2p.c:       seq_printf(p, " %8s",
dev_name(entry->smp2p->dev));

>                 else if (desc->irq_data.chip->name)
> -                       seq_printf(p, " %8s", desc->irq_data.chip->name);
> +                       seq_printf(p, "%8s", desc->irq_data.chip->name);
>                 else
> -                       seq_printf(p, " %8s", "-");
> +                       seq_printf(p, "%8s", "-");
>         } else {
> -               seq_printf(p, " %8s", "None");
> +               seq_printf(p, "%8s", "None");
>         }
>         if (desc->irq_data.domain)
>                 seq_printf(p, " %*lu", prec, desc->irq_data.hwirq);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] genirq/proc: Add missing space separator back

[Thomas Gleixner]

On Tue, Dec 03 2024 at 12:21, Geert Uytterhoeven wrote:
>
> You should also remove the leading space from the few .irq_print_chip()
> callbacks that print such a space. According to
>
>     git grep -lw irq_print_chip | xargs git grep -W "seq_.*\" "
>
> and filterng out the false positives:
>
>     arch/powerpc/sysdev/fsl_msi.c:  seq_printf(p, " fsl-msi-%d", cascade_virq);
>     drivers/bus/moxtet.c:   seq_printf(p, " moxtet-%s.%i#%i",
> mox_module_name(id), pos->idx,
>     drivers/irqchip/irq-partition-percpu.c: seq_printf(p, " %5s-%lu",
> chip->name, data->hwirq);
>     drivers/soc/qcom/smp2p.c:       seq_printf(p, " %8s",
> dev_name(entry->smp2p->dev));

Care to send a patch?

Thanks,

        tglx

Re: [PATCH 01/13] kernel/irq/proc: use seq_put_decimal_ull_width() for decimal values

[David Wang]

Hi, 
At 2024-11-20 03:55:30, "Geert Uytterhoeven" <geert@linux-m68k.org> wrote:
> 	Hi David,
>
>On Sat, 9 Nov 2024, David Wang wrote:
>> seq_printf() is costy, on a system with m interrupts and n CPUs, there
>> would be m*n decimal values yield via seq_printf() when reading
>> /proc/interrupts, the cost parsing format strings grows with number of
>> CPU. Profiling on a x86 8-core system indicates seq_printf() takes ~47%
>> samples of show_interrupts(), and replace seq_printf() with
>> seq_put_decimal_ull_width() could have near 30% performance gain.
>>
>> The improvement has pratical significance, considering many monitoring
>> tools would read /proc/interrupts periodically.
>>
>> Signed-off-by: David Wang <00107082@163.com>
>
>Thanks for your patch, which is now commit f9ed1f7c2e26fcd1
>("genirq/proc: Use seq_put_decimal_ull_width() for decimal values")
>in irqchip/irq/core.
>
>This removes a space after the last CPU column, causing the values in
>this column to be concatenated to the values in the next column.
>
>E.g. on Koelsch (R-Car M-W), the output changes from:
>
> 	       CPU0       CPU1
>      27:       1871       2017 GIC-0  27 Level     arch_timer
>      29:        646          0 GIC-0 205 Level     e60b0000.i2c
>      30:          0          0 GIC-0 174 Level     ffca0000.timer
>      31:          0          0 GIC-0  36 Level     e6050000.gpio
>      32:          0          0 GIC-0  37 Level     e6051000.gpio
>      [...]
>
>to
>
> 	       CPU0       CPU1
>      27:       1966       1900GIC-0  27 Level     arch_timer
>      29:        580          0GIC-0 205 Level     e60b0000.i2c
>      30:          0          0GIC-0 174 Level     ffca0000.timer
>      31:          0          0GIC-0  36 Level     e6050000.gpio
>      32:          0          0GIC-0  37 Level     e6051000.gpio
>      [...]
>
>making the output hard to read, and probably breaking scripts that parse
>its contents.

Thanks for reporting this, I was considering the spaces and checked it on my system,
I thought "all" descriptions have leading spaces and it's ok to remove the extra one.
But I did not check all the "irq_print_chip" codes, now when
checking the code, there are many GPIO drivers' implementations with no leading spaces.
(The behavior is not consistent cross  driver implementations though...)

Sorry for the regression, and thanks for catching this.


David

Re: [PATCH 01/13] kernel/irq/proc: use seq_put_decimal_ull_width() for decimal values

[David Wang]

At 2024-11-20 09:37:04, "David Wang" <00107082@163.com> wrote:
>Hi, 
>At 2024-11-20 03:55:30, "Geert Uytterhoeven" <geert@linux-m68k.org> wrote:
>> 	Hi David,
>>
>>On Sat, 9 Nov 2024, David Wang wrote:
>>> seq_printf() is costy, on a system with m interrupts and n CPUs, there
>>> would be m*n decimal values yield via seq_printf() when reading
>>> /proc/interrupts, the cost parsing format strings grows with number of
>>> CPU. Profiling on a x86 8-core system indicates seq_printf() takes ~47%
>>> samples of show_interrupts(), and replace seq_printf() with
>>> seq_put_decimal_ull_width() could have near 30% performance gain.
>>>
>>> The improvement has pratical significance, considering many monitoring
>>> tools would read /proc/interrupts periodically.
>>>
>>> Signed-off-by: David Wang <00107082@163.com>
>>
>>Thanks for your patch, which is now commit f9ed1f7c2e26fcd1
>>("genirq/proc: Use seq_put_decimal_ull_width() for decimal values")
>>in irqchip/irq/core.
>>
>>This removes a space after the last CPU column, causing the values in
>>this column to be concatenated to the values in the next column.
>>
>>E.g. on Koelsch (R-Car M-W), the output changes from:
>>
>> 	       CPU0       CPU1
>>      27:       1871       2017 GIC-0  27 Level     arch_timer
>>      29:        646          0 GIC-0 205 Level     e60b0000.i2c
>>      30:          0          0 GIC-0 174 Level     ffca0000.timer
>>      31:          0          0 GIC-0  36 Level     e6050000.gpio
>>      32:          0          0 GIC-0  37 Level     e6051000.gpio
>>      [...]
>>
>>to
>>
>> 	       CPU0       CPU1
>>      27:       1966       1900GIC-0  27 Level     arch_timer
>>      29:        580          0GIC-0 205 Level     e60b0000.i2c
>>      30:          0          0GIC-0 174 Level     ffca0000.timer
>>      31:          0          0GIC-0  36 Level     e6050000.gpio
>>      32:          0          0GIC-0  37 Level     e6051000.gpio
>>      [...]
>>
>>making the output hard to read, and probably breaking scripts that parse
>>its contents.
>
>Thanks for reporting this, I was considering the spaces and checked it on my system,
>I thought "all" descriptions have leading spaces and it's ok to remove the extra one.
>But I did not check all the "irq_print_chip" codes, now when
>checking the code, there are many GPIO drivers' implementations with no leading spaces.
>(The behavior is not consistent cross  driver implementations though...)

Several drivers use dev_name as format string for seq_printf,  would this raise security concerns?

       drivers/gpio/gpio-xgs-iproc.c:	seq_printf(p, dev_name(chip->dev));
        drivers/gpio/gpio-mlxbf2.c:	seq_printf(p, dev_name(gs->dev));
        drivers/gpio/gpio-omap.c:	seq_printf(p, dev_name(bank->dev));
        drivers/gpio/gpio-hlwd.c:	seq_printf(p, dev_name(hlwd->dev));
        drivers/gpio/gpio-aspeed.c:	seq_printf(p, dev_name(gpio->dev));
        drivers/gpio/gpio-pca953x.c:	seq_printf(p, dev_name(gc->parent));
        drivers/gpio/gpio-tegra186.c:	seq_printf(p, dev_name(gc->parent));
        drivers/gpio/gpio-tegra.c:	seq_printf(s, dev_name(chip->parent));
        drivers/gpio/gpio-ep93xx.c:	seq_printf(p, dev_name(gc->parent));
        drivers/gpio/gpio-aspeed-sgpio.c:	seq_printf(p, dev_name(gpio->dev));
        drivers/gpio/gpio-pl061.c:	seq_printf(p, dev_name(gc->parent));
        drivers/gpio/gpio-visconti.c:	seq_printf(p, dev_name(priv->dev));


>
>Sorry for the regression, and thanks for catching this.
>
>
>David

David

Re: [PATCH 01/13] kernel/irq/proc: use seq_put_decimal_ull_width() for decimal values

[Geert Uytterhoeven]

Hi David,

On Wed, Nov 20, 2024 at 3:08 AM David Wang <00107082@163.com> wrote:
> At 2024-11-20 09:37:04, "David Wang" <00107082@163.com> wrote:
> >At 2024-11-20 03:55:30, "Geert Uytterhoeven" <geert@linux-m68k.org> wrote:
> >>On Sat, 9 Nov 2024, David Wang wrote:
> >>> seq_printf() is costy, on a system with m interrupts and n CPUs, there
> >>> would be m*n decimal values yield via seq_printf() when reading
> >>> /proc/interrupts, the cost parsing format strings grows with number of
> >>> CPU. Profiling on a x86 8-core system indicates seq_printf() takes ~47%
> >>> samples of show_interrupts(), and replace seq_printf() with
> >>> seq_put_decimal_ull_width() could have near 30% performance gain.
> >>>
> >>> The improvement has pratical significance, considering many monitoring
> >>> tools would read /proc/interrupts periodically.
> >>>
> >>> Signed-off-by: David Wang <00107082@163.com>
> >>
> >>Thanks for your patch, which is now commit f9ed1f7c2e26fcd1
> >>("genirq/proc: Use seq_put_decimal_ull_width() for decimal values")
> >>in irqchip/irq/core.
> >>
> >>This removes a space after the last CPU column, causing the values in
> >>this column to be concatenated to the values in the next column.
> >>
> >>E.g. on Koelsch (R-Car M-W), the output changes from:
> >>
> >>             CPU0       CPU1
> >>      27:       1871       2017 GIC-0  27 Level     arch_timer
> >>      29:        646          0 GIC-0 205 Level     e60b0000.i2c
> >>      30:          0          0 GIC-0 174 Level     ffca0000.timer
> >>      31:          0          0 GIC-0  36 Level     e6050000.gpio
> >>      32:          0          0 GIC-0  37 Level     e6051000.gpio
> >>      [...]
> >>
> >>to
> >>
> >>             CPU0       CPU1
> >>      27:       1966       1900GIC-0  27 Level     arch_timer
> >>      29:        580          0GIC-0 205 Level     e60b0000.i2c
> >>      30:          0          0GIC-0 174 Level     ffca0000.timer
> >>      31:          0          0GIC-0  36 Level     e6050000.gpio
> >>      32:          0          0GIC-0  37 Level     e6051000.gpio
> >>      [...]
> >>
> >>making the output hard to read, and probably breaking scripts that parse
> >>its contents.
> >
> >Thanks for reporting this, I was considering the spaces and checked it on my system,
> >I thought "all" descriptions have leading spaces and it's ok to remove the extra one.
> >But I did not check all the "irq_print_chip" codes, now when
> >checking the code, there are many GPIO drivers' implementations with no leading spaces.
> >(The behavior is not consistent cross  driver implementations though...)
>
> Several drivers use dev_name as format string for seq_printf,  would this raise security concerns?
>
>        drivers/gpio/gpio-xgs-iproc.c:   seq_printf(p, dev_name(chip->dev));
>         drivers/gpio/gpio-mlxbf2.c:     seq_printf(p, dev_name(gs->dev));
>         drivers/gpio/gpio-omap.c:       seq_printf(p, dev_name(bank->dev));
>         drivers/gpio/gpio-hlwd.c:       seq_printf(p, dev_name(hlwd->dev));
>         drivers/gpio/gpio-aspeed.c:     seq_printf(p, dev_name(gpio->dev));
>         drivers/gpio/gpio-pca953x.c:    seq_printf(p, dev_name(gc->parent));
>         drivers/gpio/gpio-tegra186.c:   seq_printf(p, dev_name(gc->parent));
>         drivers/gpio/gpio-tegra.c:      seq_printf(s, dev_name(chip->parent));
>         drivers/gpio/gpio-ep93xx.c:     seq_printf(p, dev_name(gc->parent));
>         drivers/gpio/gpio-aspeed-sgpio.c:       seq_printf(p, dev_name(gpio->dev));
>         drivers/gpio/gpio-pl061.c:      seq_printf(p, dev_name(gc->parent));
>         drivers/gpio/gpio-visconti.c:   seq_printf(p, dev_name(priv->dev));

In theory, yes. But I guess it's hard to sneak a percent sign in these
device names.

But given the above, all of them should probably be updated to print
an initial space?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 01/13] kernel/irq/proc: use seq_put_decimal_ull_width() for decimal values

[David Wang]

At 2024-11-20 17:00:38, "Geert Uytterhoeven" <geert@linux-m68k.org> wrote:
>Hi David,
>

>>
>> Several drivers use dev_name as format string for seq_printf,  would this raise security concerns?
>>
>>        drivers/gpio/gpio-xgs-iproc.c:   seq_printf(p, dev_name(chip->dev));
>>         drivers/gpio/gpio-mlxbf2.c:     seq_printf(p, dev_name(gs->dev));
>>         drivers/gpio/gpio-omap.c:       seq_printf(p, dev_name(bank->dev));
>>         drivers/gpio/gpio-hlwd.c:       seq_printf(p, dev_name(hlwd->dev));
>>         drivers/gpio/gpio-aspeed.c:     seq_printf(p, dev_name(gpio->dev));
>>         drivers/gpio/gpio-pca953x.c:    seq_printf(p, dev_name(gc->parent));
>>         drivers/gpio/gpio-tegra186.c:   seq_printf(p, dev_name(gc->parent));
>>         drivers/gpio/gpio-tegra.c:      seq_printf(s, dev_name(chip->parent));
>>         drivers/gpio/gpio-ep93xx.c:     seq_printf(p, dev_name(gc->parent));
>>         drivers/gpio/gpio-aspeed-sgpio.c:       seq_printf(p, dev_name(gpio->dev));
>>         drivers/gpio/gpio-pl061.c:      seq_printf(p, dev_name(gc->parent));
>>         drivers/gpio/gpio-visconti.c:   seq_printf(p, dev_name(priv->dev));
>
>In theory, yes. But I guess it's hard to sneak a percent sign in these
>device names.

Yes, it is just theoretical... (Would be a wonderful story if someone manage it somehow :) )
Anyway, I send out another patch for further discussion.

>
>But given the above, all of them should probably be updated to print
>an initial space?
>
Oh, no, I did not mean to adding leading space for those in irq_print_chip()
I mentioned those just because of the format string thing.

Add leading space in those irq_print_chip() is kind of strange...
With Thomas's patch, irq_print_chip() needs not worry about the leading space issue. 


>Gr{oetje,eeting}s,
>
>                        Geert
>
>-- 
>Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
>In personal conversations with technical people, I call myself a hacker. But
>when I'm talking to journalists I just say "programmer" or something like that.
>                                -- Linus Torvalds


Thanks~
David

Re: [PATCH 01/13] kernel/irq/proc: use seq_put_decimal_ull_width() for decimal values

[Geert Uytterhoeven]

Hi David,

On Wed, Nov 20, 2024 at 10:36 AM David Wang <00107082@163.com> wrote:
> At 2024-11-20 17:00:38, "Geert Uytterhoeven" <geert@linux-m68k.org> wrote:
> >> Several drivers use dev_name as format string for seq_printf,  would this raise security concerns?
> >>
> >>        drivers/gpio/gpio-xgs-iproc.c:   seq_printf(p, dev_name(chip->dev));
> >>         drivers/gpio/gpio-mlxbf2.c:     seq_printf(p, dev_name(gs->dev));
> >>         drivers/gpio/gpio-omap.c:       seq_printf(p, dev_name(bank->dev));
> >>         drivers/gpio/gpio-hlwd.c:       seq_printf(p, dev_name(hlwd->dev));
> >>         drivers/gpio/gpio-aspeed.c:     seq_printf(p, dev_name(gpio->dev));
> >>         drivers/gpio/gpio-pca953x.c:    seq_printf(p, dev_name(gc->parent));
> >>         drivers/gpio/gpio-tegra186.c:   seq_printf(p, dev_name(gc->parent));
> >>         drivers/gpio/gpio-tegra.c:      seq_printf(s, dev_name(chip->parent));
> >>         drivers/gpio/gpio-ep93xx.c:     seq_printf(p, dev_name(gc->parent));
> >>         drivers/gpio/gpio-aspeed-sgpio.c:       seq_printf(p, dev_name(gpio->dev));
> >>         drivers/gpio/gpio-pl061.c:      seq_printf(p, dev_name(gc->parent));
> >>         drivers/gpio/gpio-visconti.c:   seq_printf(p, dev_name(priv->dev));
> >
> >In theory, yes. But I guess it's hard to sneak a percent sign in these
> >device names.
>
> Yes, it is just theoretical... (Would be a wonderful story if someone manage it somehow :) )
> Anyway, I send out another patch for further discussion.
>
> >But given the above, all of them should probably be updated to print
> >an initial space?
> >
> Oh, no, I did not mean to adding leading space for those in irq_print_chip()
> I mentioned those just because of the format string thing.
>
> Add leading space in those irq_print_chip() is kind of strange...
> With Thomas's patch, irq_print_chip() needs not worry about the leading space issue.

Sure, but there's still a slight misalignment if you have multiple
irqchips of different types:

153:          0          0 GIC-0 300 Level     feb00000.display
155:          0          0  da9063-irq   1 Level     ALARM
183:          1          0      irqc   0 Level     ee700000.ethernet-ffffffff:01
184:          0          0 GIC-0 197 Level     ee100000.mmc
185:         52          0 GIC-0 199 Level     ee140000.mmc
186:          0          0 GIC-0 200 Level     ee160000.mmc
187:          0          0  gpio-rcar   6 Edge      ee100000.mmc cd

I have just sent out a fix for another preexisting misalignment on ARM
https://lore.kernel.org/96f61cafee969c59796ac06c1410195fa0f1ba0b.1732096154.git.geert+renesas@glider.be

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds