intrng: purge INTR_SOLO

[ehem_freebsd_m5p.com]

Summary

The INTR_SOLO feature might have been thought to have potential, but no
actual uses were found. The last semi-active development appears to
have been in 2016, before the release of FreeBSD 11.0. Time to discard
the remains.

[markj]

I'm not qualified to comment on INTR_SOLO. The diff itself looks ok to me. The removal probably warrants some larger discussion on a mailing list, possibly freebsd-arm.

[ehem_freebsd_m5p.com]

This appears to have been an idea for a really small system where needing multiple handlers for a single interrupt was rare. Given how I'm unable to find any evidence of this ever being deployed and their presence serves to cause confusion, I suggest they be completely nuked.

[ehem_freebsd_m5p.com]

Switching to completely removing one comment.

Note, I believe it is correct to replace intr_irq_filter_t with driver_filter_t since those are now identical. I'm not quite certain they're serving the same purpose even though the type is identical.

[mmel]

I see a misunderstanding here.
INTR_SOLO have nothing to do with shared interrupts. It's a fast path for cascading interrupts, and instead of removing it, we should extend it to support pass-through interrupts, polish the API for it, and put it in the regular build. Both of these are common in the arm/aarch64 world, and we lose performance significantly in these cases (and the big question is whether this should help us for LPI interrupts as well).

So if you have time and want to help, please finish it. But unfortunately I can't help you much, due to external conditions I'm only online a few hours a week, usually without the energy to do anything for FreeBSD. And I can't expect it to improve by the end of the year.

[ehem_freebsd_m5p.com]

In D35607#836545, @mmel wrote:

So if you have time and want to help, please finish it. But unfortunately I can't help you much, due to external conditions I'm only online a few hours a week, usually without the energy to do anything for FreeBSD. And I can't expect it to improve by the end of the year.

That is your word choice. My interpretation: "I can't be bothered to finish this feature, but it has huge potential, so please take care of finishing this feature for me! Oh, I'm going to block your patch since it would interfere with the plans I don't have time to implement myself"
No. That is not how this works. Blocking with that sort of justification is an abuse of this system.

In D35607#836545, @mmel wrote:

I see a misunderstanding here.
INTR_SOLO have nothing to do with shared interrupts. It's a fast path for cascading interrupts,
the big question is whether this should help us for LPI interrupts as well).

Insignificant difference. It was intended to be a faster path for a case which is more common on ARM. Yet I see no progress on this approach, and I'm doubtful it has ever been deployed. I think it is time to remove the traces of what may have thought to have potential, but doesn't have enough potential to bother with.

First, the complete list of commits git blame got me:

1. rGad2be10ff043, 2018-04-04 06:12:49: This was attempting to improve the Marvell GPIO driver. From the looks of it, I suspect @mw got fooled into thinking INTR_SOLO was going to be the future. This makes two people whose time was wasted by seeing the remains of an abandoned feature and thinking it might have a future.
2. rGbff6be3e9b2c8, 2016-04-04 09:15:25: Shows up in git blame, but isn't actually development with INTR_SOLO. Simply modifies lines close enough to show up.
3. rG169e6abd8f753, 2016-03-01 10:57:29: This adds some #ifdef's around INTR_SOLO bits. "This option could be unusable very soon, if interrupt controllers implementations will not be implemented considering it." That reads like a suggestion by @skra to remove INTR_SOLO, 6 years ago.
4. rGd6233babae354, 2016-02-10 04:43:08: Another false positive in git blame, simply shows up since things were being moved around. No further development
5. rG2b3ad18853add, 2015-12-18 05:43:59: Yet another false positive in git blame, more moving things around. No further development.
6. rG686450c8984c6, 2015-10-18 18:26:19: The original commit of INTRNG. I think the author thought INTR_SOLO had some potential, but didn't think it had enough to unconditionally enable.

That is everything. If you could provide evidence of genuine interest in the INTR_SOLO feature somewhere I would be okay with abandoning this commit. Otherwise, the continued existence of this abandoned feature is causing confusion and it is overdue for removal.

[ehem_freebsd_m5p.com]

Hmm, spotted this a while ago, but didn't get Phabricator updated. The test in isrc_event_create() was wrong, this appears to be the correct action.

This situation could occur if multiple threads entered isrc_event_create() at the same time. If this occured the caller wants the event to exist and doesn't care whether a race occurred.

[ehem_freebsd_m5p.com]

Hmm, no that isn't really so elegant so keep to the smaller diff.

[ehem_freebsd_m5p.com]

In D35607#836545, @mmel wrote:

INTR_SOLO have nothing to do with shared interrupts. It's a fast path for cascading interrupts, and instead of removing it, we should extend it to support pass-through interrupts, polish the API for it, and put it in the regular build. Both of these are common in the arm/aarch64 world, and we lose performance significantly in these cases (and the big question is whether this should help us for LPI interrupts as well).

I am not 100% certain the PIC I'm looking at is what you consider a "cascading interrupt". Does match that description though. Mainly it is a PIC connected to the root via a single PPI and fans out interrupts. My test machine fans out to 6 interrupts, but the PIC can fan out to more than 4000 interrupts (the next version may do more).

The value of INTR_SOLO seems very dubious to me. My first concern is I could become the only consumer of INTR_SOLO, then you hand in your commit bit and 2 days later INTR_SOLO is scheduled for removal.

Next, this seems aimed at being used for the interrupt feeding into the fanout PIC. Problem here is this makes the input interrupt cheaper at the expense of the output interrupts; notably memory consumption and speed. If the fanout is 8, then the avoided struct intr_handler is already less than the cumulative size added to struct intr_irqsrc. Then comes the question of when the input triggers, how many outputs are triggered? If typically a single input equates to two output events, then the reduced performance on outputs has already overwhelmed the savings on the more common input.

INTR_SOLO is pretty well anathema to merging the interrupt frameworks. It gives extra performance to those interrupts by violating the layering. The most likely merge strategy is to split the layers apart into smaller pieces and see whether any pieces can be shared.

The problematic characteristics of pic_init_secondary() (D40474) are a rather greater problem to me than theoretical gains from INTR_SOLO.