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=20 > replay the lower half and add more locking folks. >=20 > The change in question is here:=20 > http://svn.freebsd.org/viewvc/base/head/sys/net/pfil.c?r1=3D173904&r2=3D1= 86187=20 >=20 > 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 u= se. > > > > > > 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=20 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 =3D=3D key) { LOCK(p); RUNLOCK(foo_list); return (p); } } RUNLOCK(foo_list); return (NULL); } =09 something_else() { foo *p; RLOCK(foo_list); LIST_FOREACH(p, foo_list) { LOCK(p); bar(p); UNLOCK(p); } RUNLOCK(foo_list); } =46rom your description it would appear that you are doing something_else()= but=20 without actually locking foo_list. Unless you can demonstrate a clear win = in=20 benchmarks from not having the lock there, I would suggest just going with= =20 the simpler and more common approach. Depending on the mtx of the individu= al=20 head doesn't do anything to solve races with removing the head from the lis= t. So, reading a bit further, I think the real fix is that the pfil API is a b= it=20 busted. What you really want to do is have pfil_get_head() operate like=20 foo_find() and lock the head before dropping the list lock. That is the on= ly=20 race I see in the current code. However, because you malloc() after callin= g=20 pfil_get_head() that is problematic. That is, the current consumer code=20 looks like this: ph =3D 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 =3D pfil_get_head(x, y); /* locks the 'ph' like foo_find() */ } =2D-=20 John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200901151659.35735.jhb>