Date: Thu, 2 Oct 2014 14:07:12 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Mateusz Guzik <mjguzik@gmail.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Will Andrews <will@freebsd.org> Subject: Re: svn commit: r272366 - head/sys/kern Message-ID: <20141002133405.W1665@besplex.bde.org> In-Reply-To: <20141001181825.GA16048@dft-labs.eu> References: <201410011532.s91FWTZL050853@svn.freebsd.org> <20141001181825.GA16048@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 1 Oct 2014, Mateusz Guzik wrote: > On Wed, Oct 01, 2014 at 03:32:29PM +0000, Will Andrews wrote: >> Author: will >> Date: Wed Oct 1 15:32:28 2014 >> New Revision: 272366 >> URL: https://svnweb.freebsd.org/changeset/base/272366 >> >> Log: >> In the syncer, drop the sync mutex while patting the watchdog. >> >> Some watchdog drivers (like ipmi) need to sleep while patting the watchdog. >> See sys/dev/ipmi/ipmi.c:ipmi_wd_event(), which calls malloc(M_WAITOK). > > Is not such malloc inherently bad in case of watchdog? > > It looks like the code waits for request completion and then frees it > unconditionally. As such, a local var on stack would do the trick. > > The code msleeps later, so this hack of dropping sync_mtx is still > needed. It's not a hack then. Then it is at the end of the loop, so any state changes are picked up by looping. However, sync_vnode() has _always_ just released the lock and re-acquired it, so there is no new problem, and probably no old one either. It is just hard to see that there is no problem in code that doesn't check for state changes after sleeping. % while (!LIST_EMPTY(slp)) { % error = sync_vnode(slp, &bo, td); % if (error == 1) { % LIST_REMOVE(bo, bo_synclist); % LIST_INSERT_HEAD(next, bo, bo_synclist); % continue; % } Since we get here, sync_vnode() didn't return early; it unlocked, and the last statement in it is to re-lock... (All sync_vnode() did before unlocking was to read a vnode from the list and lock and hold it.) % % if (first_printf == 0) ... so we can sprinkle any number of unlock/lock pairs here without adding any new problems. % wdog_kern_pat(WD_LASTVAL); % % } Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141002133405.W1665>