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

*sigh* ... you misread me here. I was saying that the point you are making and 
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 is.

Never did. Though I really do not belive that it incurres a performance loss 
as ph_busy_count will be cached together with ph_want_write anyway ... stupid 
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 == -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 
result in more overhead ... and NO, pf_busy_count can not be removed - it is 
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 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 ?

Well, I submitted it as macros, Sam turned them into __inline functions. I 
don't care eitherway, but UPPERCASE is somewhat identical to the other places 
where lock macros are used: IFNET_{R,W}[UN]LOCK etc ...

-- 
/"\  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-all/attachments/20040828/b811d7cb/attachment.bin