From owner-svn-src-all@FreeBSD.ORG Wed Dec 17 20:09:58 2008 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 75C821065690 for ; Wed, 17 Dec 2008 20:09:58 +0000 (UTC) (envelope-from max@love2party.net) Received: from moutng.kundenserver.de (moutng.kundenserver.de [212.227.126.188]) by mx1.freebsd.org (Postfix) with ESMTP id F39C78FC13 for ; Wed, 17 Dec 2008 20:09:57 +0000 (UTC) (envelope-from max@love2party.net) Received: from vampire.homelinux.org (dslb-088-067-253-008.pools.arcor-ip.net [88.67.253.8]) by mrelayeu.kundenserver.de (node=mrelayeu5) with ESMTP (Nemesis) id 0ML25U-1LD2iS2ONT-0004FB; Wed, 17 Dec 2008 21:09:56 +0100 Received: (qmail 79016 invoked from network); 17 Dec 2008 20:09:56 -0000 Received: from fbsd8.laiers.local (192.168.4.151) by laiers.local with SMTP; 17 Dec 2008 20:09:56 -0000 From: Max Laier Organization: FreeBSD To: Robert Watson Date: Wed, 17 Dec 2008 21:09:55 +0100 User-Agent: KMail/1.10.1 (FreeBSD/8.0-CURRENT; KDE/4.1.1; i386; ; ) References: <200812161703.mBGH3M7m042955@svn.freebsd.org> <200812161821.37686.max@love2party.net> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200812172109.55724.max@love2party.net> X-Provags-ID: V01U2FsdGVkX1/XLHq1HguIRnEFe1EB1xtZpLdgU8RYycf3WAD D5v/rXVSWCjX7q1q2YsCPgL7NWPtRq+V8Vu8iFKKhs8a2sNWhV GGMtKvtSRkxHqRsdHS4Bg== Cc: src-committers@freebsd.org, Kip Macy , jhb@freebsd.org, svn-src-all@freebsd.org, Attilio Rao , 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: Wed, 17 Dec 2008 20:09:58 -0000 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? > ... > >> - Don't write-lock hooks during removal because they are assumed > >> quiesced. > > > > Again, this lock also ensures that we see all the changes done in other > > threads to the pfil head recently - otherwise we might leak a hook > > function pointer or even fault due to list inconsistencies. Again, > > add/del_hook don't interact with the LIST_LOCK. > > We may run into similar problems with VIMAGE destructors now that we are > interested in tearing down network stacks. Again, perhaps jhb can opine > some. -- /"\ Best regards, | mlaier@freebsd.org \ / Max Laier | ICQ #67774661 X http://pf4freebsd.love2party.net/ | mlaier@EFnet / \ ASCII Ribbon Campaign | Against HTML Mail and News