Page MenuHomeFreeBSD

iflib: Return the correct filter result in the interrupt handler.
ClosedPublic

Authored by shurd on Feb 14 2019, 7:36 PM.

Details

Summary

iflib was returning FILTER_HANDLED, in cases where FILTER_STRAY
or FILTER_SCHEDULE_THREAD was more correct. This potentially caused
issues with shared legacy interrupts.

Obbtained-from: Haiku (a84bb9, 4947d1)
Submitted-by: Augustin Cavalier <waddlesplash@gmail.com>

https://git.haiku-os.org/haiku/commit/?id=a84bb9
https://git.haiku-os.org/haiku/commit/?id=4947d1

Test Plan

Get an em device in legacy mode sharing an interrupt and test.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

shurd created this revision.Feb 14 2019, 7:36 PM
shurd updated this revision to Diff 53932.Feb 14 2019, 7:36 PM

Remove unrelated stuff.

shurd updated this revision to Diff 53933.Feb 14 2019, 7:42 PM

Unpack the conditional for easier reading.

shurd updated this revision to Diff 53935.Feb 14 2019, 7:49 PM

Put the result variable back in. *facepalm*

gallatin added a comment.EditedFeb 14 2019, 8:21 PM

I understand the missing FILTER_STRAY.

However, I'm confused about why you are returning FILTER_SCHEDULE_THREAD now, rather than FILTER_HANDLED. Iflib always schedules a gtaskq, so it should never need that. My understanding is that for shared interrupts, the other filters will run and it is their job to start a thread if their device has work. (or if there is no filter for a device, a ithread is always launched). It seems like this patch may cause unneeded scheduling of ithreads for devices sharing legacy irq's with iflib devices.

marius requested changes to this revision.Feb 14 2019, 9:09 PM

I second Andrew; using FILTER_STRAY in case of !iflib_started is a good move,
but returning FILTER_SCHEDULE_THREAD from the filter registered via
bus_setup_intr(9) is wrong at least when done unconditionally. The latter
signals intr_event_handle() to schedule an ithread for running the handler
registered via bus_setup_intr(9), but currently neither iflib(4) nor its
consumers appear to actually register a handler. The better change in that
regard probably is to remove support for handlers from _iflib_irq_alloc() and
iflib_irq_alloc() as the approach taken by iflib(4) apparently is to schedule a
gtask by itself (rather than letting intr_event_handle() schedule an ithread) if
there's no ifi filter or the ifi filter doesn't fully handle a particular interrupt,
i. e. the ifi filter returns the somewhat abused FILTER_SCHEDULE_THREAD.
Maybe iflib_irq_alloc() should be even removed completely as its currently
unused and if a driver wants to do something fancy with an interrupt outside
of the design of iflib(4), that driver is better off setting up that interrupt entirely
by itself.

I suspect the problem Haiku is running into is that their emulation of FreeBSD
device interrupt handling doesn't properly deal with the case of a filter and a
handler being registered for the same shared interrupt by different drivers.
Maybe that's not properly handled in FreeBSD either, but then this is something
to be fixed in kern_intr.c and not to be worked around in iflib(4).

This revision now requires changes to proceed.Feb 14 2019, 9:09 PM
shurd updated this revision to Diff 53940.Feb 14 2019, 9:24 PM

Some bits of the upstream patch didn't paply cleanly... added manually.

marius requested changes to this revision.Feb 14 2019, 9:43 PM

Returning FILTER_SCHEDULE_THREAD from iflib_fast_intr_{ctx,rxtx}()
is still wrong as iflib_irq_alloc_generic() doesn't register a handler that could
be scheduled.

This revision now requires changes to proceed.Feb 14 2019, 9:43 PM

Hi, I'm the Haiku developer who authored these patches originally. I currently maintain the FreeBSD compatibility layer for Haiku (I "inherited" it last year.)

For context: Haiku has no concept of "ithreads." All our interrupt handlers are essentially filters, and any (native) drivers that want to do processing outside of interrupt context must create their own thread, or use a system daemon thread. So there are 3 possible return values for the interrupt handlers on Haiku also: B_HANDLED_INTERRUPT, B_UNHANDLED_INTERRUPT, or B_INVOKE_SCHEDULER. The first two behave exactly as they do on FreeBSD, the last one does not: we don't invoke the thread scheduler after every interrupt unless asked to do so explicitly by a driver, and that's what this value does.

Whoever wrote the compat layer's interrupt handling originally (long before me, probably 2007/2008 or so) just #defined the three to the FreeBSD ones and called it a day, it seems. When investigating the "hangs/interrupt storm with iflib-based e1000" ticket, I just guessed FILTER_SCHEDULE_THREAD was the correct thing to do based on em's usage, indeed. (Looking through our tree, it appears that asides from iflib, nothing even used FILTER_SCHEDULE_THREAD.) So indeed, it appears that using FILTER_HANDLED is correct by the FreeBSD API, and I will have to just add an #ifdef HAIKU_ to return B_INVOKE_SCHEDULER instead in the relevant locations.

I suspect the problem Haiku is running into is that their emulation of FreeBSD
device interrupt handling doesn't properly deal with the case of a filter and a
handler being registered for the same shared interrupt by different drivers.

Indeed we don't, and it would be a few days' work to support that, but indeed it seems nothing uses this so for now I'll leave it be.

Maybe that's not properly handled in FreeBSD either, but then this is something
to be fixed in kern_intr.c and not to be worked around in iflib(4).

Well, I now see the comment in bus.h indicates the behavior should be as you describe: http://xref.plausible.coop/source/xref/freebsd-current/sys/sys/bus.h#207

So I would be surprised if it didn't work for anything at all... :)

shurd updated this revision to Diff 53941.Feb 14 2019, 9:56 PM

Return FILTER_HANDLED rather than FILTER_SCHEDULE_THREAD when gtask (potentially) scheduled
per @waddlesplash_gmail.com.

marius requested changes to this revision.Feb 14 2019, 10:04 PM
marius added inline comments.
sys/net/iflib.c
1480 ↗(On Diff #53941)

"Values in return statements should be enclosed in parentheses" according to style(9),
same for other occurrences of "return result".
Apart from that this patch looks good now, thanks.

This revision now requires changes to proceed.Feb 14 2019, 10:04 PM
shurd updated this revision to Diff 53942.Feb 14 2019, 10:08 PM

FILTER_SCHEDULE_THREAD is a flag, not a discrete value. If it's set, schedule the
gtask and return FILTER_HANDLED.

shurd updated this revision to Diff 53943.Feb 14 2019, 10:10 PM

style(9) nits.

shurd marked an inline comment as done.Feb 14 2019, 10:11 PM
marius accepted this revision.Feb 14 2019, 11:15 PM
This revision is now accepted and ready to land.Feb 14 2019, 11:15 PM
gallatin accepted this revision.Feb 15 2019, 12:52 PM
sbruno removed a subscriber: sbruno.Feb 15 2019, 12:55 PM
This revision was automatically updated to reflect the committed changes.