From owner-freebsd-arch@FreeBSD.ORG Wed Mar 12 13:53:54 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 D9DE7106567D for ; Wed, 12 Mar 2008 13:53:54 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by mx1.freebsd.org (Postfix) with ESMTP id C38F68FC14 for ; Wed, 12 Mar 2008 13:53:54 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from zion.baldwin.cx (66-23-211-162.clients.speedfactory.net [66.23.211.162]) by elvis.mu.org (Postfix) with ESMTP id C89881A4D8B; Wed, 12 Mar 2008 06:53:01 -0700 (PDT) From: John Baldwin To: freebsd-arch@freebsd.org Date: Wed, 12 Mar 2008 09:45:28 -0400 User-Agent: KMail/1.9.7 References: <47D7C25D.5070908@cokane.org> In-Reply-To: <47D7C25D.5070908@cokane.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200803120945.29018.jhb@freebsd.org> Cc: Coleman Kane 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 13:53:55 -0000 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)). > @@ -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. > @@ -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. -- John Baldwin