"FreeBSD Phabricator III: The Search for Reviewers"

Two spots have multiple potential approaches and I'm unsure which to choose. Overall though this appears to have some rather nice properties since it avoids the allocation of a significant memory block. I expect this to make sysctl("hw.intrnames") use slightly more processor, but avoid consuming memory for a low-use interface.

Any word? I really would kind of like to get this in as it potentially blocks at least one device.

In its current form, this change has major problems, and is largely unreviewable. The main issue with your approach is that struct intr_event is not the correct place to stash these counters. This is evident by the fact that you are, in a few places, allocating empty (i.e. no handler) intr_event structures simply for this counter bookkeeping. This is a misuse of the structure, doing something completely outside its intended and established purpose (to execute/schedule interrupt handlers). This approach is not going to be accepted, so please set your expectations accordingly.

After some study, I agree that the intrcnt/intrnames interface is problematic. These counters belong to the machine-dependent interrupt frameworks (e.g. INTRNG), but the sysctls are implemented in the machine-independent intr_event code. This is a layering violation that forces MD code to allocate the counters and names in this inconvenient array format. An acceptable approach would be to eliminate the MI parts by moving (i.e. duplicating) the sysctl and ddb commands into each MD framework*. This will allow the implementations to evolve independently, while still maintaining the hw.intrnames/hw.intrcnt sysctl interface for userspace.

Ultimately, I believe you are searching for how to meaningfully implement isrc_release_counters(). In the INTRNG case, the counters could be stashed as part of struct intr_irqsrc, or still maintained in a dynamically-sized array, with a bitmap to track which counters are allocated. This would allow counters to be released, vs the current array+index scheme does not.

Any argument based on saved/wasted memory needs to be backed by some form of before/after measurement in order to be taken seriously.

I hope this helps.

*For the question of watchdog_fire(), I think the use of intrcnt and intrnames in that function can be dropped outright. I'm certain it was useful at one point, but since the information is available in ddb, I highly doubt anyone will miss it.

In D36610#858610, @mhorne wrote:

In its current form, this change has major problems, and is largely unreviewable. The main issue with your approach is that struct intr_event is not the correct place to stash these counters. This is evident by the fact that you are, in a few places, allocating empty (i.e. no handler) intr_event structures simply for this counter bookkeeping. This is a misuse of the structure, doing something completely outside its intended and established purpose (to execute/schedule interrupt handlers). This approach is not going to be accepted, so please set your expectations accordingly.

struct intr_event does center on this task, but this doesn't mean peripheral portions of this task shouldn't be part of struct intr_event. Could you suggest some test for distinguishing whether a given task/field/datum should be on struct intr_event versus what I'm calling interrupt_t (structures intr_irqsrc/intsrc/powerpc_intr)?

The model I would use is to map things to pre-MSI hardware. struct intr_event would then map to the interrupt pin on the interrupting device. interrupt_t would then map to a pin on the interrupt controller. The pair together would then equate to the "interrupt".

This means ie_irq belongs on interrupt_t since it is a property of the interrupt controller and not the device (D32504). This means the counters belong on struct intr_event since they are a property of the device not the controller.
For a controller which loses all interrupt mappings under certain conditions (suspend perhaps?) and has to reestablish them, this could be handled by having an array of interrupt_t and swapping the ->isrc_event fields around (yes, there is at least one controller which needs this). This keeps the handlers, counters and name all together with minimal effort (useful for display by vmstat -i).

Needing to allocate full struct intr_event for IPI counters is wasteful. Yet perhaps more handling of IPI interrupts could be done via core functions. Quite a bit of waste could be saved by allocating counters during individual processor bring-up instead of all at once. This avoids having large intrcnt/intrnames tables which by necessity must include substantial extra space.

In D36610#858610, @mhorne wrote:

After some study, I agree that the intrcnt/intrnames interface is problematic. These counters belong to the machine-dependent interrupt frameworks (e.g. INTRNG), but the sysctls are implemented in the machine-independent intr_event code. This is a layering violation that forces MD code to allocate the counters and names in this inconvenient array format. An acceptable approach would be to eliminate the MI parts by moving (i.e. duplicating) the sysctl and ddb commands into each MD framework*. This will allow the implementations to evolve independently, while still maintaining the hw.intrnames/hw.intrcnt sysctl interface for userspace.

