Interrupt counters on `struct intr_event` #1301

ehem · 2024-06-21T00:07:56Z

Every architecture has counters just outside struct intr_event. As such it seems appropriate to try to move them onto struct intr_event to reduce differences between architectures.

There is also an issue of the interface between architecture and core. The core has been implementing hw.intrcnt and hw.intrnames, yet forcing architectures to handle the tables behind these. This results in rather troublesome mixing.

ehem · 2024-06-21T00:10:22Z

I do not guarantee I've sufficiently resolved issues brought up via Phabricator, even though I have tried. This seems the way to go.

sys/kern/kern_intr.c

ehem · 2024-06-21T01:05:01Z

sys/kern/kern_intr.c

+	if (!thread && !filter) {
+		++ie->ie_stray;
+		return (stray ? ie->ie_stray : 0);
+	}


Kind of a wishlist item. I'm looking at a device which is known to generate a stray interrupt during start. I would rather like to have it counted, yet returning FILTER_STRAY causes all interrupt controllers to mask the interrupt. Given the device, this would be Bad, as a result the present driver simply always returns FILTER_HANDLED.

ehem · 2024-06-21T01:07:10Z

sys/kern/kern_intr.c

+	if ((flags & INTR_MULTIPROC) && !(ie->ie_flags & IE_MULTIPROC)) {
+#if defined(INVARIANTS) || defined(DEBUG)
+		printf("%s: Requested multi-processor interupt, but got "
+		    "uniprocessor interrupt\n", name);
+#endif
+		return (ENODEV);
+	}
+#if defined(INVARIANTS) || defined(DEBUG)
+	if (!(flags & INTR_MULTIPROC) && (ie->ie_flags & IE_MULTIPROC))
+		printf("%s: Requested uniprocessor interupt, but got "
+		    "multi-processor interrupt\n", name);
+#endif
+


Seems something which really should have some way for a driver to indicate, "I'd damn well better get a PPI interrupt, or I'll panic()!"

ehem · 2024-07-23T20:19:59Z

I'm planning to update to FreeBSD's HEAD soon. There will be a rebase in a few hours.

ehem · 2024-08-05T23:06:51Z

sys/kern/kern_clock.c

+		if (!CK_SLIST_EMPTY(&clk_intr_event->ie_handlers))
+			intr_event_handle(clk_intr_event, NULL);


@jrtc27 was objecting to this on #1358, due to looking inside struct intr_event. Issue would be, what approach should instead be used? Perhaps clock interrupts aren't considered stray if there are no handlers? Perhaps a flag to indicate no handler shouldn't be considered stray?

ehem · 2024-08-08T01:05:14Z

I don't recall who forced per-processor for PPI counters due to indicating they would veto atomic increment. This does provide the valuable feature of showing which processors the PPI is occurring on. Otherwise you're left unsure whether your PPI interrupt being triggered on a single processor, or several. Also tells you how evenly it is spread. On balance I think this is distinctly superior to INTRNG's choice to use atomic increment.

ehem · 2024-09-17T02:14:13Z

Note, ARM64 has been tested and was helpful with confirming a fully operational PPI interrupt. x86 and PowerPC are untested. I guess the point here is, why is this information being copied back and forth between the events and the architecture tables, and simply being duplicated in both places? All of this really should be in one place.

Notably this causes `sbuf_printf("%c", 0);` to produce the expected result. Appears the behavior was mistakenly added to `sbuf_putc()` at cab5b96. This propogated to `sbuf_printf()` at 01f6f5f. At eb05ee7 this was removed from `sbuf_putc()`, but not `sbuf_printf()`. A bug with a long history.

Reading event names doesn't require exclusive access. Differential Revision: https://reviews.freebsd.org/D38450

The tables of interrupt names and counts are a layering violation. Moving the SYSCTL_PROC()s to architecture functions as a first step towards moving the handling to the architecture. Differential Revision: https://reviews.freebsd.org/D38454

Some interrupt types can be signaled on multiple processors. These need adjustment to their handling (notably adjustment to counter handling). Differential Revision: https://reviews.freebsd.org/D38448

The counters also really belong to the interrupting device, not the controller. As such they should be in the device-side structure, not the interrupt controller structure. Additionally this is an architecture-independent area. This should increase shared source code by taking over. Differential Revision: https://reviews.freebsd.org/D41430

Interrupts which occur on multiple processors need special handling. They need locking, atomics or else per-processor counters. Since recording which processors they occur on is valuable, create an array of per-processor counters. Most systems are unlikely to have many such interrupts, so keep the array fairly small for now.

Announce as these are allocated to assist debugging issues with these counters (as they are a limited resource).

Comparable to the current situation. Minor savings, one caller simply passes the count directly through. Differential Revision: https://reviews.freebsd.org/D38449

This allows architectures to provide the interrupt names/counters from intr_event, instead of the architecture tables. Since the tables are inferior for normal interrupts, but functional for IPI interrupts this is a substantial step forward. Differential Revision: https://reviews.freebsd.org/D36610

