intrng: track counter allocation with a bitmap

[mhorne]

Summary

Crucially, this allows releasing counters, and interrupt sources by
extension. Where before we were incrementing intrcnt_index with atomics,
now we protect the bitmap using the existing isrc_table_lock mutex.

[mjg]

sys/kern/subr_intr.c

186

while perhaps beyond the scope of this patch, this needs to be converted to a per-cpu alloc

323

this should be a non-invariants check, as in

if (index == -1)
        panic("....");

[mhorne]

sys/kern/subr_intr.c

186

Indeed. There is quite a bit of complexity/assumptions around this simple interface (I'm talking about intrcnt/intrnames arrays), which makes such a conversion a bigger lift than it would seem at first glance.

Most interrupts are bound to a single CPU for their entire lifetime, unless explicitly rebound. So I guess you are speaking mostly about having per-CPU pools of counters to pull from when setting up a new isrc.

[mhorne]

KASSERT() -> panic()

[ehem_freebsd_m5p.com]

I strongly dislike the approach as it is piling more complexity onto a surprisingly complex situation. While the interface definition is simple, keeping it operational is expensive. This is one of the reasons for the approach of D36610. Worse, it isn't immediately clear what the interface is, you have to look at examples in order to guess.

Yet this does deal with the immediate problem, so I've no objection there.

sys/kern/subr_intr.c

156–157

I believe vmstat is merely checking for the existence of nintrcnt, not the type. As such I would leave this as a u_int (unless I'm wrong about my understanding of vmstat).

[mmel]

I like that a lot, I'd do it anyway, thanks. .

To keep the code flexible we should separate intrng core, kernel interface and statistics as much as possible.

We shouldn't need to break all these layers by putting them all in one big mishmash.

sys/kern/subr_intr.c

186

IMHO this should be solved by extending counter(9) with an operation on a given (largest, depending on the platform) data type that allows atomic reads (and writes) without locking (so it allows incrementing in an interrupt context while maintaining consistent reads).

[mhorne]

In D38437#874784, @ehem_freebsd_m5p.com wrote:

I strongly dislike the approach as it is piling more complexity onto a surprisingly complex situation. While the interface definition is simple, keeping it operational is expensive. This is one of the reasons for the approach of D36610. Worse, it isn't immediately clear what the interface is, you have to look at examples in order to guess.

Yet this does deal with the immediate problem, so I've no objection there.

The changed logic does not block future improvements. It can be removed just the same as the intrcnt_index scheme in favor of a larger counter rework.