Re: kern/41114: soft interrupt queue patch for BRIDGE_IPF to prevent lock issues

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Wed, Apr 01, 2009 at 05:00:01AM +0000, necedemalis%gmail.com@localhost wrote:
> >Description:
> Using BRIDGE_IPF causes nearly immediate lock errors.  I asked on current 
> users and apparently others had had this problem and it was resulting from 
> pfil_run_hooks on inet_pfil_hooks being unsafe for interrupts.
> 
> So I wrote this which fixes my lock issues though it might have its own 
> issues.
> Changes:
>     bridge_enqueue and bridge_forward are put on a netisr soft interrupt 
> queue using IF_QUEUE and stuffing the packet, and bridge information into an 
> mbuf.  I didn't like the idea of allocating another mbuf, but didn't see any 
> good alternative for using what little I know of the existing kernel 
> infrastructure to do this.

I don't think it's necessery to have a soft interrupt queue for
bridge_enqueue(). It should already be called only from soft interrupt
of thread context.
bridge_input()/bridge_forward() can use the ifp->if_snd queue, there's
no need to store more bridge information here (it's already a per-interface
queue, which is currently unused for bridge).
See the patch I sent to tech-net which does this.

> If bridge filtering is off, the soft interrupts are not used so there's no 
> performance penalty.

That may not be a good idea. The problem is not limited to BRIDGE_IPF;
as pointed out by tls@ calling the underlying interface's if_start function
from hard interrupt context may also be troublesome.

> 
>    stopping and starting the bridge clears if_bridge pointers in interfaces 
> that have been added to the bridge.  Previously the bridge code was called 
> even if the bridge was down and then the bridge would either return or route 
> it.  This seemed strange.  If the bridge is down, the packet should be 
> handled as normal by that interface.

This is a change in behavior that may have unwanted side effects.
This should be discussed in a separate PR I think.
Note that the current bridge code already checks IFF_RUNNING, so it effectively
behaves as a non-bridged interface when the bridge is down, even if the
bridge code is called.
> 
>    bridge_output previously passed runfilt=0 to bridge_enqueue meaning that 
> outgoing local packets were not filtered.  This seemed strange so I put it to 
> 1.  If bridge filtering is on, then you would want it on for local packets as 
> well I would assume.

ether_output() has already been called in ether_output() for this packet.
I'm not sure this change is a good idea because the same packet would
then be seen 2 times on 2 different interfaces. Again this is a 
behavior change that should be discussed separately.

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--