Re: [PATCH 1/1] genirq/proc: Try to jump over the unallocated irq hole whenever possible

[Thomas Gleixner]

On Tue, Mar 26 2024 at 21:35, Huang Adrian wrote:
> On Tue, Mar 26, 2024 at 6:32 AM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> The reason I made the changes in show_interrupts() is to minimize the
> traversal times of the 'sparse_irqs' maple tree since irq_to_desc() is
> invoked in show_interrupts().

They are not really relevant. The cache line is hot after the
irq_get_next_irq() lookup and maple tree is a highly optimized data
structure.

I'm not saying that it is free, but if you want to avoid that in the
first place then you need to do a lookup of the next descriptor and hand
it into show_interrupts() right away and not just hack some completely
obscure optimization into show_interrupts().

> @@ -19,6 +19,10 @@ static void *int_seq_next(struct seq_file *f, void
> *v, loff_t *pos)
[ 3 more citation lines. Click/Enter to show. ]
>         (*pos)++;
>         if (*pos > nr_irqs)
>                 return NULL;
> +
> +       if (!irq_to_desc(*pos))
> +               *pos = irq_get_next_irq(*pos);

How is irq_get_next_irq() valid without either holding the sparse irq
lock or RCU read lock?

Testing this with full debug enabled might give you the answer to that
question.

But that aside you are missing a massive performance problem in the
generic show_interrupts() code:

	if (desc->kstat_irqs) {
		for_each_online_cpu(j)
			any_count |= data_race(*per_cpu_ptr(desc->kstat_irqs, j));
	}

	if ((!desc->action || irq_desc_is_chained(desc)) && !any_count)
		goto outsparse;

There are two obvious problems with that, no?

Thanks,

	tglx