Correctly track index of first free irq map entry

[cperciva]

Summary

Any time an IRQ map entry was removed, irq_map_first_free_idx was being
set to the index of the removed entry; this caused problems when
entries were removed in random order since irq_map_first_free_idx was
set to a larger value than the index of the first free map entry, and
in 9beb195fd9fd ("Continue searching for an irq map from the start")
the IRQ map allocation code was adjusted to use irq_map_first_free_idx
as a starting point but ultimately scan the entire map if necessary,
including values less than irq_map_first_free_idx.

Remove that workaround and instead make irq_map_first_free_idx do what
the name suggests -- tracking the index of the first free map entry --
by only setting to the index of a newly-freed entry if that index is
lower than the existing irq_map_first_free_idx value.

Sponsored by: Amazon

[andrew]

Isn't this just working around a bug in the pci code that assumes an interrupt ID can't be PCI_INVALID_IRQ? This change doesn't fix that, just less likely to be seen. I'm not sure how useful the interrupt number field is on arm64 as it depends on which interrupt controller is used.

[cperciva]

In D49543#1129672, @andrew wrote:

Isn't this just working around a bug in the pci code that assumes an interrupt ID can't be PCI_INVALID_IRQ? This change doesn't fix that, just less likely to be seen. I'm not sure how useful the interrupt number field is on arm64 as it depends on which interrupt controller is used.

It's not even doing that, really. This doesn't prevent the panic I tripped over earlier since we're still leaking an IRQ.

This is just a matter of me noticing that the code could be made cleaner.

[mhorne]

IMO it is logically preferable to exhaust all unique/unused IDs before issuing one that was previously assigned, then freed. This is how the current code behaves.

I agree there is a shortcoming with respect to the naming of irq_map_first_free_idx.

Practically speaking either approach is unlikely to matter much, but I am inclined to vote for the current behaviour.

[cperciva]

In D49543#1129770, @mhorne wrote:

IMO it is logically preferable to exhaust all unique/unused IDs before issuing one that was previously assigned, then freed. This is how the current code behaves.

Well, no. We're going back to the most recently freed ID. If we want to exhaust the entire ID space before reusing IDs then we should remove the irq_map_first_free_idx = res_id line from intr_unmap_irq entirely.

Which would be a perfectly sensible option, and would help with finding bugs! But right now we're in a weird middle ground of sometimes reusing IDs and sometimes not.

I agree there is a shortcoming with respect to the naming of irq_map_first_free_idx.

Practically speaking either approach is unlikely to matter much, but I am inclined to vote for the current behaviour.

The one user-visible difference is that if we avoid reusing freed IDs then we can quickly hit PCI_INVALID_IRQ and get a panic. If we reuse freed IDs then we only hit that panic when we leak an IRQ (which is a separate issue I'm talking to @jhb about).

If someone can figure out how to resolve the "returning PCI_INVALID_IRQ from intr_map_irq makes bad things happen" issue then I agree there will be no functional difference.

[cperciva]

In D49543#1129854, @cperciva wrote:

The one user-visible difference is that if we avoid reusing freed IDs then we can quickly hit PCI_INVALID_IRQ and get a panic. If we reuse freed IDs then we only hit that panic when we leak an IRQ (which is a separate issue I'm talking to @jhb about).

If someone can figure out how to resolve the "returning PCI_INVALID_IRQ from intr_map_irq makes bad things happen" issue then I agree there will be no functional difference.

Never mind, once the bogus PCI IRQ allocation is gone (D49560) there's no panic because intr_map_irq isn't returning into the PCI code.

So no user-visible difference here.

@mhorne Would you prefer to remove the irq_map_first_free_idx = res_id entirely so we avoid reusing IDs? Or do you prefer the status quo of reusing some but not all IDs?

[mhorne]

In D49543#1129885, @cperciva wrote:

In D49543#1129854, @cperciva wrote:

The one user-visible difference is that if we avoid reusing freed IDs then we can quickly hit PCI_INVALID_IRQ and get a panic. If we reuse freed IDs then we only hit that panic when we leak an IRQ (which is a separate issue I'm talking to @jhb about).

If someone can figure out how to resolve the "returning PCI_INVALID_IRQ from intr_map_irq makes bad things happen" issue then I agree there will be no functional difference.

Never mind, once the bogus PCI IRQ allocation is gone (D49560) there's no panic because intr_map_irq isn't returning into the PCI code.

So no user-visible difference here.

The PCIB/ACPI interactions are outside my domain of knowledge. However, the following snippet from the end of acpi_pcib_route_interrupt() is confusing to me:

#ifdef INTRNG
    if (PCI_INTERRUPT_VALID(interrupt)) {
	interrupt  = acpi_map_intr(dev, interrupt, lnkdev);
	KASSERT(PCI_INTERRUPT_VALID(interrupt), ("mapping fail"));
    }
#endif

Why is interrupt, a value seemingly selected from the ACPI/PCI IRQ space (and respecting PCI_INVALID_IRQ), overwritten by the propagated result from intr_map_irq()?

The IRQ namespace managed by INTRNG is global, virtual, and (should be) totally separate from whatever a given bus considers an IRQ that would be assigned to, or appropriate for, a device. (Hence why it is mapped at all). We place no constraint on the legal values of the INTRNG IRQ namespace, except a tunable upper bound.

But this is a drive-by observation, and maybe I'm missing something.

@mhorne Would you prefer to remove the irq_map_first_free_idx = res_id entirely so we avoid reusing IDs? Or do you prefer the status quo of reusing some but not all IDs?

Ahh indeed you are right, and that I was too hasty in my initial re-reading of the code. Given that immediate reuse of IDs is the current intent of the code (not avoiding reuse like I thought), the change you propose here is fine by me.

sys/kern/subr_intr.c

1736–1744

Since you are removing this block, consider running the (somewhat contrived) test case that motivated it, from here:

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=271990

[cperciva]

sys/kern/subr_intr.c

1736–1744

Thanks for pointing me at that. Everything works fine in that test case with my change. In fact it works a bit better than it used to, since the same IDs get allocated every time.