The tables of interrupt names and counts are a layering violation. Let architecture code handle these as this form causes trouble for architecture interrupt implementations. Differential Revision: https://reviews.freebsd.org/D38454

For most operations using the count of interrupts instead of the raw table size is superior. Notably adding entries is faster. Computing the full table size is quite fast. In the longer term a single variable is smaller too. Differential Revision: https://reviews.freebsd.org/D38116

Clean out the now-unused counters/interrupt names for normal interrupts. The table is now strictly used for IPI interrupts, so all of this support can go. Differential Revision: https://reviews.freebsd.org/D38451

This allows architectures to provide the interrupt names/counters from intr_event, instead of the architecture tables. Since the tables are inferior for normal interrupts, but functional for IPI interrupts this is a substantial step forward. Differential Revision: https://reviews.freebsd.org/D36610

These are now strictly being used for counting IPI interrupts, thus can be removed from monoprocessor builds.

The tables of interrupt names and counts are a layering violation. Let architecture code handle these as this form causes trouble for architecture interrupt implementations. Differential Revision: https://reviews.freebsd.org/D38454

Since all interrupt table implementations use the same index variable to indicate current table size, use that instead of size variables. Differential Revision: https://reviews.freebsd.org/D38116

This allows architectures to provide the interrupt names/counters from intr_event, instead of the architecture tables. Since the tables are inferior for normal interrupts, but functional for IPI interrupts this is a substantial step forward. Differential Revision: https://reviews.freebsd.org/D36610

These counters are now used strictly for IPI/PPI interrupts. Purge support for normal interrupts as those are now *strictly* on intr_event. The comment at the nintrcnt computation was incorrect. Multiple hypervisor drivers use a per-core set of interrupts, but generally only one hypervisor driver will be loaded. Differential Revision: https://reviews.freebsd.org/D38453

The tables of interrupt names and counts are a layering violation. Let architecture code handle these as this form causes trouble for architecture interrupt implementations. Differential Revision: https://reviews.freebsd.org/D38454

Since all interrupt table implementations use the same index variable to indicate current table size, use that instead of size variables. Differential Revision: https://reviews.freebsd.org/D38116

This allows architectures to provide the interrupt names/counters from intr_event, instead of the architecture tables. Since the tables are inferior for normal interrupts, but functional for IPI interrupts this is a substantial step forward. Differential Revision: https://reviews.freebsd.org/D36610

The counters/names are now used strictly for IPI/PPI interrupts. Rather more of the source was shared between IPI and normal interrupts, so relatively little disappears. Differential Revision: https://reviews.freebsd.org/D38452

The sysctl()s are now handled by the architecture. The tables were a layering violation and have been removed. Let architecture implementations modify their handling depending on their needs. Differential Revision: https://reviews.freebsd.org/D38454

These are now strictly being used for counting IPI interrupts, thus can be removed from monoprocessor builds.

This needs checking. I'm unsure of why the notyet is still here, but removing it /might/ be bad. Differential Revision: https://reviews.freebsd.org/D38449

Filter function can return more than just FILTER_STRAY. As such a filter might return FILTER_STRAY | FILTER_HANDLED to indicate stray counters should be incremented, but the interrupt should NOT be masked.

Some interrupt types can be signaled on multiple processors. These need adjustment to their handling (notably adjustment to counter handling). Mark clk_intr_event as being multi-processor (it is). Differential Revision: https://reviews.freebsd.org/D38448

This should ensure devices needing a PPI interrupt receive a PPI interrupt. In debug kernels warn in case of mismatch between request and retrieved intr_event. Differential Revision: https://reviews.freebsd.org/D38448

The "???" interrupt doesn't appear useful any more, so remove it. Differential Revision: https://reviews.freebsd.org/D37869

clk_intr_event is a clock interrupt, not a generalized interrupt. As such everything should be in clock.h/kern_clock.c instead of interrupt.h/kern_intr.c.

clk_intr_event was added strictly for ACPI. While additional uses could be added in the future, presently this is ACPI-only. Remove it from non-ACPI kernels. Correct the uses of intr_event_handle(). If no handlers are added by ACPI, invoking intr_event_handle() is incorrect as it is stray. Fixes: aba10e1 ("Allow swi_sched() to be called from NMI context.") Differential Revision: https://reviews.freebsd.org/D40776

If swi_add() is used to initialize an event without adding a handler, the name should be used as the device name. Otherwise there can be mysterious unnamed events lying around.

ehem commented Jun 21, 2024

View reviewed changes

sys/kern/kern_intr.c Show resolved Hide resolved

ehem commented Jun 21, 2024

View reviewed changes

sys/kern/kern_intr.c Outdated Show resolved Hide resolved

ehem commented Jun 21, 2024

View reviewed changes

ehem force-pushed the intrcnt branch from 251121b to f83c369 Compare July 23, 2024 22:30

