Re: [PATCH] Remove if_watchdog use

On Tuesday 10 November 2009 4:45:03 pm Gavin Atkinson wrote:
> On Fri, 6 Nov 2009, John Baldwin wrote:
> 
> > I have a patchset that converts all the remaining users of if_watchdog to
> > using a private callout instead.  In some cases the the driver already used a
> > private timer to drive a stats timer and I merely hooked into that timer.  In
> > other cases a new callout needed to be added to the driver.  Some drivers
> > even abused the if_watchdog interface to provide a stats timer that fired
> > every second. :)  For a few drivers I also fixed other things such as busted
> > locking, order-of-operations issues in detach, or just completely busted
> > drivers (fea(4) and fpa(4) which share the pdq backend).  Please test.
> > Barring any major screaming and shouting I plan to commit this in a week or
> > so and after that to work on removing the if_watchdog/if_timer stuff from the
> > network stack.
> >
> > The patch is at http://www.FreeBSD.org/~jhb/patches/cleanup.patch
> >
> > Driver details:
> > - an(4)
> >  - Locking fixes to not do silly things like drop the lock only to call a
> >    function that immediately reacquires the lock.  Also removes recursive
> >    locking.
> >  - Hooks into the stat timer to drive the watchdog timer.
> 
> I managed to get a panic when running wpa_supplicant:
> 
> System call ioctl returning with the following locks held:
> exclusive sleep mutex an0 (network driver) r=0 (0xc58fc180) locked _at_
> /usr/src/sys/dev/an/if_an.c:2341
> panic: witness_warn
> 
> This seems to fix that:
> 
> --- /usr/src/sys/dev/an/if_an.c.orig	2009-11-10 19:26:21.000000000 
> +0000
> +++ /usr/src/sys/dev/an/if_an.c	2009-11-10 19:27:24.000000000 +0000
> _at__at_ -2570,6 +2570,9 _at__at_
>   			an_setdef(sc, &sc->areq);
>   			AN_UNLOCK(sc);
>   			break;
> +		default:
> +			AN_UNLOCK(sc);
> +			break;
>   		}
> 
>   		/*

Ok, thanks.  Sadly the ioctl handling probably needs a bit more work since it
calls copyin() while holding the an(4) mutex, but I will leave that for
another day.

> I also get the following LOR on unplug (but see it before your patch too):
> 
> lock order reversal:
> 1st 0xc50f5208 if_afdata (if_afdata) _at_ /usr/src/sys/net/if.c:912
> 2nd 0xc0f9db68 mld_mtx (mld_mtx) _at_ /usr/src/sys/netinet6/mld6.c:569

I think this has to do with the stack in general and is not specific to an(4).

> Other than that, the patches to an(4) seem to work as well before your 
> patch as after, with light TCP traffic and heavy ping traffic.  However, 
> an(4) appears to require a lot of work (unrelated to your patch) to bring 
> it up to the state of other wireless drivers.

Thanks, I will commit the an(4) bits in a bit.

> > - ixgb(4)
> >  - Uses callout_init_mtx() instead of callout_init(..., CALLOUT_MPSAFE).
> >  - Remove silly callout handling in a few places (cancelling the callout
> >    only to rescheduled it again immediately afterwards).
> >  - Hooks into the stat timer to drive the watchdog timer.
> 
> These changes to ixgb_detach() don't compile as ifp is no longer used in 
> the !DEVICE_POLLING case.
> 
> /usr/src/sys/dev/ixgb/if_ixgb.c: In function 'ixgb_detach':
> /usr/src/sys/dev/ixgb/if_ixgb.c:369: warning: unused variable 'ifp'

Oops, ixgb isn't in LINT so I keep missing it.  I need to just add it to
NOTES.  Thanks.

-- 
John Baldwin