From owner-cvs-src@FreeBSD.ORG Fri Aug 27 20:21:33 2004 Return-Path: Delivered-To: cvs-src@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 680) id 1FBCA16A4CF; Fri, 27 Aug 2004 20:21:33 +0000 (GMT) Date: Fri, 27 Aug 2004 20:21:33 +0000 From: Darren Reed To: Andre Oppermann Message-ID: <20040827202133.GC55748@hub.freebsd.org> References: <200408271516.i7RFGO8L061926@repoman.freebsd.org> <200408271812.18748.max@love2party.net> <20040827192307.GA55748@hub.freebsd.org> <412F8F68.92BDEEB0@freebsd.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <412F8F68.92BDEEB0@freebsd.org> User-Agent: Mutt/1.4.1i cc: Max Laier cc: src-committers@FreeBSD.org cc: cvs-all@FreeBSD.org cc: cvs-src@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-src@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Aug 2004 20:21:33 -0000 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 == -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 == -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 ? > > > 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. 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 ? > > 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 :( 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. The use of sx(9) should be a no-brainer. Darren