From owner-svn-src-head@FreeBSD.ORG Thu Oct 2 04:07:46 2014 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 96EAC3E2; Thu, 2 Oct 2014 04:07:46 +0000 (UTC) Received: from mail109.syd.optusnet.com.au (mail109.syd.optusnet.com.au [211.29.132.80]) by mx1.freebsd.org (Postfix) with ESMTP id 56917D86; Thu, 2 Oct 2014 04:07:45 +0000 (UTC) Received: from c122-106-147-133.carlnfd1.nsw.optusnet.com.au (c122-106-147-133.carlnfd1.nsw.optusnet.com.au [122.106.147.133]) by mail109.syd.optusnet.com.au (Postfix) with ESMTPS id E8C89D6247C; Thu, 2 Oct 2014 14:07:35 +1000 (EST) Date: Thu, 2 Oct 2014 14:07:12 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Mateusz Guzik Subject: Re: svn commit: r272366 - head/sys/kern In-Reply-To: <20141001181825.GA16048@dft-labs.eu> Message-ID: <20141002133405.W1665@besplex.bde.org> References: <201410011532.s91FWTZL050853@svn.freebsd.org> <20141001181825.GA16048@dft-labs.eu> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=fvDlOjIf c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=6I5d2MoRAAAA:8 a=UiRU701cgEFRS5dujKwA:9 a=CjuIK1q_8ugA:10 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Will Andrews X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Oct 2014 04:07:46 -0000 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