From owner-freebsd-net@FreeBSD.ORG Tue Nov 10 22:03:04 2009 Return-Path: Delivered-To: net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 16BDB1065693; Tue, 10 Nov 2009 22:03:04 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id CA37A8FC1B; Tue, 10 Nov 2009 22:03:03 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id 6396746B23; Tue, 10 Nov 2009 17:03:03 -0500 (EST) Received: from jhbbsd.hudson-trading.com (unknown [209.249.190.8]) by bigwig.baldwin.cx (Postfix) with ESMTPA id 926178A01B; Tue, 10 Nov 2009 17:03:02 -0500 (EST) From: John Baldwin To: Gavin Atkinson Date: Tue, 10 Nov 2009 17:02:57 -0500 User-Agent: KMail/1.9.7 References: <200911061508.22482.jhb@freebsd.org> <20091110194048.D61601@ury.york.ac.uk> In-Reply-To: <20091110194048.D61601@ury.york.ac.uk> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200911101702.57525.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (bigwig.baldwin.cx); Tue, 10 Nov 2009 17:03:02 -0500 (EST) X-Virus-Scanned: clamav-milter 0.95.1 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-2.5 required=4.2 tests=AWL,BAYES_00,RDNS_NONE autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on bigwig.baldwin.cx Cc: current@freebsd.org, net@freebsd.org Subject: Re: [PATCH] Remove if_watchdog use X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Nov 2009 22:03:04 -0000 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 @ > /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 > @@ -2570,6 +2570,9 @@ > 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) @ /usr/src/sys/net/if.c:912 > 2nd 0xc0f9db68 mld_mtx (mld_mtx) @ /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