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

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

The following reply was made to PR kern/41114; it has been noted by GNATS.

From: Manuel Bouyer <bouyer%antioche.eu.org@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: kern-bug-people%NetBSD.org@localhost, gnats-admin%NetBSD.org@localhost, 
netbsd-bugs%NetBSD.org@localhost
Subject: Re: kern/41114: soft interrupt queue patch for BRIDGE_IPF to
        prevent lock issues
Date: Thu, 2 Apr 2009 12:09:26 +0200

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