Details

Reviewers

gallatin
jacob.e.keller_intel.com
erj
marius

Commits

rS344471: MFC r344162:
rS344162: iflib: Improve return values of interrupt handlers.

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 - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 22514
Build 21661: arc lint + arc unit

Event Timeline

shurd created this revision.Feb 14 2019, 7:36 PM

Herald added subscribers: ae, imp. · View Herald TranscriptFeb 14 2019, 7:36 PM

Harbormaster completed remote builds in B22506: Diff 53931.Feb 14 2019, 7:36 PM

Remove unrelated stuff.

Harbormaster completed remote builds in B22507: Diff 53932.Feb 14 2019, 7:36 PM

Unpack the conditional for easier reading.

Harbormaster completed remote builds in B22508: Diff 53933.Feb 14 2019, 7:42 PM

Put the result variable back in. *facepalm*

Harbormaster completed remote builds in B22510: Diff 53935.Feb 14 2019, 7:49 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.

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

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

Harbormaster completed remote builds in B22511: Diff 53940.Feb 14 2019, 9:24 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... :)

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

Harbormaster completed remote builds in B22512: Diff 53941.Feb 14 2019, 9:56 PM

marius requested changes to this revision.Feb 14 2019, 10:04 PM

marius added inline comments.

sys/net/iflib.c
1480	"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.