cvs commit: src/sys/dev/fxp if_fxp.c if_fxpvar.h

[John Baldwin]

On 29-Apr-2003 Andrew Gallatin wrote:
>  > Where would you do this check for more pending handlers?  In detach?
>  > What does detach do when it's true?  The problem is that detach may
> 
> It sleeps.
> 
>  > be too late.  Take this sequence:
>  > 
>  > 
>  >         CPU 0           CPU 1
>  >         --------        --------
>  >         foo_detach()    ...
>  >         FOO_LOCK()      interrupt for foo
>  >         ...             foo_intr()
>  >         ...             FOO_LOCK() and blocks
>  
> You forgot:
>          prevent_device_from_generating_interrupts();
>          FOO_UNLOCK();
> 
>  >         bus_teardown_intr()

This doesn't do any good though.  When you FOO_UNLOCK, CPU 1 is
_STILL_ going to resume execution of foo_intr().  Depending on if
CPU 1 gets interrupted some more before it finishes foo_intr(), CPU 0
can finish all of detach before CPU 1 finishes foo_intr().  How do
you handle that?

[ Reading on it seems you advocate the sleep in detach to ensure
  you don't go beyond bus_teardown_intr() in detach(). ]

> In foo_detach you disable interrupts on the board, and you replace its
> handler with an orphan handler for the benefit of shared irqs.  At the
> point there can be exactly one interrupt pending which may be blocked
> anywere, possibly on the foo lock.  You then drop the foo lock.
> 
> You then grab the required locks, and see if foo's ithread is running,
> or runnable.  If it is, you drop the locks, sleep for hz and repeat
> the check.   If its not, you do the guts of ithread remove handler.

Your orphan handler doesn't buy you anything.  Due to IH_DEAD you don't have
to worry about that step, IH_DEAD already ensures that you only have the
one race case we are talking about to deal with.  Basically though, you
are moving the sleep from inside bus_teardown_intr() out into each
driver, increasing the maintenance load on the driver author.  Also,
you still haven't solved the problem that it may be _very_ bad to touch
foo's registers in foo_intr() after you've gotten into foo_detach() since
it could re-enable interrupts, etc.  You might be able to work around
that on a case-by-case basis by tweaking the order of shutdown, but that
makes it harder to write drivers.  Also, it's only a win if you avoid a
test for the special case in foo_intr().  Hmmmm.  You almost want a way
to turn off the interrupt handler as the very first thing (maybe just mark
it as dead if it is still running, but not block) while holding the driver
lock, then turn off the ability to generate interrupts, then sit and wait
for the interrupt handler to remove itself.  Maybe:


        mtx_lock(&sc->sc_mtx);
        /* handler no longer executes */
        bus_disable_intr(inthand_cookie, &sc->sc_mtx);
        /* dink with device */
        mtx_unlock(&sc->sc_mtx);
        bus_teardown_intr(inthande_cookie);

With bus_disable_intr() calling a function like this:

void
ithread_disable_handler(void *cookie, struct mtx *m)
{
        struct intrhand *ih;
        struct ithread *it;

        ih = (struct intrhand *)cookie;
        it = ih->ih_it;
        mtx_lock(&it->it_lock);
        if (ithread_is_running) {
                it->it_need = 1;
                ih->ih_flags |= IH_DISABLING;
                ih->ih_mutex = m;
                mtx_unlock(&it->it_lock);
                msleep(ih, m, ...);
        } else {
                ih->ih_flags |= IH_DISABLED;
                mtx_unlock(&it->it_lock);
        }
}

and some changes to ithread_loop() along the lines of:

                        CTR6(... /* existing */)
                        if (ih->ih_flags & IH_DISABLED)
                                continue;
                        if (ih->ih_flags & IH_DISABLING) {
                                struct mtx *m;

                                m = ih->ih_mutex;
                                ih->ih_flags |= IH_DISABLED;
                                ih->ih_flags &= ~IH_DISABLING;
                                mtx_unlock(&it->it_lock);
                                mtx_lock(&m);
                                wakeup(ih);
                                mtx_unlock(&m);
                                goto restart;
                        }
                        if (ih->ih_flags & IH_DEAD) {
                                TAILQ_REMOVE(...);
                                wakeup(ih);
                                continue;
                        }

I would need to hold the ithread lock around the TAILQ_FOREACH()
bit and drop it around the execution of the handlers, but I probably
need that anyways.  Of course, all this really does, is take your
gone test and move it up in the stack.  You are still checking a flag
on every interrupt handler invocation.

>  > > We really need to think about efficiency.  Our 5.x performance sucks.
>  > > Really sucks.  We're being nickled and dimed to death by extra
>  > > instructions here, there, and everywhere.
>  > 
>  > Unfortunately 5.x attempts to run with a thread-safe kernel, and that
>  > involves extra overhead to work around races that 4.x didn't even have
>  > to dream about.  In theory the increased performance should come from
>  > increased parallelism at the cost of increased overhead.  If FreeBSD
> 
> No, in theory increased performance should come from increased
> parallelism with no increased overhead.  Any increased overhead is a
> bug.  Linux 2.4 runs a thread safe kernel with less overhead than we
> have in 4.x.  Its possible.

How are we going to achieve increased paralellism w/o increased overhead?
Discounting redesigning algo's which would have been a win in the
non-parallel kernel as well.  A mutex is far more expensive than an spl.
You have to protect against more things.  Of course overhead is going to
go up.

> As we get closer to a stable branchpoing, and continue to suck, I'm
> starting to think we should start over.

Well, the Project is free to choose that if it wishes.

-- 

John Baldwin <jhb at FreeBSD.org>  <><  http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve!"  -  http://www.FreeBSD.org/