Page MenuHomeFreeBSD

intrng: track counter allocation with a bitmap
ClosedPublic

Authored by mhorne on Feb 8 2023, 1:43 PM.
Tags
None
Referenced Files
Unknown Object (File)
Jan 3 2024, 5:21 AM
Unknown Object (File)
Dec 23 2023, 1:06 AM
Unknown Object (File)
Dec 22 2023, 3:17 PM
Unknown Object (File)
Dec 11 2023, 1:07 AM
Unknown Object (File)
Sep 5 2023, 11:57 PM
Unknown Object (File)
Aug 14 2023, 10:41 AM
Unknown Object (File)
Aug 14 2023, 3:50 AM
Unknown Object (File)
Jun 20 2023, 4:23 PM
Subscribers

Details

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.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

mhorne requested review of this revision.Feb 8 2023, 1:43 PM
mjg added inline comments.
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("....");
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.

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).

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).

This revision is now accepted and ready to land.Feb 11 2023, 3:42 PM

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.