Date: Wed, 17 Dec 2008 21:09:55 +0100 From: Max Laier <max@love2party.net> To: Robert Watson <rwatson@freebsd.org> Cc: src-committers@freebsd.org, Kip Macy <kmacy@freebsd.org>, jhb@freebsd.org, svn-src-all@freebsd.org, Attilio Rao <attilio@freebsd.org>, svn-src-head@freebsd.org Subject: Re: svn commit: r186187 - head/sys/net Message-ID: <200812172109.55724.max@love2party.net> In-Reply-To: <alpine.BSF.1.10.0812161751250.75599@fledge.watson.org> References: <200812161703.mBGH3M7m042955@svn.freebsd.org> <200812161821.37686.max@love2party.net> <alpine.BSF.1.10.0812161751250.75599@fledge.watson.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200812172109.55724.max>