Re: FILTER_SCHEDULE_THREAD is not a bit-value

On Tue, 2012-01-31 at 15:57 +0700, Max Khon wrote: 
> Hello,
> 
> On Tue, Jan 31, 2012 at 12:44 AM, Ian Lepore
> <freebsd_at_damnhippie.dyndns.org> wrote:
> 
> >> sys/bus.h documents the following semantics for FILTER_SCHEDULE_THREAD:
> >>
> >> /**
> >>  * _at_brief Driver interrupt filter return values
> >>  *
> >>  * If a driver provides an interrupt filter routine it must return an
> >>  * integer consisting of oring together zero or more of the following
> >>                                  ^^^^^^^
> >>  * flags:
> >>  *
> >>  *      FILTER_STRAY    - this device did not trigger the interrupt
> >>  *      FILTER_HANDLED  - the interrupt has been fully handled and can be EOId
> >>  *      FILTER_SCHEDULE_THREAD - the threaded interrupt handler should be
> >>  *                        scheduled to execute
> >>  *
> >>  * If the driver does not provide a filter, then the interrupt code will
> >>  * act is if the filter had returned FILTER_SCHEDULE_THREAD.  Note that it
> >>  * is illegal to specify any other flag with FILTER_STRAY and that it is
> >>  * illegal to not specify either of FILTER_HANDLED or FILTER_SCHEDULE_THREAD
> >>  * if FILTER_STRAY is not specified.
> >>  */
> >> #define FILTER_STRAY            0x01
> >> #define FILTER_HANDLED          0x02
> >> #define FILTER_SCHEDULE_THREAD  0x04
> >>
> >> But actually FILTER_SCHEDULE_THREAD is not used as a bit-value (see
> >> kern/kern_intr.c):
> >>
> >>                 if (!thread) {
> >>                         if (ret == FILTER_SCHEDULE_THREAD)
> >>                                 thread = 1;
> >>                 }
> >>
> >> There is at least one in-tree driver that could be broken because of
> >> this (asmc(8), but I found the problem with some other out-of-tree
> >> driver).
> >> This should be "if (ret & FILTER_SCHEDULE_THREAD)" instead. Attached
> >> patch fixes the problem.
> >>
> >> What do you think?
> >>
> >> Max
> >
> > I think returning (FILTER_HANDLED | FILTER_SCHEDULE_THREAD) makes no
> > sense given the definition "the interrupt has been fully handled and can
> > be EOId".  If you EOI in the primary interrupt context and then schedule
> > a threaded handler to run as well you're likely to need complex locking
> > between the primary and threaded interrupt handlers and I was under the
> > impression that's just the sort of thing the filter/threaded scheme was
> > designed to avoid.
> 
> I see no sense here.
> 1) You would have to implement locking anyway to protect concurrent
> access from ithread/filter and other driver methods (char device or
> network device callbacks)
> 

That is often, but not always, the case.  Depending on the hardware and
the needs of the driver, the guaranteed temporal separation between
primary and threaded interrupt context can reduce the need for locking.
In one case I managed to avoid the need to do any locking at all in the
primary context (in a pps driver to replace the stock one that lost the
ability to handle interrupts in a primary context at all).

> 2) ithread and filter can already be executed simultaneously even when
> only FILTER_SCHEDULE_THREAD is returned: when ithread is scheduled to
> be executed the device can emit a new interrupt and it will be
> preempted by filter
> 

No, if the primary-context handler does not return FILTER_HANDLED and
the interrupt dispatcher code does not EOI the interrupt until after the
threaded handler has run, then another hardware interrupt from that
source cannot interrupt the threaded handler.  This amounts to implicit
temporal synchronization between primary and threaded interrupt contexts
that eliminates the need for explicit synchronization using locks.

> > In other words, the part about ORing together values seems to be staking
> > out room for future growth, because the current set of flags and the
> > words about how to use them imply that only one of the current set of
> > values should be returned at once.
> 
> No, the text does not imply that only one of the values is supposed to
> be returned (where did you see it). See also KASSERT checks in
> intr_event_handle() -- they clearly show that the intention was to
> allow FILTER_HANDLED and FILTER_SCHEDULE_THREAD to be returned
> simultaneously.
> 
> Max

I have to admit that the text doesn't specifically forbid returning both
values ORed together, but it seems to me that doing so is nonsensical.
The reason is the corollary to the above point:  if you return
FILTER_HANDLED and allow the interrupt to be EOI'd from the primary
context, then you have to deal with the possibility of being
re-interrupted in the threaded handler.  You have to cope with the
possiblity of getting a new interrupt before having fully handled the
prior one.  

Hmmm, but I guess I could imagine a situation where the primary context
could capture and enqueue information on a list and then return
FILTER_HANDLED specifically to request the EOI so that having multiple
hardware interrupts occur between invocations of the threaded handler
would work out okay.  I wonder if that situation happens does the thread
get invoked as many times as the primary context ran and returned
FILTER_SCHEDULE_THREAD, or does it get invoked just once and has to
handle finishing up the work for any number of previous primary-context
handler invokations?  (Yeah, I could look at the code, but I really need
to wrap up my morning mail and get on with my paying-job work. :)

Heh, looks like I talked myself into the idea that ORing together the
two values might be useful for some drivers.

-- Ian