I really dislike the duplication of source code this would cause (also the interface of "hw.intrnames" is surprisingly brittle). Having played with this a bit it seems more useful to have the counters on struct intr_event. An advantage to this approach is stray interrupts which were generated inside an interrupt controller could be recorded, though nominally those shouldn't happen since the interrupt shouldn't be unmasked until after a struct intr_event was allocated.

I'm not absolutely against this approach, just the results of going the opposite direction seem rather superior to me.

In D36610#858610, @mhorne wrote:

Ultimately, I believe you are searching for how to meaningfully implement isrc_release_counters(). In the INTRNG case, the counters could be stashed as part of struct intr_irqsrc, or still maintained in a dynamically-sized array, with a bitmap to track which counters are allocated. This would allow counters to be released, vs the current array+index scheme does not.

Your belief is correct in spirit. Any other approach which allows intr_isrc_deregister() to not simply be a wrapper around panic() and doesn't require too much effort on my part is nominally acceptable to me.

Of those two, counters on struct intr_irqsrc would be massively superior. This would allow a simple implementation for "hw.intrcnt" whereas tracking with a bitmap or linked list would require a rather more complicated implementation.

In D36610#858610, @mhorne wrote:

I hope this helps.

Does. Though some more convincing is still needed.

In D36610#858610, @mhorne wrote:

*For the question of watchdog_fire(), I think the use of intrcnt and intrnames in that function can be dropped outright. I'm certain it was useful at one point, but since the information is available in ddb, I highly doubt anyone will miss it.

If others are okay with this I would be quite happy with this.

In D36610#858662, @ehem_freebsd_m5p.com wrote:

In D36610#858610, @mhorne wrote:

In its current form, this change has major problems, and is largely unreviewable. The main issue with your approach is that struct intr_event is not the correct place to stash these counters. This is evident by the fact that you are, in a few places, allocating empty (i.e. no handler) intr_event structures simply for this counter bookkeeping. This is a misuse of the structure, doing something completely outside its intended and established purpose (to execute/schedule interrupt handlers). This approach is not going to be accepted, so please set your expectations accordingly.

struct intr_event does center on this task, but this doesn't mean peripheral portions of this task shouldn't be part of struct intr_event. Could you suggest some test for distinguishing whether a given task/field/datum should be on struct intr_event versus what I'm calling interrupt_t (structures intr_irqsrc/intsrc/powerpc_intr)?

Members of struct intr_event should be primarily (with small exceptions) accessed and managed within the kern_event(9) API, that is, kern_event.c. The counters are allocated and incremented outside of this file, even in the current version of your change. This is a good indicator that struct intr_event does not (and should not) own them.

In D36610#858610, @mhorne wrote:

After some study, I agree that the intrcnt/intrnames interface is problematic. These counters belong to the machine-dependent interrupt frameworks (e.g. INTRNG), but the sysctls are implemented in the machine-independent intr_event code. This is a layering violation that forces MD code to allocate the counters and names in this inconvenient array format. An acceptable approach would be to eliminate the MI parts by moving (i.e. duplicating) the sysctl and ddb commands into each MD framework*. This will allow the implementations to evolve independently, while still maintaining the hw.intrnames/hw.intrcnt sysctl interface for userspace.

I really dislike the duplication of source code this would cause (also the interface of "hw.intrnames" is surprisingly brittle). Having played with this a bit it seems more useful to have the counters on struct intr_event. An advantage to this approach is stray interrupts which were generated inside an interrupt controller could be recorded, though nominally those shouldn't happen since the interrupt shouldn't be unmasked until after a struct intr_event was allocated.

I'm not absolutely against this approach, just the results of going the opposite direction seem rather superior to me.

Well, then you need to weigh your desire to get this work accepted against your belief of how the code should look. I've given you my major objections.

In D36610#858610, @mhorne wrote:

I hope this helps.

Does. Though some more convincing is still needed.

I am not interested in spending time trying to argue over this or convince you, so I won't. It's evident to me that nobody else wants to touch these reviews, because they just grow and grow with vague questions and assertions while remaining completely non-actionable.

In D36610#858879, @mhorne wrote:

