On 20/12/18(Thu) 11:35, David Gwynne wrote:
> My last attempt in "let etherip(4) output directly to the ip stack"
> didn't go so well. This one at least doesn't break bridge(4) on
> Ethernet.
It breaks it on mpw(4).
> This diff effectively turns if_enqueue into a per interface function
> pointer. Things still call if_enqueue(), which does some pf stuff, and then
> it calls ifp->if_enqueue. By default, that function pointer is
> if_enqueue_ifq() which puts the packet on the interface send queues.
>
> Ethernet interfaces intercept the call to if_enqueue_ifq(). They get set
> up to call ether_enqueue, which does the bridge stuff and then calls
> ac->ac_enqueue, which shadows the ifp->if_enqueue functionality.
Why do we need two abstraction layers? It feels weird to have
pseudo-drivers rely on a pointer in 'struct arpcom', because
they don't necessarily need this structure.
> This takes Ethernet bridge(4) knowledge out of the generic interface
> code. Non-Ethernet things that want to bypass ifqs can override
> ifp->if_enqueue. Ethernet things can override ac->ac_enqueue.
Why take it out of the generic? We spent a lot of energy putting it
there, now you're taking the opposite direction. So let's spread
NBRIDGE > 1 all over the stack again?
> This diff includes a change to vlan(4) to take advantage of this. It
> overrides ac_enqueue with vlan_enqueue(). So a packet sent out a vlan
> interface will go into if_enqueue(), which will call ether_enqueue()
> because ether_ifattach set that up. ether_enqueue() will call
> ac->ac_enqueue which is vlan_enqueue on vlan interfaces, which will
> eventually call if_enqueue() on the parent interface.
I like the idea behind vlan_transmit().
>
> There are some things I'd like to try if this goes in. eg, it may be
> possible to have ether_enqueue simple call ac->ac_enqueue, and let
> bridge(4) swap it out with bridge_enqueue code. Doing that now would
> complicate the diff though.
Why? What's the gain?
> Anyway, how's this look, esp relative to the previous diff?
It looks better but I'm afraid this diff still breaks stuff, I'd
suggest you to keep your fix simple. I'm afraid with all the
abstractions that you're introducing. They look like premature
optimizations to me.
One more comment below.
> Index: net/if.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.569
> diff -u -p -r1.569 if.c
> --- net/if.c 11 Dec 2018 22:08:57 -0000 1.569
> +++ net/if.c 19 Dec 2018 06:48:37 -0000
> @@ -683,34 +685,33 @@ if_qstart_compat(struct ifqueue *ifq)
> int
> if_enqueue(struct ifnet *ifp, struct mbuf *m)
> {
> - unsigned int idx;
> - struct ifqueue *ifq;
> - int error;
> -
> #if NPF > 0
> if (m->m_pkthdr.pf.delay > 0)
> return (pf_delay_pkt(m, ifp->if_index));
> -#endif
> -
> -#if NBRIDGE > 0
> - if (ifp->if_bridgeport && (m->m_flags & M_PROTO1) == 0) {
> - KERNEL_LOCK();
> - error = bridge_output(ifp, m, NULL, NULL);
> - KERNEL_UNLOCK();
> - return (error);
> - }
> -#endif
>
> -#if NPF > 0
> pf_pkt_addr_changed(m);
> #endif /* NPF > 0 */
Are you sure calling pf_pkt_addr_changed() before bridge_output() won't
create any unwanted side effect? In any case I'd suggest keeping this
separated, to be able to revert it easily :D
