Re: Fileassoc locking

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

Hey,

The latest version of the file is at

    http://www.netbsd.org/~elad/kern_fileassoc.c

(This time I've built a kernel with LOCKDEBUG and did the tests again,
uncovering some bugs where I freed undestroyed mutexes/condition
variables etc.)

On Sat, Dec 26, 2009 at 10:30 AM, Mindaugas Rasiukevicius
<rmind%netbsd.org@localhost> wrote:

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

Unless I'm missing something, there's only one place where we're
allocating memory while holding a mutex -- fileassoc_table_resize(). I
rewrote it to only allocate memory once (through hashinit(), removing
the kmem_alloc()/kmem_free() calls). I can split the functionality to
two -- fileassoc_table_resize() and fileassoc_table_migrate(), where
the former will do the memory allocation without holding any locks and
the latter the actual migration while holding the table locked; does
this make sense...? (Keep in mind that the migration process is going
to be the real time consumer here...)

Can you spot other such places where I'm allocating memory while holding a lock?

> - 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
>  assumptions.
>
> - fileassoc_deregister() should remove entry from the list first, wait while
>  for references to exit, and then destroy.

I've fixed this, and added some KASSERTS()s. Let me know if there are
some places I'm missing more...

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

Why do we need to hold the table exclusively in fileassoc_table_lookup()?

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

Hmm... Why should the table be locked? If we hold a reference, and
modifiers of the table will not modify it until they hold it
exclusively, why should we lock it...? Also, I'm holding a reference
to the file because we use it in the comparisons (of the file handle
etc.) -- this is to prevent it from being modified while in use.

As for returning the entry with a reference -- I'll do that once the
rest of the locking is "done" because it'll require changes to users
of fileassoc(9)...

> - fileassoc_table_clear() and fileassoc_table_run() lack locking.  Callback
>  execution without locks held will be problematic here.

fileassoc_table_clear() is holding a reference to the assoc and the
table, and locks the file it uses. I presume you mean it lacks locking
for the table -- could you explain why? Functions that modify the
table are waiting for all other users to finish, so no entries can be
added/removed while it's traversing the table. (Same applies for
fileassoc_table_run(), only that in this case we don't lock the file,
but also just reference it to prevent it from being modified/removed
while it's in use.)

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

fileassoc_file_add() does not wait for zero references; it waits until
its caller is the only user of the table to which the file is about to
be added -- as it's going to modify the table.

fileassoc_file_delete() indicates that a file has been deleted, and
fileassoc should remove all "assocs" associated with it. Therefore,
once called, we're waiting for all references to the file to go away,
and only then remove it from the internal table. Think of fileassoc
storage like a file-system. Even if a file is unused (i.e. has
reference count of zero) it should not be deleted; conversely, once
it's in use, the user won't delete it when done even though it's the
"last" user.

As for your suggestion that the KPI user should check for duplicates
-- note that to the KPI user, a "file" is really a combination of
"file" and "assoc," which is properly detected in fileassoc_add()
where EEXIST is returned if you're trying to add a combination that
already exists. Otherwise, the "file" part is used internally, so the
KPI user can't (and shouldn't) check whether it exists or not.

> - I think fileassoc_init() should be called manually, instead of spreading
>  RUN_ONCE() across API routines.

That's unrelated to locking, but anyway, it was a decision made by
yamt@ IIRC -- it used to be done the way you suggest. Since I don't
care how it's initialized as long as it gets done, I'll leave it as it
is but won't object to any changes.

Thanks,

-e.