From owner-svn-src-all@FreeBSD.ORG Fri Nov 14 17:32:21 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 32041FE0; Fri, 14 Nov 2014 17:32:21 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 0A8BF7C4; Fri, 14 Nov 2014 17:32:21 +0000 (UTC) Received: from ralph.baldwin.cx (pool-173-70-85-31.nwrknj.fios.verizon.net [173.70.85.31]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id DD29AB97D; Fri, 14 Nov 2014 12:32:19 -0500 (EST) From: John Baldwin To: Adrian Chadd Subject: Re: svn commit: r274493 - head/sys/dev/ath Date: Fri, 14 Nov 2014 12:25:49 -0500 Message-ID: <3477265.ncieCK2P6c@ralph.baldwin.cx> User-Agent: KMail/4.14.2 (FreeBSD/10.1-PRERELEASE; KDE/4.14.2; amd64; ; ) In-Reply-To: <201411140426.sAE4QQYN085796@svn.freebsd.org> References: <201411140426.sAE4QQYN085796@svn.freebsd.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Fri, 14 Nov 2014 12:32:20 -0500 (EST) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 14 Nov 2014 17:32:21 -0000 On Friday, November 14, 2014 04:26:26 AM Adrian Chadd wrote: > Author: adrian > Date: Fri Nov 14 04:26:26 2014 > New Revision: 274493 > URL: https://svnweb.freebsd.org/changeset/base/274493 > > Log: > Migrate the callouts from using mutex locks to being mpsafe with > the locks being held by the callers. > > Kill callout_drain() and use callout_stop(). > > Modified: > head/sys/dev/ath/if_ath.c > > Modified: head/sys/dev/ath/if_ath.c > ============================================================================ > == --- head/sys/dev/ath/if_ath.c Fri Nov 14 00:25:10 2014 (r274492) > +++ head/sys/dev/ath/if_ath.c Fri Nov 14 04:26:26 2014 (r274493) > @@ -669,8 +669,8 @@ ath_attach(u_int16_t devid, struct ath_s > goto bad; > } > > - callout_init_mtx(&sc->sc_cal_ch, &sc->sc_mtx, 0); > - callout_init_mtx(&sc->sc_wd_ch, &sc->sc_mtx, 0); > + callout_init(&sc->sc_cal_ch, 1); /* MPSAFE */ > + callout_init(&sc->sc_wd_ch, 1); /* MPSAFE */ Why did you do this? You probably don't want this. You can use callout_init_mtx() with callout_stop(), and if you do it closes races between your callout routine and callout_stop(). If you use callout_init() instead, then you need to patch your callout routines to now explicitly check your own private flag after doing ATH_LOCK(). That is, you can either do: foo_attach() { callout_init_mtx(&sc->timer, &sc->mtx, 0); } foo_timeout(void *arg) { sc = arg; mtx_assert(&sc->mtx, MA_OWNED); /* periodic actions */ callout_schedule() /* or callout_reset */ } foo_start() { mtx_lock(&sc->mtx); callout_reset(&sc->timer, ...); } foo_stop() { mtx_lock(&sc->mtx); callout_stop(&sc->timer); /* foo_timeout will not execute after this point */ mtx_unlock(&sc->mtx); } or you can do this: foo_attach() { callout_init(&sc->timer, CALLOUT_MPSAFE); } foo_timeout(void *arg) { sc = arg; mtx_lock(&sc->mtx); if (sc->stopped) { mtx_unlock(&sc->mtx); return; } /* periodic actions */ callout_schedule() mtx_unlock(&sc->mtx); } foo_start() { mtx_lock(&sc->mtx); foo->stopped = 0; callout_reset(&sc->timer, ...); } foo_stop() { mtx_lock(&sc->mtx); foo->stopped = 1; callout_stop(&sc->timer); /* * foo_timeout might still execute, but it will see the 'stopped' * flag and return */ mtx_unlock(&sc->mtx); } Personally, I find the former (using callout_init_mtx()) a lot simpler to read and work with. As it is you can now either switch back to using callout_init_mtx() and undo the changes to ath_calibrate() (but leave all your other changes in place), or you now need to add a new stopped flag that you set / clear in the places you do callout_stop() / callout_reset() and add a new check for the new stopped flag in ath_calibrate(). -- John Baldwin