ehem force-pushed the intrcnt branch from f83c369 to ed65fa5 Compare August 2, 2024 20:52

ehem mentioned this pull request Aug 3, 2024

Minimal collection for running on Xen/ARM64 #1358

Draft

ehem force-pushed the intrcnt branch 2 times, most recently from 8bd76cd to d862657 Compare August 5, 2024 23:01

ehem commented Aug 5, 2024

View reviewed changes

ehem force-pushed the intrcnt branch from d862657 to 68d3cbb Compare October 25, 2024 21:27

ehem added 15 commits November 4, 2024 10:44

kern/intr: replace mutex with read-write lock

a9a2f2a

Reading event names doesn't require exclusive access. Differential Revision: https://reviews.freebsd.org/D38450

kern/intr: add intr_event flag for multi-proc interrupts

452946e

Some interrupt types can be signaled on multiple processors. These need adjustment to their handling (notably adjustment to counter handling). Differential Revision: https://reviews.freebsd.org/D38448

kern/intr: add verbose boot message for multi-processor counters

f23751c

Announce as these are allocated to assist debugging issues with these counters (as they are a limited resource).

kern/intr: switch intr_event_handle() to return stray count

89f672d

Comparable to the current situation. Minor savings, one caller simply passes the count directly through. Differential Revision: https://reviews.freebsd.org/D38449

intrng: remove normal interrupts from intrcnt/intrnames

fdcfdb2

Clean out the now-unused counters/interrupt names for normal interrupts. The table is now strictly used for IPI interrupts, so all of this support can go. Differential Revision: https://reviews.freebsd.org/D38451

intrng: remove intrcnts from non-SMP builds

d5d2c69

These are now strictly being used for counting IPI interrupts, thus can be removed from monoprocessor builds.

ehem added 17 commits November 4, 2024 10:44

intr/x86: switch to index vars from table size vars

3da35a8

Since all interrupt table implementations use the same index variable to indicate current table size, use that instead of size variables. Differential Revision: https://reviews.freebsd.org/D38116

intr/powerpc: switch to index var from table size vars

0e7b398

Since all interrupt table implementations use the same index variable to indicate current table size, use that instead of size variables. Differential Revision: https://reviews.freebsd.org/D38116

intrng: remove intrcnts from non-SMP builds

c4e62d7

These are now strictly being used for counting IPI interrupts, thus can be removed from monoprocessor builds.

kern/intr: enable stray counting for non-acknowledged interrupts

7e9b476

This needs checking. I'm unsure of why the notyet is still here, but removing it /might/ be bad. Differential Revision: https://reviews.freebsd.org/D38449

kern/intr: add handling for handled stray filter return

e66d837

Filter function can return more than just FILTER_STRAY. As such a filter might return FILTER_STRAY | FILTER_HANDLED to indicate stray counters should be incremented, but the interrupt should NOT be masked.

kern/intr: enforce PPI interrupt for PPI requests

99cd9c2

This should ensure devices needing a PPI interrupt receive a PPI interrupt. In debug kernels warn in case of mismatch between request and retrieved intr_event. Differential Revision: https://reviews.freebsd.org/D38448

sys/intr: remove questionable interrupt report

bc3edc6

The "???" interrupt doesn't appear useful any more, so remove it. Differential Revision: https://reviews.freebsd.org/D37869

kern: move clk_intr_event to clock.[ch]

c99b038

clk_intr_event is a clock interrupt, not a generalized interrupt. As such everything should be in clock.h/kern_clock.c instead of interrupt.h/kern_intr.c.

kern/intr: use swi_add() string as name if no handler

110d7c7

If swi_add() is used to initialize an event without adding a handler, the name should be used as the device name. Otherwise there can be mysterious unnamed events lying around.

ehem force-pushed the intrcnt branch from 68d3cbb to 110d7c7 Compare November 4, 2024 18:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interrupt counters on `struct intr_event` #1301

Interrupt counters on `struct intr_event` #1301

ehem commented Jun 21, 2024

ehem commented Jun 21, 2024

ehem Jun 21, 2024

ehem Jun 21, 2024

ehem commented Jul 23, 2024

ehem Aug 5, 2024

ehem commented Aug 8, 2024

ehem commented Sep 17, 2024

		if (!CK_SLIST_EMPTY(&clk_intr_event->ie_handlers))
		intr_event_handle(clk_intr_event, NULL);

Interrupt counters on struct intr_event #1301

Are you sure you want to change the base?

Interrupt counters on struct intr_event #1301

Conversation

ehem commented Jun 21, 2024

ehem commented Jun 21, 2024

ehem Jun 21, 2024

Choose a reason for hiding this comment

ehem Jun 21, 2024

Choose a reason for hiding this comment

ehem commented Jul 23, 2024

ehem Aug 5, 2024

Choose a reason for hiding this comment

ehem commented Aug 8, 2024

ehem commented Sep 17, 2024

Interrupt counters on `struct intr_event` #1301

Interrupt counters on `struct intr_event` #1301