From owner-cvs-all@FreeBSD.ORG Fri Aug 27 20:54:46 2004 Return-Path: Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id E716F16A4CE; Fri, 27 Aug 2004 20:54:46 +0000 (GMT) Received: from moutng.kundenserver.de (moutng.kundenserver.de [212.227.126.188]) by mx1.FreeBSD.org (Postfix) with ESMTP id 5104743D1D; Fri, 27 Aug 2004 20:54:46 +0000 (GMT) (envelope-from max@love2party.net) Received: from [212.227.126.179] (helo=mrelayng.kundenserver.de) by moutng.kundenserver.de with esmtp (Exim 3.35 #1) id 1C0nkD-0000BH-00; Fri, 27 Aug 2004 22:54:45 +0200 Received: from [84.128.129.215] (helo=donor.laier.local) by mrelayng.kundenserver.de with asmtp (TLSv1:RC4-MD5:128) (Exim 3.35 #1) id 1C0nkC-0001Yh-00; Fri, 27 Aug 2004 22:54:45 +0200 From: Max Laier To: Darren Reed Date: Fri, 27 Aug 2004 22:52:57 +0200 User-Agent: KMail/1.6.2 References: <200408271516.i7RFGO8L061926@repoman.freebsd.org> <412F8F68.92BDEEB0@freebsd.org> <20040827202133.GC55748@hub.freebsd.org> In-Reply-To: <20040827202133.GC55748@hub.freebsd.org> MIME-Version: 1.0 Content-Type: multipart/signed; protocol="application/pgp-signature"; micalg=pgp-sha1; boundary="Boundary-02=_685LBltutL4qFGW"; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <200408272253.14317.max@love2party.net> X-Provags-ID: kundenserver.de abuse@kundenserver.de auth:61c499deaeeba3ba5be80f48ecc83056 cc: cvs-src@FreeBSD.org cc: src-committers@FreeBSD.org cc: Andre Oppermann cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/share/man/man4 ipfirewall.4 src/share/man/man9 pfil.9 src/sys/alpha/conf GENERIC src/sys/amd64/conf GENERIC src/sys/conf NOTES files options src/sys/i386/conf GENERIC src/sys/ia64/conf GENERIC SKI src/sys/modules/bridge Makefile ... X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Aug 2004 20:54:47 -0000 --Boundary-02=_685LBltutL4qFGW Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline On Friday 27 August 2004 22:21, Darren Reed wrote: > On Fri, Aug 27, 2004 at 09:45:44PM +0200, Andre Oppermann wrote: > > Darren Reed wrote: > > > On Fri, Aug 27, 2004 at 06:12:11PM +0200, Max Laier wrote: > > > > Maybe we should hide: > > > > if (inet_pfil_hook.ph_busy_count =3D=3D -1) > > > > > > There's now a double check on ph_busy_count for inet & inet6 as it's > > > first statement in pfil_run_hooks(). Seems a bit silly... > > > > It's not. There is quite a bit of code that follows the pfil_run_hooks= () > > to look for certain things that might have happend to a packet. If no > > hooks are connected we can save the work and simply jump over it. Take > > a look the code that is jumped over. > > You misunderstand what I'm saying here. I'm not saying get rid of the > outer check or don't do it (or that's not my intention, anyway.) > > If you were to unroll the pfil_run_hooks(), the code would be: > > if (ph_busy_count =3D=3D -1) > goto passin; > if (ph_busy_count =3D=3D -1 || ph_want_write =3D=3D 1) > goto passin; > > Now that's an oversimplification but perhaps it better illustrates > what I see the problem as being. Can you see what's wrong here ? This is actually right. The check inside pfil_run_hooks() was introduced=20 before PFIL_HOOKS got part of GENERIC to avoid the lock operation for empty= =20 hooks. It is okay to remove it now that we do the check earlier. A wrapper = is=20 called for, nontheless! > > > > behind a macro in case we modify the locking for pfil_hooks in the > > > > future. I am thinking of something like: > > > > if (PFIL_IS_EMPTY(&inet_pfil_hook)) > > > > > > Unless pfil(9) is being used with a protocol that has been loaded via > > > an LKM (and can therefore disappear), there should be no risk here to > > > warrant the use of locking. > > Actually, my analysis there is wrong. The only risk here is if > the pfil_head structure can disappear and at present they're all > staticly allocated. > > > Locking is used to protect changes to the hooks. If you hook/unhook > > there might be another CPU traversing the hooks while you yank them > > underneath it. Panic. > > Right, but you've not understood what I'm saying here, again. > > The check to see if the pfil_head is empty doesn't traverse it, > does it? So there's no need to get a lock to do it. Even if you > were to replace the check on ph_busy_count with a "is the list > empty" check, you still don't need to lock the pfil_head until > before you start to walk it. The worst that can happen is that > a packet will either bypass or enter pfil_run_hooks() when it > might have gone in, depending on timing. That is right. Nobody asked to lock the check in the first place. The whole= =20 point in the check for ph_busy_count =3D=3D -1 is to avoid the lock/unlock = in the=20 case of an empty hook list. > You've doubled up on an if() for performance reasons, yet you want > to put in a macro to hide an operation that is even more expensive > than is ther enow - mutex attempt - at some point in the future ? > This doesn't add up ? You don't understood what I am saying. The problem is that ph_busy_count mi= ght=20 be removed once we revisit the locking and thus we need to modify the check= =2E=20 Hence we should wrap the check in anticipation of that. > > > The pfil locking should be overhauled, however, with the main aim > > > to replace PFIL_*LOCK() with the use of sx(9). > > > > Please read the reply from Max. He already explained why sx(9) is > > inappropriate. > > I don't seem to have it in my mailbox... > > I'd like to say he's wrong but without reading his reply, I can't :( Take a look at how sx(9) is implemented. What is there in pfil now is not v= ery=20 different. Long story short sx(9) has even more overhead and has starvation= =20 problems. > The strategy currently in place is more akin to something that would > be used in a device driver where you have two "halves" and one will > sleep. In none of the code wrapped by PFIL_WLOCK is there anything > that will make the system wait. You cannot sleep with PFIL_WLOCK held. It's a plain mutex lock! > The use of sx(9) should be a no-brainer. Yes, but it will reduce performance and introduce stravation problems. Once= we=20 have a better sx(9) implementation I am all for switching over. Right now i= t=20 does not work. =2D-=20 /"\ Best regards, | mlaier@freebsd.org \ / Max Laier | ICQ #67774661 X http://pf4freebsd.love2party.net/ | mlaier@EFnet / \ ASCII Ribbon Campaign | Against HTML Mail and News --Boundary-02=_685LBltutL4qFGW Content-Type: application/pgp-signature Content-Description: signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.5 (FreeBSD) iD8DBQBBL586XyyEoT62BG0RAnIRAJ99e106oqhm2Ofe1yzgFpk2EY3FZQCfVefF wUaI0IISwCY6YGRZz6+IyrA= =Gn4c -----END PGP SIGNATURE----- --Boundary-02=_685LBltutL4qFGW--