Date: Fri, 27 Aug 2004 20:21:33 +0000 From: Darren Reed <darrenr@hub.freebsd.org> To: Andre Oppermann <andre@freebsd.org> Cc: cvs-src@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: <20040827202133.GC55748@hub.freebsd.org> In-Reply-To: <412F8F68.92BDEEB0@freebsd.org> References: <200408271516.i7RFGO8L061926@repoman.freebsd.org> <200408271812.18748.max@love2party.net> <20040827192307.GA55748@hub.freebsd.org> <412F8F68.92BDEEB0@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 ?
> > > 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.
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 ?
> > 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 :(
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.
The use of sx(9) should be a no-brainer.
Darren
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040827202133.GC55748>