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 ...

[Max Laier]

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 == -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 ?

This is actually right. 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!

> > > > 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 
point in the check for ph_busy_count == -1 is to avoid the lock/unlock in the 
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 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.

> > > 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 very 
different. Long story short sx(9) has even more overhead and has starvation 
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 
have a better sx(9) implementation I am all for switching over. Right now it 
does not work.

-- 
/"\  Best regards,                      | mlaier at freebsd.org
\ /  Max Laier                          | ICQ #67774661
 X   http://pf4freebsd.love2party.net/  | mlaier at EFnet
/ \  ASCII Ribbon Campaign              | Against HTML Mail and News
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: signature
Url : http://lists.freebsd.org/pipermail/cvs-src/attachments/20040827/25725955/attachment.bin