Re: Fileassoc locking

[Elad Efrat <elad%NetBSD.org@localhost>]

On Fri, Dec 25, 2009 at 7:50 PM, Adam Hamsik <haaaad%gmail.com@localhost> wrote:

> RW locks have some performance problems, and they should not be used until 
> it's needed. I think that same think can be accomplished by using reference 
> counters and mutexes/cv variables. You can look at dm_dev_t I have used same 
> approach there. Rw locks has very bad effect on CPU caches and can slow 
> things down on a bigger SMP systems.

I didn't think it was this bad... Okay, I'll have a look and try to
redo it using mutexes and condition variables.

> Also it would be good to describe locking protocol used in fileassoc on top 
> of this file

Once it's done...

>> +/* Expects faf writer-locked. */
>> static void
>> file_free(struct fileassoc_file *faf)
>> {
>> @@ -126,11 +134,19 @@ file_free(struct fileassoc_file *faf)
>>
>>       LIST_REMOVE(faf, faf_list);
>>
>> +     rw_enter(&fileassoc_list_lock, RW_READER);
>>       LIST_FOREACH(assoc, &fileassoc_list, assoc_list) {
>> +             rw_enter(&assoc->assoc_lock, RW_READER);
>>               file_cleanup(faf, assoc);
>> +             rw_exit(&assoc->assoc_lock);
>>       }
>> +     rw_exit(&fileassoc_list_lock);
>> +
>>       vfs_composefh_free(faf->faf_handle);
>>       specificdata_fini(fileassoc_domain, &faf->faf_data);
>> +
>> +     rw_exit(&faf->faf_lock);
>> +
>
> Where faf->faf_lock was entered here ?

file_free() expects it to be locked -- see comment.

>> +     rw_init(&assoc->assoc_lock);
>>
>> +     /* Lock it so no one can use this assoc until we're done. */
>> +     rw_enter(&assoc->assoc_lock, RW_WRITER);
>> +
>> +     rw_enter(&fileassoc_list_lock, RW_WRITER);
>>       LIST_INSERT_HEAD(&fileassoc_list, assoc, assoc_list);
>> +     rw_exit(&fileassoc_list_lock);
>> +
>> +     rw_exit(&assoc->assoc_lock);
>>
>
> With one mutex used for fileassoc_list_lock you do not need to do this dance 
> you jsut lock mutex, insert head, unlock.

I have removed the locking on "assoc" here. There's going to be a
favorable reader:writer ratio for fileassoc_list_lock -- should it
still be a mutex?

> If function returns locked tbl it should be documented somewhere in this file.

Comments added where necessary...

>> -     return file_getdata(faf, assoc);
>> +     rw_enter(&assoc->assoc_lock, RW_READER);
>> +     data = file_getdata(faf, assoc);
>> +     rw_exit(&assoc->assoc_lock);
>> +
>> +     rw_exit(&faf->faf_lock);
>> +
>> +     return data;
>
> Here you return pointer for assoc data but, this can be released/freed by 
> another thread while caller of this will sleep. With reference counting you 
> will hold reference counter on assoc/data and no one can change them until he 
> has last reference holding by himself.

I think maybe to make fileassoc_lookup() always increase the reference
count on the file entry before it returns, and require the caller
(user of the KPI) to fileassoc_release() once it's done to avoid the
data being freed while in use.

> Maybe you should post whole file somewhere because it's hard to review your 
> changes this way because they are too large.

I'll do that in future posts.

Thanks,

-e.