In D36610#858662, @ehem_freebsd_m5p.com wrote:

In D36610#858610, @mhorne wrote:

In its current form, this change has major problems, and is largely unreviewable. The main issue with your approach is that struct intr_event is not the correct place to stash these counters. This is evident by the fact that you are, in a few places, allocating empty (i.e. no handler) intr_event structures simply for this counter bookkeeping. This is a misuse of the structure, doing something completely outside its intended and established purpose (to execute/schedule interrupt handlers). This approach is not going to be accepted, so please set your expectations accordingly.

struct intr_event does center on this task, but this doesn't mean peripheral portions of this task shouldn't be part of struct intr_event. Could you suggest some test for distinguishing whether a given task/field/datum should be on struct intr_event versus what I'm calling interrupt_t (structures intr_irqsrc/intsrc/powerpc_intr)?

Members of struct intr_event should be primarily (with small exceptions) accessed and managed within the kern_event(9) API, that is, kern_event.c. The counters are allocated and incremented outside of this file, even in the current version of your change. This is a good indicator that struct intr_event does not (and should not) own them.

I'm unsure which way you were confused when you typed that. Since find sys/sys sys/kern -type f -print0 | xargs -0 grep -eintr_event -l | sort =>

sys/kern/kern_clock.c
sys/kern/kern_intr.c
sys/kern/subr_intr.c
sys/sys/interrupt.h
sys/sys/intr.h

I think you meant kern_intr.c as struct intr_event is unrelated to kern_event.c. This is a good point, so taking a look at what I've got. Previously non-IPI interrupts had their counters incremented in 3 places, sys/powerpc/powerpc/intr_machdep.c: powerpc_dispatch_intr(), sys/kern/subr_intr.c: intr_isrc_dispatch() and sys/x86/x86/intr_machdep.c: intr_execute_handlers().
Afterwards non-IPI counters are only incremented in sys/kern/kern_intr.c: intr_event_handle(), so by this metric the counters for non-IPI interrupts are perfectly reasonable to have in struct intr_event.

Meanwhile the increments outside of intr_event_handle() and the allocations you're objecting to are strictly for IPI interrupts. This suggests two approaches:

1. IPI interrupts are too distinct from non-IPI interrupts and they need to be severed from each other
2. IPI interrupt handling needs more merging with non-IPI interrupts

That really does explain all the messiness presently in D36610. The two interrupt types have been partially, but not fully merged and the result is a mess. Unfortunately this is at least partially orthogonal to the goal of D36610.
There is certainly a lot of commonality with the two types of interrupt, yet perhaps they really do need to remain separate right now.

In D36610#858879, @mhorne wrote:

In D36610#858610, @mhorne wrote:

After some study, I agree that the intrcnt/intrnames interface is problematic. These counters belong to the machine-dependent interrupt frameworks (e.g. INTRNG), but the sysctls are implemented in the machine-independent intr_event code. This is a layering violation that forces MD code to allocate the counters and names in this inconvenient array format. An acceptable approach would be to eliminate the MI parts by moving (i.e. duplicating) the sysctl and ddb commands into each MD framework*. This will allow the implementations to evolve independently, while still maintaining the hw.intrnames/hw.intrcnt sysctl interface for userspace.

I really dislike the duplication of source code this would cause (also the interface of "hw.intrnames" is surprisingly brittle). Having played with this a bit it seems more useful to have the counters on struct intr_event. An advantage to this approach is stray interrupts which were generated inside an interrupt controller could be recorded, though nominally those shouldn't happen since the interrupt shouldn't be unmasked until after a struct intr_event was allocated.

I'm not absolutely against this approach, just the results of going the opposite direction seem rather superior to me.

Well, then you need to weigh your desire to get this work accepted against your belief of how the code should look. I've given you my major objections.

I hope I've given some good responses to at least some of those objections. I'm also considering the possibilities since the above gives me a good hint as to how I would do things.

In D36610#858879, @mhorne wrote:

In D36610#858610, @mhorne wrote:

Any argument based on saved/wasted memory needs to be backed by some form of before/after measurement in order to be taken seriously.

You seem intent on ignoring this bit.

