From owner-svn-src-all@FreeBSD.ORG Thu Jan 15 22:47:22 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 492D61065747; Thu, 15 Jan 2009 22:47:22 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 05EEC8FC38; Thu, 15 Jan 2009 22:47:22 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from server.baldwin.cx (pool-98-109-39-197.nwrknj.fios.verizon.net [98.109.39.197]) by cyrus.watson.org (Postfix) with ESMTPSA id E3A5446B2D; Thu, 15 Jan 2009 17:47:18 -0500 (EST) Received: from localhost (john@localhost [127.0.0.1]) (authenticated bits=0) by server.baldwin.cx (8.14.3/8.14.3) with ESMTP id n0FMkttI004922; Thu, 15 Jan 2009 17:47:13 -0500 (EST) (envelope-from jhb@freebsd.org) From: John Baldwin To: Max Laier Date: Thu, 15 Jan 2009 16:59:35 -0500 User-Agent: KMail/1.9.7 References: <200812161703.mBGH3M7m042955@svn.freebsd.org> <200812172109.55724.max@love2party.net> In-Reply-To: <200812172109.55724.max@love2party.net> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline Message-Id: <200901151659.35735.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [127.0.0.1]); Thu, 15 Jan 2009 17:47:13 -0500 (EST) X-Virus-Scanned: ClamAV 0.94.2/8870/Thu Jan 15 15:57:00 2009 on server.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-4.4 required=4.2 tests=ALL_TRUSTED,BAYES_00 autolearn=ham version=3.1.3 X-Spam-Checker-Version: SpamAssassin 3.1.3 (2006-06-01) on server.baldwin.cx 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 22:47:23 -0000 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