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@xxxxxxx> wrote:
> At 2024-11-20 09:37:04, "David Wang" <00107082@xxxxxxx> wrote:
> >At 2024-11-20 03:55:30, "Geert Uytterhoeven" <geert@xxxxxxxxxxxxxx> 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@xxxxxxx>
> >>
> >>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@xxxxxxxxxxxxxx

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