I'm not. I haven't done proper measurements, so I'm estimating and due to the nature of estimates you're allowed to equate that to a concession. I suspect it will end up in the noise floor on many systems. In the present state there is increased use for IPIs, but decreased use due to removal of all the maintenance values and the intrnames copy.

In D36610#858957, @ehem_freebsd_m5p.com wrote:

I'm unsure which way you were confused when you typed that. Since find sys/sys sys/kern -type f -print0 | xargs -0 grep -eintr_event -l | sort =>

sys/kern/kern_clock.c
sys/kern/kern_intr.c
sys/kern/subr_intr.c
sys/sys/interrupt.h
sys/sys/intr.h

I think you meant kern_intr.c as struct intr_event is unrelated to kern_event.c.

Yes, thanks. I meant kern_intr.c and intr_event(9).

In D36610#859012, @mhorne wrote:

Yes, thanks. I meant kern_intr.c and intr_event(9).

Except this is a non-sequitur as intr_event(9) doesn't relate to anything here.

I was actually hoping for some feedback on what I stated afterwards since I was trying to point in the general direction of the alternative I would tend to prefer.

In D36610#858957, @ehem_freebsd_m5p.com wrote:

I think you meant kern_intr.c as struct intr_event is unrelated to kern_event.c. This is a good point, so taking a look at what I've got. Previously non-IPI interrupts had their counters incremented in 3 places, sys/powerpc/powerpc/intr_machdep.c: powerpc_dispatch_intr(), sys/kern/subr_intr.c: intr_isrc_dispatch() and sys/x86/x86/intr_machdep.c: intr_execute_handlers().
Afterwards non-IPI counters are only incremented in sys/kern/kern_intr.c: intr_event_handle(), so by this metric the counters for non-IPI interrupts are perfectly reasonable to have in struct intr_event.

Meanwhile the increments outside of intr_event_handle() and the allocations you're objecting to are strictly for IPI interrupts. This suggests two approaches:

1. IPI interrupts are too distinct from non-IPI interrupts and they need to be severed from each other
2. IPI interrupt handling needs more merging with non-IPI interrupts

That really does explain all the messiness presently in D36610. The two interrupt types have been partially, but not fully merged and the result is a mess. Unfortunately this is at least partially orthogonal to the goal of D36610.
There is certainly a lot of commonality with the two types of interrupt, yet perhaps they really do need to remain separate right now.

What I presently have in D36610 is more hospitable of approach #2. Allocating struct intr_event for each interrupt which gets reported. If IPI handling could be readily further merged then this is better (unfortunately I'm already looking at too many issues to attack this now).

If you're that much against allocating blank struct intr_events on a temporary basis, then what do you think of approach #1?

The function sysctl_intrnames() in kern_intr.c could be renamed to intr_event_sysctl_intrnames(), made global; and then give sysctl_intrcnt() similar treatment. Then add sysctl_intrnames()/sysctl_intrcnt() implementations to the architectures which report the IPIs from architecture tables and then call intr_event_sysctl_intrnames()/intr_event_sysctl_intrcnt().

This would keep the counters on struct intr_event instead of moving them to the interrupt_t structures. This has the useful advantage of stray software interrupts get reported (there is one presently lurking out there).

Actually, my alternative may simply be removing normal interrupts from intrcnt/intrnames, moving them to struct intr_event and adjusting sysctl_intrcnt()/sysctl_intrnames() to report both locations.

As threatened, this is pushing IPI and non-IPI interrupt implementations further apart. Does in fact provide some nice characteristics, at the cost of reducing commonality.

Note, this requires D36628 as this tickles that bug.

There are a bunch of things here which need review. I'm also wondering whether this needs to be split into several reviews. Problem is all the pieces have so much overlap. I'm also suspicious the commits are likely to need rearrangement too.

I guess the goal of D36610 has moved a bit. What is really needed on this is feedback on the overall approach, then parts can be broken off as things clean up.

Moving the non-IPI interrupt counters to struct intr_isrc really does seem the way to go. ie_fullname isn't going to disappear (several uses in kern_intr.c, neither PowerPC nor x86 were storing them anywhere besides intrnames) so keeping the counters next to it seems worthwhile.

This fully shares them between the architectures instead of creating a poor interface. Crucially unlike 9b33b154b53, this moves them rather than creating duplicate copies.

