From owner-svn-src-all@FreeBSD.ORG Thu Jan 15 23:44:20 2009 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 82FAF1065676 for ; Thu, 15 Jan 2009 23:44:20 +0000 (UTC) (envelope-from max@love2party.net) Received: from moutng.kundenserver.de (moutng.kundenserver.de [212.227.126.177]) by mx1.freebsd.org (Postfix) with ESMTP id B69198FC14 for ; Thu, 15 Jan 2009 23:44:19 +0000 (UTC) (envelope-from max@love2party.net) Received: from vampire.homelinux.org (dslb-088-066-043-041.pools.arcor-ip.net [88.66.43.41]) by mrelayeu.kundenserver.de (node=mrelayeu4) with ESMTP (Nemesis) id 0ML21M-1LNbso26f9-0007mM; Fri, 16 Jan 2009 00:44:18 +0100 Received: (qmail 45440 invoked from network); 15 Jan 2009 23:44:18 -0000 Received: from fbsd8.laiers.local (192.168.4.151) by router.laiers.local with SMTP; 15 Jan 2009 23:44:18 -0000 From: Max Laier Organization: FreeBSD To: John Baldwin Date: Fri, 16 Jan 2009 00:44:17 +0100 User-Agent: KMail/1.10.4 (FreeBSD/8.0-CURRENT; KDE/4.1.4; i386; ; ) References: <200812161703.mBGH3M7m042955@svn.freebsd.org> <200812172109.55724.max@love2party.net> <200901151659.35735.jhb@freebsd.org> In-Reply-To: <200901151659.35735.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200901160044.18044.max@love2party.net> X-Provags-ID: V01U2FsdGVkX1/miOsifbHE/WzLD02cOWxmwx/MKu9QLzNgGU1 h+m75L0Rp5WMBPQRm/DMRWNpF2LzjNkJx7gzvXiboPAQCNgH/9 cjXYxamaVBu+pAg+XlztA== Cc: src-committers@freebsd.org, Kip Macy , svn-src-all@freebsd.org, Attilio Rao , Robert Watson , svn-src-head@freebsd.org Subject: Re: svn commit: r186187 - head/sys/net X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Jan 2009 23:44:20 -0000 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