From owner-cvs-all@FreeBSD.ORG Sat Aug 28 17:52:04 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 BDD2416A4CE; Sat, 28 Aug 2004 17:52:04 +0000 (GMT) Received: from moutng.kundenserver.de (moutng.kundenserver.de [212.227.126.173]) by mx1.FreeBSD.org (Postfix) with ESMTP id E8D4D43D68; Sat, 28 Aug 2004 17:52:03 +0000 (GMT) (envelope-from max@love2party.net) Received: from [212.227.126.208] (helo=mrelayng.kundenserver.de) by moutng.kundenserver.de with esmtp (Exim 3.35 #1) id 1C17Mx-00072H-00; Sat, 28 Aug 2004 19:52:03 +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 1C17Mw-0005Qw-00; Sat, 28 Aug 2004 19:52:03 +0200 From: Max Laier To: Darren Reed Date: Sat, 28 Aug 2004 19:50:26 +0200 User-Agent: KMail/1.6.2 References: <200408271516.i7RFGO8L061926@repoman.freebsd.org> <200408272253.14317.max@love2party.net> <20040828164043.GA86995@hub.freebsd.org> In-Reply-To: <20040828164043.GA86995@hub.freebsd.org> MIME-Version: 1.0 Content-Type: multipart/signed; protocol="application/pgp-signature"; micalg=pgp-sha1; boundary="Boundary-02=_pXMMBuqX74IOXEv"; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <200408281950.33363.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: Sat, 28 Aug 2004 17:52:04 -0000 --Boundary-02=_pXMMBuqX74IOXEv Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline On Saturday 28 August 2004 18:40, Darren Reed wrote: > I hope I didn't send a reply to this already, I think I had to reconnect.= =2E. > > On Fri, Aug 27, 2004 at 10:52:57PM +0200, Max Laier wrote: > > > 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. > > No. *sigh* ... you misread me here. I was saying that the point you are making = and=20 your analysis is right. > Checking ph_busy_count should be inside pfil_run_hooks or outside of it. > > Not in both places. > > > The check inside pfil_run_hooks() was introduced > > before PFIL_HOOKS got part of GENERIC to avoid the lock operation for > > empty hooks. It is okay to remove it now that we do the check earlier. A > > wrapper is called for, nontheless! > > Right, it needs removing and wasn't - just don't say it is right how it i= s. Never did. Though I really do not belive that it incurres a performance los= s=20 as ph_busy_count will be cached together with ph_want_write anyway ... stup= id=20 micro-optimizations ... > > > 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 point in the check for ph_busy_count =3D=3D -1 is to avoid the > > lock/unlock in the case of an empty hook list. > > I wonder if ph_busy_lock could be made to disappear and its use replaced > with a check for TAILQ_EMPTY() ? This would nto require a lock/unlock. While TAILQ_EMPTY is okay it'd require a direction lookup and that would=20 result in more overhead ... and NO, pf_busy_count can not be removed - it i= s=20 a property of the locking (which you didn't read, I suppose). > > > 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. > > Wrong - I haven't read anything else from you so that's not possible. > > > The problem is that ph_busy_count might > > be removed once we revisit the locking and thus we need to modify the > > check. Hence we should wrap the check in anticipation of that. > > Whether it goes inside a macro or not is immaterial. > > > Take a look at how sx(9) is implemented. What is there in pfil now is n= ot > > very different. > > Well why won't you rewrite sx(9) then and fix the perceived problems ? > > > > The use of sx(9) should be a no-brainer. > > > > Yes, but it will reduce performance and introduce stravation problems. > > Once we > > have a better sx(9) implementation I am all for switching over. Right n= ow > > it does not work. > > sx(9) seems to work well enough for me :) > > Meanwhile, should all the functions in pfil.c that are PFIL_[A-Z]* be > renamed to pfil_[a-z]* ? This has got to be a KNF violation ? Well, I submitted it as macros, Sam turned them into __inline functions. I= =20 don't care eitherway, but UPPERCASE is somewhat identical to the other plac= es=20 where lock macros are used: IFNET_{R,W}[UN]LOCK etc ... =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=_pXMMBuqX74IOXEv Content-Type: application/pgp-signature Content-Description: signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.5 (FreeBSD) iD8DBQBBMMXpXyyEoT62BG0RAi0FAJ0UGe4VeiUKAkupzZE/nWynjdpOuACfUFlu fD7ybmcGfqh2VpxPp7Tqx3g= =Sg2G -----END PGP SIGNATURE----- --Boundary-02=_pXMMBuqX74IOXEv--