Page MenuHomeFreeBSD

intrng: allocate event during isrc setup
Needs ReviewPublic

Authored by ehem_freebsd_m5p.com on May 19 2023, 8:12 PM.
Tags
None
Referenced Files
F102160317: D40166.diff
Fri, Nov 8, 8:53 AM
Unknown Object (File)
Fri, Nov 1, 2:22 PM
Unknown Object (File)
Fri, Nov 1, 2:22 PM
Unknown Object (File)
Fri, Nov 1, 2:22 PM
Unknown Object (File)
Fri, Nov 1, 2:22 PM
Unknown Object (File)
Fri, Nov 1, 2:21 PM
Unknown Object (File)
Fri, Nov 1, 2:21 PM
Unknown Object (File)
Fri, Nov 1, 2:11 PM
Subscribers

Details

Reviewers
markj
imp
mw
andrew
Group Reviewers
arm64
Summary

There is minimal benefit in delaying event allocation. Worse, this
reduces opportunities for merging architectural interrupt structures
together.

Diff Detail

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

Event Timeline

Seems pretty reasonable to do lazy allocation of struct intr_event, but there are circumstances where it could be a disadvantage. The other likely allocation time is during the setup step, so using a flag seems appropriate. The question which comes to mind is, which flag value should be used? Should this use a high flag number since this could be discarded after the setup step? Be strictly sequential?

Appears intr_isrc_register()/intr_isrc_deregister() would need to be after isrc_event_create(). I'm unsure whether to move the former down versus the latter up. Right now I'm mostly interested in whether this seems reasonable.

I've been concerned over whether the initial version of D40166 would pass. The use case was too specialized. As such an update, remove the lazy interrupt allocation.

I don't know what benefit lazy interrupt event allocation is supposed to confer. In nearly every case the event will simply happen later. More importantly this allows one to assume all initialized interrupts have an event. No more special case for architecture-agnostic things.

ehem_freebsd_m5p.com retitled this revision from intrng: add flag requesting early event allocation to intrng: allocate event during isrc setup.Aug 22 2023, 1:16 AM
ehem_freebsd_m5p.com edited the summary of this revision. (Show Details)
ehem_freebsd_m5p.com added a reviewer: arm64.

One extra may be needed for this. The file sys/arm/mv/gpio.c has a call to intr_event_create(). Unfortunately this isn't setup for INTRNG allocating events early. Interestingly it seems ad2be10ff043 is exactly the same issue I'm running into.

Move the setting line to isrc_alloc_irq(). This would allow keeping the interrupt number strictly on the event and removing it from intr_irqsrc.

I haven't yet figured out whether sys/arm/mv/gpio.c needs adjustment. Interestingly that file includes workarounds rather similar to my alternative plans.

Squash review, adding removal of ->isrc_irq. This is one reason why allocating the event earlier has value, ->ie_irq can then substitute everywhere. If reviewers want this split back off, that can be done.

Splitting D40166 back up. While I like the second part, it is lower value. This would serve to make all architectures noticably more similar.

sys/kern/subr_intr.c
509

I should note, this is wrong. The author was thinking conflict between "INTR_SOLO" and events, but this could also occur from a race to create the event. This is inappropriate to address as part of D40166, but I notice it is lurking.