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

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

[David Wang]

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>
---
 kernel/irq/proc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index 9081ada81c3d..988ce781e813 100644
--- 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) {
-- 
2.39.2

[tip: irq/core] genirq/proc: Use seq_put_decimal_ull_width() for decimal values

[tip-bot2 for David Wang]

The following commit has been merged into the irq/core branch of tip:

Commit-ID:     f9ed1f7c2e26fcd19781774e310a6236d7525c11
Gitweb:        https://git.kernel.org/tip/f9ed1f7c2e26fcd19781774e310a6236d7525c11
Author:        David Wang <00107082@163.com>
AuthorDate:    Sat, 09 Nov 2024 00:07:17 +08:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Wed, 13 Nov 2024 17:36:35 +01:00

genirq/proc: Use seq_put_decimal_ull_width() for decimal values

seq_printf() is more expensive than seq_put_decimal_ull_width() due to the
format string parsing costs.

Profiling on a x86 8-core system indicates seq_printf() takes ~47% samples
of show_interrupts(). Replacing it with seq_put_decimal_ull_width() yields
almost 30% performance gain.

[ tglx: Massaged changelog and fixed up coding style ]

Signed-off-by: David Wang <00107082@163.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/all/20241108160717.9547-1-00107082@163.com

---
 kernel/irq/proc.c |  9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index d226282..f36c33b 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -495,9 +495,12 @@ 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);
-	for_each_online_cpu(j)
-		seq_printf(p, "%10u ", desc->kstat_irqs ? per_cpu(desc->kstat_irqs->cnt, j) : 0);
+	seq_printf(p, "%*d:", prec, i);
+	for_each_online_cpu(j) {
+		unsigned int cnt = desc->kstat_irqs ? per_cpu(desc->kstat_irqs->cnt, j) : 0;
+
+		seq_put_decimal_ull_width(p, " ", cnt, 10);
+	}
 
 	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

[Thomas Gleixner]

On Sat, Nov 09 2024 at 00:07, David Wang wrote:
> The improvement has pratical significance, considering many monitoring
> tools would read /proc/interrupts periodically.

I've applied this, but ...

looking at a 256 CPU machine. /proc/interrupts provides data for 560
interrupts, which amounts to ~1.6MB data size.

There are 560 * 256 = 143360 interrupt count fields. 140615 of these
fields are zero, which means 140615 * 11 bytes. That's 96% of the
overall data size. The actually useful information is less than
50KB if properly condensed.
 
I'm really amused that people spend a lot of time to improve the
performance of /proc/interrupts instead of actually sitting down and
implementing a proper new interface for this purpose, which would make
both the kernel and the tools faster by probably several orders of
magnitude.

Thanks,

        tglx

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

[David Wang]

Hi,

At 2024-11-14 03:10:08, "Thomas Gleixner" <tglx@linutronix.de> wrote:
>On Sat, Nov 09 2024 at 00:07, David Wang wrote:
>> The improvement has pratical significance, considering many monitoring
>> tools would read /proc/interrupts periodically.
>
>I've applied this, but ...
>
>looking at a 256 CPU machine. /proc/interrupts provides data for 560
>interrupts, which amounts to ~1.6MB data size.
>
>There are 560 * 256 = 143360 interrupt count fields. 140615 of these
>fields are zero, which means 140615 * 11 bytes. That's 96% of the
>overall data size. The actually useful information is less than
>50KB if properly condensed.
> 
>I'm really amused that people spend a lot of time to improve the
>performance of /proc/interrupts instead of actually sitting down and
>implementing a proper new interface for this purpose, which would make
>both the kernel and the tools faster by probably several orders of
>magnitude.

That's a great idea~.
I tried to make changes and verify the performance, the result is good,
only that kernel side improvement is not that big, but still significant.

Here is what I did, (draft codes are at the end):
Created two new /proc entry for comparision:
1. /proc/interruptsp,  non-zero value only, arch-independent irqs without description
	$ cat /proc/interruptsp 
	IRQ CPU counter # positive only
	0 0 40
	38 5 23
	39 0 81
	40 1 6
	41 2 111
	...
	$ cat /proc/interruptsp  | wc
	     18      57     181

	$ strace -e read -T cat /proc/interruptsp > /dev/null
	...
	read(3, "IRQ CPU counter # positive only\n"..., 131072) = 181 <0.000144>
	read(3, "", 131072)                     = 0 <0.000009>

	$ time ./interruptsp  # 1 million rounds of open/read(all)/close;

	real	1m54.727s
	user	0m0.368s
	sys	1m54.309s



2. /proc/interruptso, same with old format, except arch-dependent irqs are removed
	$ cat /proc/interruptso | wc
	     32     388    4439
	$ strace -e read -T cat /proc/interruptso > /dev/null
	...
	read(3, "            CPU0       CPU1     "..., 131072) = 4005 <0.000071>
	read(3, "  88:          0          0     "..., 131072) = 434 <0.000111>
	read(3, "", 131072)                     = 0 <0.000009>

        $ time ./interruptso # 1 million rounds of open/read(all)/close;

	real	2m19.284s
	user	0m0.400s
	sys	2m18.756s


The size is indeed tens of times shorter, and would have huge improvement
for those applications parsing the whole content; But as for kernel side
improvement, strace and stress test indicates the improvement is not 
that huge, well, but still significant ~40%.


The bottleneck seems to be mtree_load called by irq_to_desc, based on a simple
profiling (not sure whether this is expected or not):

	show_interruptsp(74.724% 845541/1131554)
	    mtree_load(56.596% 478544/845541)
	    __rcu_read_unlock(5.914% 50004/845541)
	    __rcu_read_lock(3.056% 25840/845541)
	    irq_to_desc(2.151% 18184/845541)
	    seq_put_decimal_ull_width(1.211% 10243/845541)
		...


I think the improvement worth pursuing. Maybe a new interface for "active"
interrupts, say /proc/activeinterrupts?, and the old /proc/interrupts can
serve as a table for available ids/cpus/descriptions.

Do you plan to work on this? If not, I can take time on it.



Draft codes:
	int show_interruptsp(struct seq_file *p, void *v)
	{

		int i = *(loff_t *) v, j;
		struct irq_desc *desc;

		if (i >= ACTUAL_NR_IRQS)
			return 0;

		/* print header and calculate the width of the first column */
		if (i == 0) {
			seq_puts(p, "IRQ CPU counter # positive only\n");
		}

		rcu_read_lock();
		desc = irq_to_desc(i);
		if (!desc || irq_settings_is_hidden(desc))
			goto outsparse;

		if (!desc->action || irq_desc_is_chained(desc) || !desc->kstat_irqs)
			goto outsparse;

		for_each_online_cpu(j) {
			unsigned int cnt = desc->kstat_irqs ? per_cpu(desc->kstat_irqs->cnt, j) : 0;
			if (cnt > 0) {
				seq_put_decimal_ull(p, "", i);
				seq_put_decimal_ull(p, " ", j);
				seq_put_decimal_ull(p, " ", cnt);
				seq_putc(p, '\n');
			}
		}

	outsparse:
		rcu_read_unlock();
		return 0;
	}

	int show_interruptso(struct seq_file *p, void *v)
	{
		static int prec;

		int i = *(loff_t *) v, j;
		struct irqaction *action;
		struct irq_desc *desc;
		unsigned long flags;

		if (i >= ACTUAL_NR_IRQS) <<---return when arch-independent irqs are done.
			return 0;   
		...



Thanks~
David

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

[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

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