Date: Thu, 15 Jan 2009 16:59:35 -0500 From: John Baldwin <jhb@freebsd.org> To: Max Laier <max@love2party.net> 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: <200901151659.35735.jhb@freebsd.org> In-Reply-To: <200812172109.55724.max@love2party.net> References: <200812161703.mBGH3M7m042955@svn.freebsd.org> <alpine.BSF.1.10.0812161751250.75599@fledge.watson.org> <200812172109.55724.max@love2party.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wednesday 17 December 2008 3:09:55 pm Max Laier wrote: > As the upper half of this thread has turned into a style(9) bikeshed, let me > replay the lower half and add more locking folks. > > The change in question is here: > http://svn.freebsd.org/viewvc/base/head/sys/net/pfil.c?r1=173904&r2=186187 > > On Tuesday 16 December 2008 19:20:40 Robert Watson wrote: > > On Tue, 16 Dec 2008, Max Laier wrote: > ... > > >> - Don't lock the hook during registration, since it's not yet in use. > > > > > > Shouldn't the WLOCK be obtained regardless in order to obtain a memory > > > barrier for the reading threads? pfil_run_hooks doesn't interact with > > > the LIST_LOCK so it's unclear in what state the hook head will be when > > > the hook head is first read. > > > > Hmm. Interesting observation. However, that approach is not consistent > > with lots of other code, so possibly that other code needs to change as > > well. For example, ip_init() initializes lots of other global data > > structures, including the fragment reassembly queue and protocol switch, > > without acquiring locks in such a way as to force visibility on other CPUs. > > > > I've CC'd John Baldwin, who might be able to comment on this issue more > > generally. Should we be, for example, calling { IPQ_LOCK(); IPQ_UNLOCK(); > > } after initializing the IP reassembly queues to make sure that > > initialization is visible to other CPUs that will be calling IPQ_LOCK() > > before using it? 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); 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. 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() */ } -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200901151659.35735.jhb>