Re: Fileassoc locking

[Adam Hamsik <haaaad%gmail.com@localhost>]

On Dec,Saturday 26 2009, at 2:33 AM, Elad Efrat wrote:

> 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.

Locking structure in one routine and then releasing it in other is not standard 
behavior. I think that you should revisit it. In best case you should do 
mutex_enter(foo) and mutex_exit(foo) in same routine not in two or more.

>>> +     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 forfileassoc_list_lock -- should it
> still be a mutex?

I think that mutexes are much cheaper then rw locks in locking. bu you should 
hold mutex on fileassoc_list_lock only for absolutely required time (you should 
not do any heavy operations like, memory allocation, memory free etc.)

> 
>> 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.

Yes this seems like a good approach to me. protocol can be 
mutex_enter(faf_list)
... find faf ...
 mutex_enter(faf)
 increase ref counter
 mutex_exit(faf)
mutex_exit(faf_list)

But you need to be sure that there is no other code who is doing mutex_enter on 
faf_list and faf locks in a reversed order.

>> 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.

Regards

Adam.