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