I'm unsure how many Phabricator Diffs this will need in the end. I'm pretty sure the first few commits/patches will be:

1. kern/intr: replace mutex with shared-exclusive lock
2. kern/intr: add interrupt counters to intr_isrc
3. kern/intr: switch intr_event_handle() to return stray count

After that move sysctl_intrnames()/sysctl_intrcnt() to architecture. Then intr_event_sysctl_intrnames()/intr_event_sysctl_intrcnt(), then nuking the old table handling. I would really like to leave sysctl_intrnames()/sysctl_intrcnt() in better shape, but I doubt I need that to accomplish the goal.

This of course assumes the split is acceptable. I note INTRNG has had issues with IPI versus non-IPI interrupts. In 2016 it did both, but that got broken off. Now IPI handling may be moving back into INTRNG. This does underscore IPI interrupts being quite different from other interrupt types.

I think I've got a viable commit series for this, but I do hope for some feedback before creating 8 Phabricator diffs. As noted, placing the counters onto struct intr_event makes them architecture-independent, but causes them to split from the IPI counters. I think this is reasonable since this is moving the counters, not merely copying them (this is the error made at rG9b33b154b53).

I reviewed the current version and had some comments written up. Overall this approach is a big improvement over allocating empty intr_event structures for IPIs. Treating these distinct types of interrupts differently is sensible, so I was on board with the direction.

However, the vmstat -M core -i use-case poses a real problem for this change. This debugging feature relies on being able to read the counters from intrcnt[] and intrnames[] post-mortem. By scattering I/O interrupt counters elsewhere, it breaks this reporting functionality. Even if said feature is exceedingly niche, we can't just break userspace. It would be possible to collect interrupt counter statistics into some secondary array at panic-time, and have newer vmstat binaries read from there, but at that point I think the added complexity exceeds any tangible benefit.

So, I must say that IMO this is the final nail in the coffin for trying to make this counter array interface nicer. I think it needs to be left alone, and the inconveniences it poses need to be worked around in other ways.

In D36610#874590, @mhorne wrote:

So, I must say that IMO this is the final nail in the coffin for trying to make this counter array interface nicer. I think it needs to be left alone, and the inconveniences it poses need to be worked around in other ways.

To that end I have posted D38437, which should unblock you regarding intr_isrc_deregister().

In D36610#874590, @mhorne wrote:

However, the vmstat -M core -i use-case poses a real problem for this change. This debugging feature relies on being able to read the counters from intrcnt[] and intrnames[] post-mortem. By scattering I/O interrupt counters elsewhere, it breaks this reporting functionality. Even if said feature is exceedingly niche, we can't just break userspace. It would be possible to collect interrupt counter statistics into some secondary array at panic-time, and have newer vmstat binaries read from there, but at that point I think the added complexity exceeds any tangible benefit.

I believe only sysctl("hw.intrnames") and sysctl("hw.intrcnt") are the only supported userspace interface. The use by vmstat is debugging functionality which needs to adapt to kernel changes, rather than trying to preserve kernel data structures to suit unusual userspace programs. Reading the git history of usr.bin/vmstat/vmstat.c is a long list of adjustments to adapt to kernel changes. For this portion in particular fd036deac16 was the last time it was changed and the change was incompatible with earlier. That suggests this is freely changeable, just vmstat needs to be updated.

I believe @jhb or @kib would have better insight. The complexity involved in maintaining this interface suggests to me it is time to go.

Some of this network of commits could be useful without the core change. Check the "Stack" tab for the entire commit network.

I don't know what problem this is trying to solve and definitely don't want to read the entire review, so I have no further comments for the time being.

The end goal is getting INTRNG to be able to release interrupts. The goal of this series is to mostly remove intrcnt/intrnames as they're what cause trouble with releasing interrupts. For that matter the tables themselves are a layering violation and they force the architecture interrupt handling functionality to work around how they were designed.

The alternative D38437 has been proposed, but I'm not quite ready to give up on this approach (those tables are trouble).

This wasn't my goal, but it looks like this series, D38451 in particular fixes the intr_isrc_dispatch() call in sys/arm/mv/gpio.c. Since the counters get moved to the event, it ends up with functioning counters, plus no uninitialized counters get incremented (the function mv_gpio_intr_handler()).