Messages in this thread | | | Date | Tue, 27 Feb 2024 19:20:00 +0800 | Subject | Re: [PATCHv10 3/4] genirq: Avoid summation loops for /proc/interrupts | From | Bitao Hu <> |
| |
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
| |