Re: [PATCHv9 2/3] irq: use a struct for the kstat_irqs in the interrupt descriptor

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, Feb 22 2024 at 17:34, Bitao Hu wrote:

First of all the subsystem prefix is 'genirq:'. 'git log kernel/irq/'
gives you a pretty good hint. It's documented....

Secondly the subject line does not match what this patch is about. It's
not about using a struct, it's about providing a snapshot mechanism, no?

> The current implementation uses an int for the kstat_irqs in the
> interrupt descriptor.
>
> However, we need to know the number of interrupts which happened
> since softlockup detection took a snapshot in order to analyze
> the problem caused by an interrupt storm.
>
> Replacing an int with a struct and providing sensible interfaces
> for the watchdog code can keep it self contained to the interrupt
> core code.

So something like this makes a useful change log for this:

 Subject: genirq: Provide a snapshot mechanism for interrupt statistics

 The soft lockup detector lacks a mechanism to identify interrupt storms
 as root cause of a lockup. To enable this the detector needs a
 mechanism to snapshot the interrupt count statistics on a CPU when the
 detector observes a potential lockup scenario and compare that against
 the interrupt count when it warns about the lockup later on. The number
 of interrupts in that period give a hint whether the lockup might be
 caused by an interrupt storm.

 Instead of having extra storage in the lockup detector and accessing
 the internals of the interrupt descriptor directly, convert the per CPU
 irq_desc::kstat_irq member to a data structure which contains the
 counter plus a snapshot member and provide interfaces to take a
 snapshot of all interrupts on the current CPU and to retrieve the delta
 of a specific interrupt later on.

Hmm?

> Signed-off-by: Bitao Hu <yaoma@linux.alibaba.com>

Interesting. You fully authored the patch?

That's not how it works. You cannot take work from others and claim that
it is yours. The minimal courtesy is to add a 'Originally-by:' tag.

> diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
> index 623b8136e9af..3ad40cf30c66 100644
> --- a/kernel/irq/proc.c
> +++ b/kernel/irq/proc.c
> @@ -488,18 +488,15 @@ int show_interrupts(struct seq_file *p, void *v)
>  	if (!desc || irq_settings_is_hidden(desc))
>  		goto outsparse;
>  
> -	if (desc->kstat_irqs) {
> -		for_each_online_cpu(j)
> -			any_count |= data_race(*per_cpu_ptr(desc->kstat_irqs, j));
> -	}
> +	if (desc->kstat_irqs)
> +		any_count = data_race(desc->tot_count);

This is an unrelated change and needs to be split out into a separate
patch with a proper changelog which explains why this is equivalent.
  
Thanks,

        tglx