Date: Sun, 04 Mar 2001 15:11:11 -0800 (PST) From: John Baldwin <jhb@FreeBSD.org> To: "Justin T. Gibbs" <gibbs@scsiguy.com> Cc: cvs-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/kern kern_intr.c src/sys/sys interrupt.h Message-ID: <XFMail.010304151111.jhb@FreeBSD.org> In-Reply-To: <200103042237.f24MbfO84003@aslan.scsiguy.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 04-Mar-01 Justin T. Gibbs wrote:
>>
>>I'd rather have the code not care either way to make it easier on driver
>>writers if need be. I do agree that drivers probably shouldn't be holding
>>locks while removing handlers. Note that pccard does make a lot of the cases
>>more extreme as the hardware can leave at any time.
>
> I don't think you can do this and still give the deregister method the
> semantics that on return your handler is not and can not ever execute
> again.
Yes, currently your handler might be currently executing though it won't do so
in the future once that one completes...
> We're talking past each other. Forget what your code does for a moment.
> Instead
> of removing the entry directly, you simply change the handler to a function
> that
> will wake you up after removing the entry. The entry is then always removed
> from
> the ithread context.
Or I can just have the removal thread tsleep on the intrhand address and do a
wakeup on that in the ithread if needed. Just changing the function we call is
more tricky because the ithread needs to know that the list has changed and
that it should restart from the beginning of the list to avoid walking through
free'd memory.
>>while it is executing foo() and while it is blocked another thread removes
>>foo() from the list, what possible good can changing the functino pointer
>>do to change the fact that the ithread is executing foo()?
>
> It seemed the easiest way to register state about who would be woken up as
> the entry is really and truely killed off. If you want to use a different
> method, so be it, but whatever it is, it must allow mutiple, concurrent
> deregistrations.
The multiple, concurrent bit already works. If we set the bit again it stays
set. However, having multiple CPU's removing the same exact handler may not
work very well if the handler is free'd by the ithread while we are walking
back up the new-bus heirarchy. Calling bus_teardown_intr() on the same cookie
multiple times would be a Bad Thing(tm). I think it was in the old interrupt
code as well.
>>Also, since I set it_need, I _do_ force
>>the ithread to loop again to purge the item from the list. I guess
>>your new change to this is you want to sleep on the intrhand until the
>>ithread runs to release it? I don't see why that is needed. Once the
>>intrhand is dead, we don't dereference the function pointer again, so
>>we aren't in danger of calling the function on a free'd softc, etc.
>
> Rather you are in danger of freeing the softc just before the interrupt
> handler is run. This will always be the case in your scheme unless you
> hold a lock across the call to the interrupt handler and that lock is
> held by the interrupt deregistration method to close the race. There
> may also be cases in the interrupt handler where you release your
> softc lock. If, during that window, the detach method decides to
> destroy your driver mutext or otherwise change the state of the softc
> (hey, I've dregistered my interrupt handler, so who else could need
> this any more?), you are hosed.
You can't free it before it is run, but you can free it while it is already
running. A tsleep/wakeup pair between ithread_remove_handler() and
ithread_loop() can be used to work around this, however. Untested patch
included below.
>>The only bad case is if the function is already running, and changing
>>the function pointer doesn't do anything to help with that.
>
> Blocking is what fixes it and by storing *some state* in the handler
> entry itself, you can handle more than one waiter for different
> deregistrations at the same time.
The IH_DEAD flag is the state in question. However, one thing I may play with
is just locking the list with a mutex at some point in time, as it may not end
up being all that expensivem, esp. since it would almost always be
non-contested. Not sure about this yet, though.
Index: kern/kern_intr.c
===================================================================
RCS file: /usr/cvs/src/sys/kern/kern_intr.c,v
retrieving revision 1.49
diff -u -r1.49 kern_intr.c
--- kern/kern_intr.c 2001/03/02 06:07:38 1.49
+++ kern/kern_intr.c 2001/03/04 23:07:22
@@ -316,15 +316,16 @@
* it on the list.
*/
ithread->it_need = 1;
+ mtx_unlock_spin(&sched_lock);
+ mtx_unlock_spin(&ithread_list_lock);
+ tsleep(handler, PUSER, "itrmh", 0);
} else {
+ mtx_unlock_spin(&sched_lock);
TAILQ_REMOVE(&ithread->it_handlers, handler, ih_next);
ithread_update(ithread);
- }
- mtx_unlock_spin(&sched_lock);
- mtx_unlock_spin(&ithread_list_lock);
-
- if ((handler->ih_flags & IH_DEAD) == 0)
+ mtx_unlock_spin(&ithread_list_lock);
free(handler, M_ITHREAD);
+ }
return (0);
}
@@ -507,6 +508,7 @@
ih_next);
ithread_update(ithd);
mtx_unlock_spin(&ithread_list_lock);
+ wakeup(ih);
if (!mtx_owned(&Giant))
mtx_lock(&Giant);
free(ih, M_ITHREAD);
--
John Baldwin <jhb@FreeBSD.org> -- http://www.FreeBSD.org/~jhb/
PGP Key: http://www.baldwin.cx/~john/pgpkey.asc
"Power Users Use the Power to Serve!" - http://www.FreeBSD.org/
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe cvs-all" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?XFMail.010304151111.jhb>