Date: Sat, 28 Aug 2004 19:50:26 +0200 From: Max Laier <max@love2party.net> To: Darren Reed <darrenr@hub.freebsd.org> 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: <200408281950.33363.max@love2party.net> In-Reply-To: <20040828164043.GA86995@hub.freebsd.org> References: <200408271516.i7RFGO8L061926@repoman.freebsd.org> <200408272253.14317.max@love2party.net> <20040828164043.GA86995@hub.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--Boundary-02=_pXMMBuqX74IOXEv
Content-Type: text/plain;
charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline
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.=
=2E.
>
> 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 =3D=3D -1)
> > > goto passin;
> > > if (ph_busy_count =3D=3D -1 || ph_want_write =3D=3D 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=20
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 i=
s.
Never did. Though I really do not belive that it incurres a performance los=
s=20
as ph_busy_count will be cached together with ph_want_write anyway ... stup=
id=20
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 =3D=3D -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=20
result in more overhead ... and NO, pf_busy_count can not be removed - it i=
s=20
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 n=
ot
> > 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 n=
ow
> > 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=
=20
don't care eitherway, but UPPERCASE is somewhat identical to the other plac=
es=20
where lock macros are used: IFNET_{R,W}[UN]LOCK etc ...
=2D-=20
/"\ Best regards, | mlaier@freebsd.org
\ / Max Laier | ICQ #67774661
X http://pf4freebsd.love2party.net/ | mlaier@EFnet
/ \ ASCII Ribbon Campaign | Against HTML Mail and News
--Boundary-02=_pXMMBuqX74IOXEv
Content-Type: application/pgp-signature
Content-Description: signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (FreeBSD)
iD8DBQBBMMXpXyyEoT62BG0RAi0FAJ0UGe4VeiUKAkupzZE/nWynjdpOuACfUFlu
fD7ybmcGfqh2VpxPp7Tqx3g=
=Sg2G
-----END PGP SIGNATURE-----
--Boundary-02=_pXMMBuqX74IOXEv--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200408281950.33363.max>