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

[Darren Reed]

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