Date: Fri, 16 Jan 2009 00:44:17 +0100 From: Max Laier <max@love2party.net> To: John Baldwin <jhb@freebsd.org> Cc: src-committers@freebsd.org, Kip Macy <kmacy@freebsd.org>, svn-src-all@freebsd.org, Attilio Rao <attilio@freebsd.org>, Robert Watson <rwatson@freebsd.org>, svn-src-head@freebsd.org Subject: Re: svn commit: r186187 - head/sys/net Message-ID: <200901160044.18044.max@love2party.net> In-Reply-To: <200901151659.35735.jhb@freebsd.org> References: <200812161703.mBGH3M7m042955@svn.freebsd.org> <200812172109.55724.max@love2party.net> <200901151659.35735.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Thank you for getting back to this ...
On Thursday 15 January 2009 22:59:35 John Baldwin wrote:
> So the usual model would be to do something like this:
>
> LIST(foo, ) foo_list;
>
> foo_add(foo *p)
> {
>
> /* Construct foo, but don't lock it. */
> WLOCK(foo_list);
> LIST_INSERT(foo_list, foo);
> WUNLOCK(foo_list);
> }
>
> foo *
> foo_find(int key)
> {
> foo *p;
>
> RLOCK(foo_list);
> LIST_FOREACH(p, foo_list) {
> if (p->key == key) {
> LOCK(p);
> RUNLOCK(foo_list);
Is this allowed now? I was under the impression that it was bad for some
reason to interchange locks/unlocks like this. Something about WITNESS
getting confused and/or real lock order problems? I'm more than happy if
that's not the case.
> return (p);
> }
> }
> RUNLOCK(foo_list);
> return (NULL);
> }
>
> something_else()
> {
> foo *p;
>
> RLOCK(foo_list);
> LIST_FOREACH(p, foo_list) {
> LOCK(p);
> bar(p);
> UNLOCK(p);
> }
> RUNLOCK(foo_list);
> }
>
> From your description it would appear that you are doing something_else()
> but without actually locking foo_list. Unless you can demonstrate a clear
> win in benchmarks from not having the lock there, I would suggest just
> going with the simpler and more common approach. Depending on the mtx of
> the individual head doesn't do anything to solve races with removing the
> head from the list.
The thing is that the list of pfil_heads is just a "configuration time"
construct. We really don't want to look at it in the hot path. Hence we
allow the caller of pfil_head_register() (foo_add) to hold on to its reference
of the pfil_head and work with it without requiring any interaction with the
lookup list. This, however, is not the problem at all. It's the
responsibility of the caller to ensure that it won't fiddle with the cached
reference after calling pfil_head_unregister().
> So, reading a bit further, I think the real fix is that the pfil API is a
> bit busted. What you really want to do is have pfil_get_head() operate
> like foo_find() and lock the head before dropping the list lock. That is
> the only race I see in the current code. However, because you malloc()
> after calling pfil_get_head() that is problematic. That is, the current
> consumer code looks like this:
>
> ph = pfil_get_head(x, y);
>
> pfil_add_hook(..., ph); /* calls malloc() */
>
> I think you should change the API to be this instead:
>
> pfil_add_hook(..., x, y);
>
> and have pfil_get_head() do the lookup internally:
>
> pfil_add_hook()
> {
>
> malloc(...);
>
> ph = pfil_get_head(x, y); /* locks the 'ph' like foo_find() */
> }
This is a good idea/catch, but still not my initial problem.
My real problem is what foo_remove() should look like in the scenario above.
I assume that it should look something like this:
foo_remove(int key) {
WLOCK(foo_list);
LIST_FOREACH(p, foo_list)
if (p->key == key) {
LIST_REMOVE(p, entries);
LOCK(p); /* <--- HERE */
WUNLOCK(foo_list);
/* free resources inside p */
/* uninit lock */
free(p);
return;
}
WUNLOCK(foo_list);
}
I assume that locking the element's lock above is necessary as a pfil_add_hook
as you describe above will only hold the element's lock while adding resources
to it and not the list lock (as foo_find drops that before returning) and as
such we might not see all changes done by pfil_add_hook in the thread that is
doing the foo_remove(), right?
Something similar is true for the the foo_add() above. Is there a reason why
we don't want to lock the element's lock while we initialize? While it is
safe if we always go through foo_find() before altering the element and thus
synchronize on the list lock, it seems bogus to rely on this (even though it
does make sense to go through foo_find() every time as I wrote earlier).
--
/"\ Best regards, | mlaier@freebsd.org
\ / Max Laier | ICQ #67774661
X http://pf4freebsd.love2party.net/ | mlaier@EFnet
/ \ ASCII Ribbon Campaign | Against HTML Mail and News
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200901160044.18044.max>