From owner-freebsd-arch@FreeBSD.ORG Wed Mar 12 14:30:06 2008 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 67C42106567B for ; Wed, 12 Mar 2008 14:30:06 +0000 (UTC) (envelope-from cokane@cokane.org) Received: from QMTA04.westchester.pa.mail.comcast.net (qmta04.westchester.pa.mail.comcast.net [76.96.62.40]) by mx1.freebsd.org (Postfix) with ESMTP id 0544D8FC19 for ; Wed, 12 Mar 2008 14:30:05 +0000 (UTC) (envelope-from cokane@cokane.org) Received: from OMTA04.westchester.pa.mail.comcast.net ([76.96.62.35]) by QMTA04.westchester.pa.mail.comcast.net with comcast id 08yg1Z00D0ldTLk540US00; Wed, 12 Mar 2008 14:19:09 +0000 Received: from discordia ([24.61.189.203]) by OMTA04.westchester.pa.mail.comcast.net with comcast id 0EL31Z00R4PktZC3Q00000; Wed, 12 Mar 2008 14:20:04 +0000 X-Authority-Analysis: v=1.0 c=1 a=yWIViUiLWPYA:10 a=CUMa_SbteGx0BZgX1pgA:9 a=HrSz8c6paNKFlKRnIwkA:7 a=LGVNa8fhoMo1oVVE5VAFUJlUgBwA:4 a=zUBsD6tbDSsA:10 a=-rtcXVvtY7498SX-e9UA:9 a=lEhTTu5oXNJMJ4XxhzkA:7 a=DneVQdTm6Pk93g1E2aIrDb4HF0cA:4 a=NfA2RSpTaHsA:10 Received: by discordia (Postfix, from userid 103) id BEB961636F9; Wed, 12 Mar 2008 10:20:03 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.1.8-gr1 (2007-02-13) on discordia X-Spam-Level: X-Spam-Status: No, score=-4.4 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=ham version=3.1.8-gr1 Received: from [172.20.1.3] (erwin.int.cokane.org [172.20.1.3]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by discordia (Postfix) with ESMTP id F31B31636F8; Wed, 12 Mar 2008 10:19:51 -0400 (EDT) Message-ID: <47D7E5BF.2060102@cokane.org> Date: Wed, 12 Mar 2008 10:16:31 -0400 From: Coleman Kane User-Agent: Thunderbird 2.0.0.12 (X11/20080304) MIME-Version: 1.0 To: John Baldwin References: <47D7C25D.5070908@cokane.org> <200803120945.29018.jhb@freebsd.org> In-Reply-To: <200803120945.29018.jhb@freebsd.org> Content-Type: multipart/mixed; boundary="------------080002040909020109040007" Cc: freebsd-arch@freebsd.org Subject: Re: SMPTODO: remove timeout(9) from ffs_softdep.c X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 12 Mar 2008 14:30:06 -0000 This is a multi-part message in MIME format. --------------080002040909020109040007 Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit John Baldwin wrote: > On Wednesday 12 March 2008 07:45:33 am Coleman Kane wrote: > >> Hi all, >> >> I was poking around SMPTODO for some work during an idle night, and I >> decided to fix the non-MPSAFE use of timeout(9) in ffs_softdep.c, and >> learn more about the callout_* API in the kernel. I'm attaching a patch >> of what I've done, which I am running in my current kernel at the moment >> (and I am using softupdates on a number of filesystems on this SMP >> machine). >> >> Can anyone else try it out / review it / give feedback? >> >> @@ -1403,7 +1406,9 @@ softdep_initialize() >> void >> softdep_uninitialize() >> { >> - >> + ACQUIRE_LOCK(&lk); >> + callout_drain(&softdep_callout); >> + FREE_LOCK(&lk); >> hashdestroy(pagedep_hashtbl, M_PAGEDEP, pagedep_hash); >> hashdestroy(inodedep_hashtbl, M_INODEDEP, inodedep_hash); >> hashdestroy(newblk_hashtbl, M_NEWBLK, newblk_hash); >> > > Don't hold the mutex over a drain and leave the blank line at the start of the > function (style(9)). > Thanks. This point was not completely clear from the man page (whether to hold the lock around it or not). I went looking around for examples of this... Had I looked further, I would have found my answer in bge_detach of if_bge.c. > >> @@ -5858,8 +5863,16 @@ request_cleanup(mp, resource) >> * We wait at most tickdelay before proceeding in any case. >> */ >> proc_waiting += 1; >> - if (handle.callout == NULL) >> - handle = timeout(pause_timer, 0, tickdelay > 2 ? tickdelay : 2); >> + ACQUIRE_LOCK(&lk); >> + if(callout_active(&softdep_callout) == FALSE) { >> + /* >> + should always return zero due to callout_active being called to verify that no active >> + timeout already exists, which is the case where this would return non-zero (and >> + callout_active(&softdep_callout) would be TRUE. >> + */ >> + callout_reset(&softdep_callout, tickdelay > 2 ? tickdelay : 2, pause_timer, 0); >> + } >> + FREE_LOCK(&lk); >> msleep((caddr_t)&proc_waiting, &lk, PPAUSE, "softupdate", 0); >> proc_waiting -= 1; >> return (1); >> > > The lock is already held, so no need to lock it again. Also, space after > 'if'. I'm not sure the new comment is needed as the reader can already > infer that from the callout_active() test. Also, I think you really want > callout_pending() rather than callout_active() if pause_timer() executes > normally without rescheduling itself the callout will still be marked > active and the next time this function is invoked it won't schedule the > callout. > Thanks, I see this now. Every call to request_cleanup seems to already acquire lk. This solves the use of callout_deactivate, below. > >> @@ -5873,15 +5886,17 @@ static void >> pause_timer(arg) >> void *arg; >> { >> - >> - ACQUIRE_LOCK(&lk); >> + /* Implied by callout_* API */ >> + /* ACQUIRE_LOCK(&lk); */ >> *stat_countp += 1; >> wakeup_one(&proc_waiting); >> - if (proc_waiting > 0) >> - handle = timeout(pause_timer, 0, tickdelay > 2 ? tickdelay : 2); >> - else >> - handle.callout = NULL; >> - FREE_LOCK(&lk); >> + if (proc_waiting > 0) { >> + /* We don't care about the return value here. */ >> + callout_reset(&softdep_callout, tickdelay > 2 ? tickdelay : 2, pause_timer, 0); >> + } else { >> + callout_deactivate(&softdep_callout); >> + } >> + /* FREE_LOCK(&lk); */ >> } >> > > No need to use callout_deactivate() here, the callout is already deactivated > when it is invoked. I think you can also leave out the comment about the > return value as the vast majority of places in the kernel that call > callout_reset() ignore the return value, so it is a common practice. > Technically, the callout is no longer considered "pending". According to the man page, it isn't deactivated at the return of pause_timer. Nonetheless, the pointer above about s/callout_active/callout_pending/ makes this check here unnecessary, and I'm sure that's what you're meaning by this comment. I am attaching the revised patch. -- Coleman Kane --------------080002040909020109040007 Content-Type: text/x-patch; name="ffs_softdep.c-newcallout2.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="ffs_softdep.c-newcallout2.diff" diff --git a/sys/ufs/ffs/ffs_softdep.c b/sys/ufs/ffs/ffs_softdep.c index 3e8ba26..d5c8536 100644 --- a/sys/ufs/ffs/ffs_softdep.c +++ b/sys/ufs/ffs/ffs_softdep.c @@ -664,7 +664,7 @@ static int maxindirdeps = 50; /* max number of indirdeps before slowdown */ static int tickdelay = 2; /* number of ticks to pause during slowdown */ static int proc_waiting; /* tracks whether we have a timeout posted */ static int *stat_countp; /* statistic to count in proc_waiting timeout */ -static struct callout_handle handle; /* handle on posted proc_waiting timeout */ +static struct callout softdep_callout; static int req_pending; static int req_clear_inodedeps; /* syncer process flush some inodedeps */ #define FLUSH_INODES 1 @@ -1394,6 +1394,9 @@ softdep_initialize() bioops.io_complete = softdep_disk_write_complete; bioops.io_deallocate = softdep_deallocate_dependencies; bioops.io_countdeps = softdep_count_dependencies; + + /* Initialize the callout with an mtx. */ + callout_init_mtx(&softdep_callout, &lk, 0); } /* @@ -1404,6 +1407,7 @@ void softdep_uninitialize() { + callout_drain(&softdep_callout); hashdestroy(pagedep_hashtbl, M_PAGEDEP, pagedep_hash); hashdestroy(inodedep_hashtbl, M_INODEDEP, inodedep_hash); hashdestroy(newblk_hashtbl, M_NEWBLK, newblk_hash); @@ -5858,8 +5862,9 @@ request_cleanup(mp, resource) * We wait at most tickdelay before proceeding in any case. */ proc_waiting += 1; - if (handle.callout == NULL) - handle = timeout(pause_timer, 0, tickdelay > 2 ? tickdelay : 2); + if (callout_pending(&softdep_callout) == FALSE) { + callout_reset(&softdep_callout, tickdelay > 2 ? tickdelay : 2, pause_timer, 0); + } msleep((caddr_t)&proc_waiting, &lk, PPAUSE, "softupdate", 0); proc_waiting -= 1; return (1); @@ -5874,14 +5879,12 @@ pause_timer(arg) void *arg; { - ACQUIRE_LOCK(&lk); + /* The callout_ API has acquired mtx and will hold it around this function call. */ *stat_countp += 1; wakeup_one(&proc_waiting); - if (proc_waiting > 0) - handle = timeout(pause_timer, 0, tickdelay > 2 ? tickdelay : 2); - else - handle.callout = NULL; - FREE_LOCK(&lk); + if (proc_waiting > 0) { + callout_reset(&softdep_callout, tickdelay > 2 ? tickdelay : 2, pause_timer, 0); + } } /* --------------080002040909020109040007--