[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: Fileassoc locking
Elad Efrat <elad%NetBSD.org@localhost> wrote:
> So... attached is a second attempt at fileassoc locking. (Entire source
> file to make reading easier.) This time there are no rw locks, and I'm
> using mutexes and condition variables. I've tested this version as well,
> but once again, I'd appreciate a review for the changes.
I do not have time for a full review, but from a quick glance:
- Code should be re-structured to allocate/create objects before taking any
locks, and free/destroy after last locks. For example, operations which
might block or take a lot of time should be outside the locked paths.
- There are deadlocks (e.g. fileassoc_deregister). If lock order is
fileassoc_list_lock -> assoc_mtx, then you cannot lock in other way round.
Apart from comments, please add KASSERT()s for locking and ref-counting
- fileassoc_deregister() should remove entry from the list first, wait while
for references to exit, and then destroy.
- fileassoc_table_lookup() or its uses are missing locking (reference does
not provide exclusion). Thus, fileassoc_table does not need ref-counting,
unless for workarounds in fileassoc_table_run(), see below. Use of RW-lock
would fit in this case.
- fileassoc_file_lookup() is wrong: fileassoc_table should be locked when
performing lookup, it does not need ref-counting. Also, no need to hold
a reference on each fileassoc_file entry, reference must be held and kept
for caller on entry which was found.
- fileassoc_table_clear() and fileassoc_table_run() lack locking. Callback
execution without locks held will be problematic here.
- Why fileassoc_file_add() and fileassoc_file_delete() are waiting for zero
references? It does not look to me that fileassoc_file requires condvar,
e.g. last reference holder can free it. Also, fileassoc_file_add() checks
for duplicates - I think caller (of API) should do that.
- I think fileassoc_init() should be called manually, instead of spreading
RUN_ONCE() across API routines.
Main Index |
Thread Index |