Re: [PATCHv11 2/4] genirq: Provide a snapshot mechanism for interrupt statistics

[Bitao Hu <yaoma@linux.alibaba.com>]

Hi,

On 2024/3/2 03:22, Thomas Gleixner wrote:
> Doug!
> 
> On Wed, Feb 28 2024 at 14:44, Doug Anderson wrote:
>> I won't insist on it, but I continue to worry about memory
>> implications with large numbers of CPUs. With a 4-byte int, 8192 max
>> CPUs, and 100 IRQs the extra "ref" value takes up over 3MB of memory
>> (8192 * 4 bytes * 100).
>>
>> Technically, you could add a new symbol like "config
>> NEED_IRQ_SNAPSHOTS". This wouldn't be a symbol selectable by the end
>> user but would automatically be selected by "config
>> SOFTLOCKUP_DETECTOR_INTR_STORM". If the config wasn't defined then the
>> struct wouldn't contain "ref" and the snapshot routines would just be
>> static inline stubs.
>>
>> Maybe Thomas has an opinion about whether this is something to worry
>> about. Worst case it wouldn't be hard to do in a follow-up patch.
> 
> I'd say it makes sense to give people a choice to save memory especially
> when the softlock detector code is not enabled.
> 
> It's rather straight forward to do.
> 
> Thanks,
> 
>          tglx
> ---
> --- a/include/linux/irqdesc.h
> +++ b/include/linux/irqdesc.h
> @@ -24,7 +24,9 @@ struct pt_regs;
>    */
>   struct irqstat {
>   	unsigned int	cnt;
> +#ifdef CONFIG_GENIRQ_STAT_SNAPSHOT
>   	unsigned int	ref;
> +#endif
>   };
>   
>   /**
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -978,6 +978,7 @@ static unsigned int kstat_irqs(unsigned
>   	return sum;
>   }
>   
> +#ifdef CONFIG_GENIRQ_STAT_SNAPSHOT
>   void kstat_snapshot_irqs(void)
>   {
>   	struct irq_desc *desc;
> @@ -998,6 +999,7 @@ unsigned int kstat_get_irq_since_snapsho
>   		return 0;
>   	return this_cpu_read(desc->kstat_irqs->cnt) - this_cpu_read(desc->kstat_irqs->ref);
>   }
> +#endif
>   
>   /**
>    * kstat_irqs_usr - Get the statistics for an interrupt from thread context
> --- a/kernel/irq/Kconfig
> +++ b/kernel/irq/Kconfig
> @@ -108,6 +108,10 @@ config GENERIC_IRQ_MATRIX_ALLOCATOR
>   config GENERIC_IRQ_RESERVATION_MODE
>   	bool
>   
> +# Snapshot for interrupt statistics
> +config GENERIC_IRQ_STAT_SNAPSHOT
> +	bool
> +
>   # Support forced irq threading
>   config IRQ_FORCED_THREADING
>          bool

I think we should follow Douglas's suggestion by making
"config GENERIC_IRQ_STAT_SNAPSHOT" automatically selectable by
"config SOFTLOCKUP_DETECTOR_INTR_STORM". This can prevent users
from inadvertently disabling "config GENERIC_IRQ_STAT_SNAPSHOT"
while enabling "config SOFTLOCKUP_DETECTOR_INTR_STORM".

Best Regards,
	Bitao Hu

diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
index 2531f3496ab6..9cf3b2d4c2a8 100644
--- a/kernel/irq/Kconfig
+++ b/kernel/irq/Kconfig
@@ -108,6 +108,15 @@ config GENERIC_IRQ_MATRIX_ALLOCATOR
  config GENERIC_IRQ_RESERVATION_MODE
         bool

+# Snapshot for interrupt statistics
+config GENERIC_IRQ_STAT_SNAPSHOT
+       bool
+       help
+
+         Say Y here to enable the kernel to provide a snapshot mechanism
+         for interrupt statistics.
+
+
  # Support forced irq threading
  config IRQ_FORCED_THREADING
         bool
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 49f652674bd8..899b69fcb598 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1032,6 +1032,7 @@ config SOFTLOCKUP_DETECTOR
  config SOFTLOCKUP_DETECTOR_INTR_STORM
         bool "Detect Interrupt Storm in Soft Lockups"
         depends on SOFTLOCKUP_DETECTOR && IRQ_TIME_ACCOUNTING
+       select GENERIC_IRQ_STAT_SNAPSHOT
         default y if NR_CPUS <= 128
         help
           Say Y here to enable the kernel to detect interrupt storm