Date: Fri, 14 Nov 2014 12:25:49 -0500 From: John Baldwin <jhb@freebsd.org> To: Adrian Chadd <adrian@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r274493 - head/sys/dev/ath Message-ID: <3477265.ncieCK2P6c@ralph.baldwin.cx> In-Reply-To: <201411140426.sAE4QQYN085796@svn.freebsd.org> References: <201411140426.sAE4QQYN085796@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3477265.ncieCK2P6c>