Date: Wed, 12 Mar 2008 10:16:31 -0400 From: Coleman Kane <cokane@cokane.org> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-arch@freebsd.org Subject: Re: SMPTODO: remove timeout(9) from ffs_softdep.c Message-ID: <47D7E5BF.2060102@cokane.org> In-Reply-To: <200803120945.29018.jhb@freebsd.org> References: <47D7C25D.5070908@cokane.org> <200803120945.29018.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?47D7E5BF.2060102>