SMPTODO: remove timeout(9) from ffs_softdep.c

[Coleman Kane]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffs_softdep.c-newcallout2.diff
Type: text/x-patch
Size: 2251 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-arch/attachments/20080312/162badc7/ffs_softdep.c-newcallout2.bin