From owner-freebsd-net@FreeBSD.ORG Tue Nov 10 22:16:25 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 2BC10106566B for ; Tue, 10 Nov 2009 22:16:25 +0000 (UTC) (envelope-from gavin@FreeBSD.org) Received: from mail-gw2.york.ac.uk (mail-gw2.york.ac.uk [144.32.128.247]) by mx1.freebsd.org (Postfix) with ESMTP id D3D9E8FC08 for ; Tue, 10 Nov 2009 22:16:24 +0000 (UTC) Received: from mail-gw6.york.ac.uk (mail-gw6.york.ac.uk [144.32.129.26]) by mail-gw2.york.ac.uk (8.13.6/8.13.6) with ESMTP id nAALj39T028689; Tue, 10 Nov 2009 21:45:03 GMT Received: from ury.york.ac.uk ([144.32.108.81]) by mail-gw6.york.ac.uk with esmtps (TLSv1:AES256-SHA:256) (Exim 4.68) (envelope-from ) id 1N7yWN-0006QZ-L9; Tue, 10 Nov 2009 21:45:03 +0000 Received: from ury.york.ac.uk (localhost.york.ac.uk [127.0.0.1]) by ury.york.ac.uk (8.14.3/8.14.3) with ESMTP id nAALj3xI040287; Tue, 10 Nov 2009 21:45:03 GMT (envelope-from gavin@FreeBSD.org) Received: from localhost (gavin@localhost) by ury.york.ac.uk (8.14.3/8.14.3/Submit) with ESMTP id nAALj3pn040284; Tue, 10 Nov 2009 21:45:03 GMT (envelope-from gavin@FreeBSD.org) X-Authentication-Warning: ury.york.ac.uk: gavin owned process doing -bs Date: Tue, 10 Nov 2009 21:45:03 +0000 (GMT) From: Gavin Atkinson X-X-Sender: gavin@ury.york.ac.uk To: John Baldwin In-Reply-To: <200911061508.22482.jhb@freebsd.org> Message-ID: <20091110194048.D61601@ury.york.ac.uk> References: <200911061508.22482.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-York-MailScanner: Found to be clean X-York-MailScanner-From: gavin@freebsd.org 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:16:25 -0000 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; } /* 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 KDB: stack backtrace: db_trace_self_wrapper(c0cb835f,e54b1b20,c08e7b55,c08d891b,c0cbb215,...) at db_trace_self_wrapper+0x26 kdb_backtrace(c08d891b,c0cbb215,c4d30e18,c4d2a9c0,e54b1b7c,...) at kdb_backtrace+0x29 _witness_debugger(c0cbb215,c0f9db68,c0cbb365,c4d2a9c0,c0cd457d,...) at _witness_debugger+0x25 witness_checkorder(c0f9db68,9,c0cd457d,239,0,...) at witness_checkorder+0x839 _mtx_lock_flags(c0f9db68,0,c0cd457d,239,c5340ba0,...) at _mtx_lock_flags+0xc9 mld_domifdetach(c50f5000,c0db5e54,c0dc0880,e54b1c24,c09532a6,...) at mld_domifdetach+0x2c in6_domifdetach(c50f5000,c5340ba0,390,4be,c50f522c,...) at in6_domifdetach+0x15 if_detach(c50f5000) at if_detach+0x916 ether_ifdetach(c50f5000,0,c0c6cbbc,34f,c555d380,...) at ether_ifdetach+0x3d an_detach(c555d380,c4ead060,c0da0758,a76,e54b1c94,...) at an_detach+0xb5 device_detach(c555d380,0,c0d5a9b8,0,c4ff6500,...) at device_detach+0x8c pccard_detach_card(c4ff6500,c4e3e8bc,c0d5a9b8) at pccard_detach_card+0x44 exca_removal(c4ed2004,0,c0c93731,1da,c8,...) at exca_removal+0x59 cbb_event_thread(c4ed2000,e54b1d38,c0cb02eb,343,c4d81aa0,...) at cbb_event_thread+0x107 fork_exit(c0720cb0,c4ed2000,e54b1d38) at fork_exit+0xb8 fork_trampoline() at fork_trampoline+0x8 --- trap 0, eip = 0, esp = 0xe54b1d70, ebp = 0 --- an0: detached 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. > - 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' Hope that helps, Gavin