cvs commit: src/sys/netinet ip_divert.c

[Andre Oppermann]

Robert Watson wrote:
> 
> On Tue, 19 Oct 2004, Andre Oppermann wrote:
> 
> > >   Annotate a newly introduced race present due to the unloading of
> > >   protocols: it is possible for sockets to be created and attached
> > >   to the divert protocol between the test for sockets present and
> > >   successful unload of the registration handler.  We will need to
> > >   explore more mature APIs for unregistering the protocol and then
> > >   draining consumers, or an atomic test-and-unregister mechanism.
> >
> > Thanks.
> >
> > I'll commit a little extended locking of that section in a few minutes.
> >
> > Is it possible for someone else to spin on the lock while we are holding
> > it?  If yes, it would be impossible to destroy the lock on the next
> > line.  The same problem is then resident in ipfw unloading as well.
> 
> Yes.  In fact, that's precisely what adaptive mutexes do, as our context
> switch time appears to be more expensive than to spin in most cases.
> 
> We have started to address this and related problems in a couple of ways
> in a couple of places -- for example, with the MAC Framework, we count the
> number of threads that have entered the framework and wait until all
> threads drain before unloading (potentially starving the unloader).  In
> PFIL, we drop packets that try to enter while a module is waiting for
> threads to drain so it can unload, avoiding the starvation problem.  I
> believe Poul-Henning has also been working along these lines in the device
> driver code so that during unload there's an atomic decision point such
> that we stop letting new threads in once the decision to unload has been
> finalized.  These models appear to be easier to implement than atomic "are
> we busy or unload" tests, and by putting part of the decision logic in the
> framework we avoid impossible-to-address races since we can quench the
> flow of calls into the module (avoiding "unload while the instruction
> pointer is in a function testing to see if it can be there").

Hmm... I'll take a look at those attempts and see what I can come up
with to get some general solution for the protocol cases.  The approach
of callout_drain() looks promising though.

> As you point out, we don't have this problem a whole lot right now since
> load and unload events are pretty infrequent, but it would be nice to
> avoid adding new APIs that aren't designed to handle safe unload, in as
> much as is reasonable.  For my own part, I'd be willing to simply say "you
> can't unload protocols once you've loaded them" because it simplifies the
> assumptions a great deal, but I recognize that that limits the usefulness
> of loadable protocols for the purposes of a rapid development cycle (load,
> unload, load, unload, ...)

Exactly.  I want to leave this decision with the module writer.  You can
always get this semantics simply with returning EBUSY on unload attempts.
And depending on what you protocol module does the race can be so small
that chances to get hit by it are so low that it almost doesn't matter.

-- 
Andre