On Thu, Sep 20, 2012 at 10:11:20AM +0200, Camiel Dobbelaar wrote:
> I need help testing this bridge diff, as I cannot test (or even imagine) 
> all the possible bridge setups.
> 
> It brings a nice speed improvement and simplifies the code.
> 
> Testing especially appreciated with gif, tun and vether interfaces in the 
> bridge.
> 
> I can provide i386 and amd64 kernels to make it convenient.  :-)
> 
> Thanks!

The diff reads fine. I like the idea, storing a pointer to the bridge
port itself makes much more sense than having everyone and their uncle
loop over the iflist in the bridge softc to find the port.

I'll note that apart from making the output path more efficient this
diff also hides the bridge iflist internals inside the bridge core code.
Are you planning on changing the list to e.g. a tree going forward?

I'm gonna test this on my firewalls. Let's see if it runs as well
as it looks :)

There are two small changes buried in the diff which are unrelated
to the overall change you're making to the bridge code, see below.

> Index: netinet/if_ether.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/if_ether.c,v
> retrieving revision 1.93
> diff -u -p -r1.93 if_ether.c
> --- netinet/if_ether.c        18 Sep 2011 11:17:58 -0000      1.93
> +++ netinet/if_ether.c        18 Sep 2012 09:56:02 -0000

> @@ -671,7 +674,9 @@ in_arpinput(struct mbuf *m)
>                                  ac->ac_if.if_xname);
>                               goto out;
>                       } else if (rt->rt_ifp != &ac->ac_if) {
> +#if NCARP > 0
>                               if (ac->ac_if.if_type != IFT_CARP)
> +#endif
>                                       log(LOG_WARNING,
>                                          "arp: attempt to overwrite entry for"
>                                          " %s on %s by %s on %s\n",

The above NCARP > 0 addition, and ...

> @@ -689,19 +694,25 @@ in_arpinput(struct mbuf *m)
>                               rt->rt_expire = 1; /* no longer static */
>                       }
>                   }
> -             } else if (rt->rt_ifp != &ac->ac_if && !(ac->ac_if.if_bridge &&
> -                 (rt->rt_ifp->if_bridge == ac->ac_if.if_bridge)) &&
> +             } else if (rt->rt_ifp != &ac->ac_if &&
> +#if NBRIDGE > 0
> +                 !(SAME_BRIDGE(ac->ac_if.if_bridgeport,
> +                 rt->rt_ifp->if_bridgeport)) &&
> +#endif

... the one quoted below.
I'd suggest splitting these out into a separate diff.

> +#if NCARP > 0
>                   !(rt->rt_ifp->if_type == IFT_CARP &&
>                   rt->rt_ifp->if_carpdev == &ac->ac_if) &&
>                   !(ac->ac_if.if_type == IFT_CARP &&
> -                 ac->ac_if.if_carpdev == rt->rt_ifp)) {
> -                 log(LOG_WARNING,
> -                     "arp: attempt to add entry for %s "
> -                     "on %s by %s on %s\n",
> -                     inet_ntoa(isaddr), rt->rt_ifp->if_xname,
> -                     ether_sprintf(ea->arp_sha),
> -                     ac->ac_if.if_xname);
> -                 goto out;
> +                 ac->ac_if.if_carpdev == rt->rt_ifp &&
> +#endif

Reply via email to