On Mon, Jun 06, 2016 at 12:50:04PM +0200, Martin Pieuchot wrote: > On 06/06/16(Mon) 16:23, Masao Uebayashi wrote: > > On Mon, Jun 06, 2016 at 08:50:49AM +0200, Martin Pieuchot wrote: > > > On 06/06/16(Mon) 13:04, Masao Uebayashi wrote: > > > > Broadcast frame, coming into a bridge'ed interface, passes if_input() 3 > > > > times, > > > > and actually input (ether_input()) twice. > > > > > > > > - A frame enters an interface (e.g. pair(4)), the interface calls > > > > if_input() > > > > on it. The frame is queued in if_input_queue. > > > > > > > > - A task running if_input_process() is triggered. It takes the frame > > > > and > > > > calls bridge_input(). Frame is queued in bridgeintrq. > > > > > > > > - bridge_process() dispatches frame as multicast/broadcast (if > > > > (ETHER_IS_MULTICAST())) and calls bridge_ifinput() on it, then passes > > > > the > > > > frame to bridgeintr_frame(). > > > > > > > > - bridgeintr_frame() calls bridge_broadcast() on it. > > > > > > > > - bridge_broadcast() calls bridge_localbroadcast(), which again calls > > > > bridge_ifinput(). > > > > > > > > bridge_ifinput() is called twice for each broadcast frames. > > > > bridge_ifinput() > > > > calls if_input(). Thus 3 if_input() for each. > > > > > > > > These duplicate frames confuse pppoe(4), that's why it stops working. > > > > > > What do you mean by "confuse pppoe(4)"? I still don't understand what's > > > the link between pppoe(4) and bpf(4) in this case and why BPF matters > > > for a kernel driver. > > > > - PPPoE client (pppoe(4)) sends a PPPoE Discovery "initiation" frame, which > > is > > broadcast. > > > > - PPPoE server (npppd(8)) receives 3 copies of it via bpf(4), then returns > > 3 PPPoE Discovery "offer" frames. > > > > - pppoe(4) receives 3 "offer" frames and gets confused ... somehow. > > > > I don't know the internal of pppoe(4) yet. pppoe(4) might have a bug, it > > might be able to work even if it receives 3 replies at once, I don't know. > > > > I don't think that npppd(8) receiving 3 copies of broadcast frames (via > > bpf(4)) is an intentional design anyway. > > I agree. Diff below should reduce the number of copies to 2.
Diff works as expected for me, and reads me good. OK uebayasi@. > In order > to remove the last copy somebody has to turn bridge_process() MP-safe > and merge it with bridge_input(). > > Once this is done, bridge_input() can return "0" for multicast packets > and ether_input() will be call directly without the need for requeueing > the packet. I admit I don't fully understand the intention of if_ih_* handler. Can you safely assume that there is always ether_input() hook just after bridge_input()? > Index: net/if_bridge.c > =================================================================== > RCS file: /cvs/src/sys/net/if_bridge.c,v > retrieving revision 1.279 > diff -u -p -r1.279 if_bridge.c > --- net/if_bridge.c 30 May 2016 12:56:16 -0000 1.279 > +++ net/if_bridge.c 6 Jun 2016 10:46:01 -0000 > @@ -1129,7 +1129,7 @@ bridge_process(struct ifnet *ifp, struct > } else > #endif /* NGIF */ > bridge_ifinput(ifp, mc); > - > + > bridgeintr_frame(sc, ifp, m); > return; > } > @@ -1225,14 +1225,15 @@ bridge_broadcast(struct bridge_softc *sc > if (bridge_filterrule(&p->bif_brlout, eh, m) == > BRL_ACTION_BLOCK) > continue; > > - bridge_localbroadcast(sc, dst_if, eh, m); > - > /* > * Don't retransmit out of the same interface where > * the packet was received from. > */ > if (dst_if->if_index == ifp->if_index) > continue; > + > + bridge_localbroadcast(sc, dst_if, eh, m); > + > #if NMPW > 0 > /* > * Split horizon: avoid broadcasting messages from wire to
