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

["David Wang" <00107082@163.com>]

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