Hi,
On 2024/2/27 17:26, Thomas Gleixner wrote:
On Mon, Feb 26 2024 at 10:09, Bitao Hu wrote:
We could use the irq_desc::tot_count member to avoid the summation
loop for interrupts which are not marked as 'PER_CPU' interrupts in
'show_interrupts'. This could reduce the time overhead of reading
/proc/interrupts.
"Could" is not really a technical term. Either we do or we do not. Also
please provide context for your change and avoid the 'We'.
OK.
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -121,6 +121,8 @@ static inline void irq_unlock_sparse(void) { }
extern struct irq_desc irq_desc[NR_IRQS];
#endif
+extern bool irq_is_nmi(struct irq_desc *desc);
+
If at all this wants to be in kernel/irq/internal.h. There is zero
reason to expose this globally.
-static bool irq_is_nmi(struct irq_desc *desc)
+bool irq_is_nmi(struct irq_desc *desc)
{
return desc->istate & IRQS_NMI;
}
If at all this really wants to be a static inline in internals.h, but
instead of blindly copying code this can be done smarter:
unsigned int kstat_irq_desc(struct irq_desc *desc)
{
unsigned int sum = 0;
int cpu;
if (!irq_settings_is_per_cpu_devid(desc) &&
!irq_settings_is_per_cpu(desc) &&
!irq_is_nmi(desc))
return data_race(desc->tot_count);
for_each_possible_cpu(cpu)
sum += data_race(*per_cpu_ptr(desc->kstat_irqs, cpu));
return sum;
}
and then let kstat_irqs() and show_interrupts() use it. See?
I have a concern. kstat_irqs() uses for_each_possible_cpu() for
summation. However, show_interrupts() uses for_each_online_cpu(),
which means it only outputs interrupt statistics for online cpus.
If we use for_each_possible_cpu() in show_interrupts() to calculate
'any_count', there could be a problem with the following scenario:
If an interrupt has a count of zero on online cpus but a non-zero
count on possible cpus, then 'any_count' would not be zero, and the
statistics for that interrupt would be output, which is not the
desired behavior for show_interrupts(). Therefore, I think it's not
good to have kstat_irqs() and show_interrupts() both use the same
logic. What do you think?
With that a proper changelog would be:
show_interrupts() unconditionally accumulates the per CPU interrupt
statistics to determine whether an interrupt was ever raised.
This can be avoided for all interrupts which are not strictly per CPU
and not of type NMI because those interrupts provide already an
accumulated counter. The required logic is already implemented in
kstat_irqs().
Split the inner access logic out of kstat_irqs() and use it for
kstat_irqs() and show_interrupts() to avoid the accumulation loop
when possible.
Best Regards,
Bitao Hu
