Re: [PATCH 3/3] powerpc/irq: Suppress unlikely interrupt stats by default

[Shrikanth Hegde <sshegde@linux.ibm.com>]

Hi Christophe.
Thanks for reviewing the patch series.

On 5/29/26 1:21 PM, Christophe Leroy (CS GROUP) wrote:
> 
> 
> Le 23/05/2026 à 19:40, Shrikanth Hegde a écrit :
>> Some interrupts are always zero and that is expected since they occur
>> very rarely and are mostly error indications. Don't print them by
>> default.
>>
>> "MCE" -  "Machine check exceptions"
>> "NMI" -  "System Reset interrupts"
>>
>> Print them if they occur once. Maintain a bitmap to know which
>> interrupts are to be printed.
> 
> Is that bitmap needed at all ? Can't we just print them as soon as they 
> are not zero ?
> 

I think Yes. Otherwise need to traverse all the per_cpu count and to see 
if it is zero or not.


>>
>> Time taken to read /proc/interrupts 1000 times.
>> Base and v6 details can be found in cover-letter.
>> Base            :  103us
>> v6              :   63us
>> v6+patch 1+2    :   57us
>> v6+patch 1+2+3  :   54us
>>
>> Patch 3 shows an additional 5% gain compared to patch 1+2. So it does
>> make sense to print them only if they are ever set.
>>
>> Note: Since /proc/interrupts depend on kconfig and arch dependent,
>> userspace tools don't make explicit assumptions.
>>
>> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
>> ---
>>   arch/powerpc/include/asm/hardirq.h |  1 +
>>   arch/powerpc/kernel/irq.c          | 37 +++++++++++++++++++++++++++---
>>   arch/powerpc/kernel/traps.c        |  4 ++--
>>   3 files changed, 37 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/hardirq.h b/arch/powerpc/ 
>> include/asm/hardirq.h
>> index 38098e35b241..be6cd5aab016 100644
>> --- a/arch/powerpc/include/asm/hardirq.h
>> +++ b/arch/powerpc/include/asm/hardirq.h
>> @@ -31,6 +31,7 @@ DECLARE_PER_CPU(unsigned int, __softirq_pending);
>>   #define local_softirq_pending_ref       __softirq_pending
>>   #define inc_irq_stat(index)    
>> __this_cpu_inc(irq_stat.counts[IRQ_COUNT_##index])
>> +void inc_irq_stat_and_enable(enum irq_stat_counts which);
>>   #define __ARCH_IRQ_STAT
>>   #define __ARCH_IRQ_EXIT_IRQS_DISABLED
>> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
>> index e67a18f62142..048ddfa66fc4 100644
>> --- a/arch/powerpc/kernel/irq.c
>> +++ b/arch/powerpc/kernel/irq.c
>> @@ -87,9 +87,13 @@ u32 tau_interrupts(unsigned long cpu);
>>   struct irq_stat_info {
>>       const char    *symbol;
>>       const char    *text;
>> +    int        skip;
> 
> I'd call it 'optional' instead, and then during the print, if value 0 
> and optional then don't print.
> 

Ok. Makes sense. Will do it in v2.

Will add below way

         if (!(info->optional && test_bit(i, irq_stat_count_show)))
             continue;

>>   };
>> -#define ISE(idx, sym, txt)[IRQ_COUNT_##idx] = { .symbol = sym, .text 
>> = txt}
>> +/* ISE - IRQ STAT ENABLED, ISC - IRQ STAT CONDITIONAL */
>> +#define ISE(idx, sym, txt)[IRQ_COUNT_##idx] = { .symbol = sym, .text 
>> = txt, .skip = 0}
>> +#define ISC(idx, sym, txt)[IRQ_COUNT_##idx] = { .symbol = sym, .text 
>> = txt, .skip = 1}
>> +
>>   static struct irq_stat_info irq_stat_info[IRQ_COUNT_MAX] 
>> __ro_after_init = {
>>       ISE(LOC_TIMER,        "LOC", "  Local timer interrupts for timer 
>> event device\n"),
>> @@ -97,8 +101,8 @@ static struct irq_stat_info 
>> irq_stat_info[IRQ_COUNT_MAX] __ro_after_init = {
>>       ISE(OTHER_TIMER,    "LOC", "  Local timer interrupts for 
>> others\n"),
>>       ISE(SPURIOUS,        "SPU", "  Spurious interrupts\n"),
>>       ISE(PMI,        "PMI", "  Performance monitoring interrupts\n"),
>> -    ISE(MCE,        "MCE", "  Machine check exceptions\n"),
>> -    ISE(NMI_SRESET,        "NMI", "  System Reset interrupts\n"),
>> +    ISC(MCE,        "MCE", "  Machine check exceptions\n"),
>> +    ISC(NMI_SRESET,        "NMI", "  System Reset interrupts\n"),
>>   #ifdef CONFIG_PPC_WATCHDOG
>>       ISE(WATCHDOG,        "WDG", "  Watchdog soft-NMI interrupts\n"),
>>   #endif
>> @@ -107,11 +111,25 @@ static struct irq_stat_info 
>> irq_stat_info[IRQ_COUNT_MAX] __ro_after_init = {
>>   #endif
>>   };
>> +/*
>> + * Used for default disabled counters to increment the stats and to 
>> enable the
>> + * entry for /proc/interrupts output.
>> + */
>> +static DECLARE_BITMAP(irq_stat_count_show, IRQ_COUNT_MAX) __read_mostly;
>> +void inc_irq_stat_and_enable(enum irq_stat_counts which)
>> +{
>> +    __this_cpu_inc(irq_stat.counts[which]);
>> +    set_bit(which, irq_stat_count_show);
>> +}
>> +
>>   int arch_show_interrupts(struct seq_file *p, int prec)
>>   {
>>       const struct irq_stat_info *info = irq_stat_info;
>>       for (unsigned int i = 0; i < ARRAY_SIZE(irq_stat_info); i++, 
>> info++) {
>> +        if (!test_bit(i, irq_stat_count_show))
>> +            continue;
>> +
>>           seq_printf(p, "%*s:", prec, info->symbol);
>>           irq_proc_emit_counts(p, &irq_stat.counts[i]);
>>           seq_puts(p, info->text);
>> @@ -138,6 +156,19 @@ int arch_show_interrupts(struct seq_file *p, int 
>> prec)
>>       return 0;
>>   }
>> +static int __init irq_init_stats(void)
>> +{
>> +    struct irq_stat_info *info = irq_stat_info;
>> +
>> +    for (unsigned int i = 0; i < ARRAY_SIZE(irq_stat_info); i++, 
>> info++) {
>> +        if (info->skip == 0)
>> +            set_bit(i, irq_stat_count_show);
>> +    }
>> +
>> +    return 0;
>> +}
>> +late_initcall(irq_init_stats);
>> +
>>   /*
>>    * /proc/stat helpers
>>    */
>> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
>> index a8f15154bd9a..3eacbd20fc80 100644
>> --- a/arch/powerpc/kernel/traps.c
>> +++ b/arch/powerpc/kernel/traps.c
>> @@ -459,7 +459,7 @@ DEFINE_INTERRUPT_HANDLER_NMI(system_reset_exception)
>>       }
>>       hv_nmi_check_nonrecoverable(regs);
>> -    inc_irq_stat(NMI_SRESET);
>> +    inc_irq_stat_and_enable(IRQ_COUNT_NMI_SRESET);
>>       /* See if any machine dependent calls */
>>       if (ppc_md.system_reset_exception) {
>> @@ -816,7 +816,7 @@ static void __machine_check_exception(struct 
>> pt_regs *regs)
>>   {
>>       int recover = 0;
>> -    inc_irq_stat(MCE);
>> +    inc_irq_stat_and_enable(IRQ_COUNT_MCE);
>>       add_taint(TAINT_MACHINE_CHECK, LOCKDEP_NOW_UNRELIABLE);
>