Page MenuHomeFreeBSD

intrng: purge INTR_SOLO
Needs ReviewPublic

Authored by ehem_freebsd_m5p.com on Jun 26 2022, 6:29 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 8, 3:07 PM
Unknown Object (File)
Dec 20 2023, 7:11 AM
Unknown Object (File)
Dec 10 2023, 8:19 PM
Unknown Object (File)
Jun 28 2023, 8:45 PM
Unknown Object (File)
Jun 20 2023, 6:17 PM
Unknown Object (File)
May 6 2023, 9:18 PM
Unknown Object (File)
May 3 2023, 9:29 PM
Unknown Object (File)
Apr 4 2023, 8:18 AM
Subscribers

Details

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.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 53261
Build 50152: arc lint + arc unit

Event Timeline

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.

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.

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 requested changes to this revision.Oct 2 2022, 5:18 PM

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.

This revision now requires changes to proceed.Oct 2 2022, 5:18 PM
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.

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.

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