cvs commit: src/sys/dev/an if_an.c src/sys/dev/arl if_arl_isa.c src/sys/dev/awi if_awi_pccard.c src/sys/dev/cm if_cm_isa.c src/sys/dev/cnw if_cnw.c src/sys/dev/cp if_cp.c src/sys/dev/cs if_cs.c src/sys/dev/ed if_ed.c src/sys/dev/em if_em.c ...

[John Baldwin]

On Wednesday 21 September 2005 11:41 am, Ruslan Ermilov wrote:
> On Tue, Sep 20, 2005 at 10:33:43PM +0100, Robert Watson wrote:
> > On Tue, 20 Sep 2005, Robert Watson wrote:
> > >On Tue, 20 Sep 2005, John Baldwin wrote:
> > >>Regarding other comments I saw today on some e-mail or another, I do
> > >>think that to make the locking sane, we might should push the checks
> > >> for IFF_DRV_RUNNING down into the foo_start() routines rather than
> > >> doing it in the network layer where the driver lock isn't held.
> > >
> > >This was a change I was thinking of trying to get into 6.0 a few weeks
> > >ago, but was worried that wholesale frobbing of the network interface
> > >drivers would introduce too many bugs.  Also, it will increase the cost
> > >of injecting packets into the send queue under load as you'll always
> > >have to acquire and drop the driver mutex to test the flag.  I.e., it's
> > >not clear we're actually racing, but we might pay a hefty cost for
> > >fixing it.  If you want to take a cut at it, I'm happy to help
> > >characterize the cost and decide if it's the right thing to do.  It
> > >would be nice to get it into 6.0 if possible as it will become part of
> > >the device driver API if so.
> >
> > ... getting late ...
> >
> > I mean the IFF_DRV_OACTIVE flag test used in the handoff.
>
> Here's what I'd like to do, if nobody objects:
>
> 1.  Finish the if_free() move after bus_teardown_intr() that Warner
>     has started.  This is needed to fix foo_intr() accessing already
>     freed struct ifnet.
>
> 2.  Fix all drivers to check IFF_DRV_RUNNING in foo_intr() and exit
>     if it's unset.  This should fix some of the shutdown panics
>     when shared IRQs are in use.
>
> 3.  Fix all drivers to check IFF_DRV_RUNNING in foo_start() and exit
>     if it's unset.  This should fix another half of shutdown panics,
>     e.g. the one demonstrated by glebius@ in recent if_em.c commit.
>
> 4.  Remove IFF_DRV_RUNNING check from ether_output().
>
> 5.  Fix all drivers to set some flag in foo_detach() and foo_shutdown()
>     and refuse to work in foo_ioctl() if it's set.  This should fix
>     panics when BPF listener is attached while interface goes away or
>     module is unloaded.
>
> Relevant PRs: kern/85005, kern/62889.
>
> Attached is a demo patch for rl(4) that does all of the above
> except #4.

I think you can leave #4 in if the race doesn't hurt anything.  3) already 
handles the condition you are worried about.  I'd rather 5) be simpler in 
that it only check in the flags case to not force the driver lock to be 
acquired for all the ioctls that the driver doesn't actually handle.  
Actually, I think I'd really prefer that we think about how to fix the BPF 
issue in BPF itself if possible.  It may be that we don't need to set the 
flags (i.e. skip the actual ioctl) if the interface is in the process of 
detaching and we can make that change centrally without having to scatter 
gone flags in all the drivers.

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