Date: Sat, 28 Aug 2004 16:40:43 +0000 From: Darren Reed <darrenr@hub.freebsd.org> To: Max Laier <max@love2party.net> 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 ... Message-ID: <20040828164043.GA86995@hub.freebsd.org> In-Reply-To: <200408272253.14317.max@love2party.net> References: <200408271516.i7RFGO8L061926@repoman.freebsd.org> <412F8F68.92BDEEB0@freebsd.org> <20040827202133.GC55748@hub.freebsd.org> <200408272253.14317.max@love2party.net>
next in thread | previous in thread | raw e-mail | index | archive | help
I hope I didn't send a reply to this already, I think I had to reconnect... 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 == -1) > > goto passin; > > if (ph_busy_count == -1 || ph_want_write == 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. 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 is. > > 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 == -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. > > 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 not > 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 now 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 ? Darren
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040828164043.GA86995>