Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 27 Aug 2004 22:52:57 +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:  <200408272253.14317.max@love2party.net>
In-Reply-To: <20040827202133.GC55748@hub.freebsd.org>
References:  <200408271516.i7RFGO8L061926@repoman.freebsd.org> <412F8F68.92BDEEB0@freebsd.org> <20040827202133.GC55748@hub.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help

--Boundary-02=_685LBltutL4qFGW
Content-Type: text/plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline

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 =3D=3D -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 =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. The check inside pfil_run_hooks() was introduced=20
before PFIL_HOOKS got part of GENERIC to avoid the lock operation for empty=
=20
hooks. It is okay to remove it now that we do the check earlier. A wrapper =
is=20
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=
=20
point in the check for ph_busy_count =3D=3D -1 is to avoid the lock/unlock =
in the=20
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 mi=
ght=20
be removed once we revisit the locking and thus we need to modify the check=
=2E=20
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 v=
ery=20
different. Long story short sx(9) has even more overhead and has starvation=
=20
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=20
have a better sx(9) implementation I am all for switching over. Right now i=
t=20
does not work.

=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=_685LBltutL4qFGW
Content-Type: application/pgp-signature
Content-Description: signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (FreeBSD)

iD8DBQBBL586XyyEoT62BG0RAnIRAJ99e106oqhm2Ofe1yzgFpk2EY3FZQCfVefF
wUaI0IISwCY6YGRZz6+IyrA=
=Gn4c
-----END PGP SIGNATURE-----

--Boundary-02=_685LBltutL4qFGW--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200408272253.14317.max>