From owner-freebsd-arch@FreeBSD.ORG Wed Mar 12 12:05:05 2008 Return-Path: Delivered-To: arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D21491065680 for ; Wed, 12 Mar 2008 12:05:05 +0000 (UTC) (envelope-from cokane@cokane.org) Received: from QMTA08.westchester.pa.mail.comcast.net (qmta08.westchester.pa.mail.comcast.net [76.96.62.80]) by mx1.freebsd.org (Postfix) with ESMTP id 6FC138FC1D for ; Wed, 12 Mar 2008 12:05:04 +0000 (UTC) (envelope-from cokane@cokane.org) Received: from OMTA01.westchester.pa.mail.comcast.net ([76.96.62.11]) by QMTA08.westchester.pa.mail.comcast.net with comcast id 0B911Z0030EZKEL5802v00; Wed, 12 Mar 2008 11:48:24 +0000 Received: from discordia ([24.61.189.203]) by OMTA01.westchester.pa.mail.comcast.net with comcast id 0Bp41Z0054PktZC3M00000; Wed, 12 Mar 2008 11:49:04 +0000 X-Authority-Analysis: v=1.0 c=1 a=Pj4_Y536NHEA:10 a=0RHiSNdv7K1cofAIcD8A:9 a=c4nhsv1tePooTTT5wNglNuvk8YcA:4 a=50e4U0PicR4A:10 a=-rtcXVvtY7498SX-e9UA:9 a=34nm3S-1UQFYRJjl--IA:7 a=XnLui3lyQwI9Xcj7tu-HLo7BWC8A:4 a=NfA2RSpTaHsA:10 Received: by discordia (Postfix, from userid 103) id 1C5351636F9; Wed, 12 Mar 2008 07:49:04 -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 7440D1636F8 for ; Wed, 12 Mar 2008 07:48:53 -0400 (EDT) Message-ID: <47D7C25D.5070908@cokane.org> Date: Wed, 12 Mar 2008 07:45:33 -0400 From: Coleman Kane User-Agent: Thunderbird 2.0.0.12 (X11/20080304) MIME-Version: 1.0 To: arch@FreeBSD.org Content-Type: multipart/mixed; boundary="------------000300090804080100080401" Cc: Subject: 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 12:05:05 -0000 This is a multi-part message in MIME format. --------------000300090804080100080401 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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? -- Coleman Kane --------------000300090804080100080401 Content-Type: text/x-patch; name="ffs_softdep.c-newcallout.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="ffs_softdep.c-newcallout.diff" diff --git a/sys/ufs/ffs/ffs_softdep.c b/sys/ufs/ffs/ffs_softdep.c index 3e8ba26..3e9122f 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); } /* @@ -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); @@ -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); @@ -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); */ } /* --------------000300090804080100080401--