On Thu, Feb 27, 2014 at 08:08:05AM -0500, Vlad Yasevich wrote: > On 02/27/2014 02:53 AM, Michael S. Tsirkin wrote: > > On Wed, Feb 26, 2014 at 12:35:08PM -0500, Vlad Yasevich wrote: > >> On 02/26/2014 11:57 AM, Stephen Hemminger wrote: > >>> On Wed, 26 Feb 2014 10:18:21 -0500 > >>> Vlad Yasevich <vyasevic@xxxxxxxxxx> wrote: > >>> > >>>> When a static fdb entry is created, add the mac address to the bridge > >>>> address list. This list is used to program the proper port's > >>>> address list. > >>>> > >>>> Signed-off-by: Vlad Yasevich <vyasevic@xxxxxxxxxx> > >>> > >>> I don't like this level of bookkeeping it starts to mix > >>> layers between the bridge network interface as entity for talking to the > >>> local host, and forwarding table entries. > >> > >> Actually this is one of the reasons this isn't done through the > >> br->dev->uc. Forwarding table entries are still per-port. > >> > >>> > >>> Many times static entries are used as alternative to flooding in > >>> environments which don't trust STP. > >> > >> Ok, and how would this be problematic? If one wants to turn off > >> promisc in this environment, then receive filters needs to be properly > >> programmed. > >> > >>> > >>> Plus, it looks like another major source of bugs. > >>> > >> > >> Any new code is a potential source of issues. Are you saying > >> No to any new code in bridge? > >> > >> -vlad > > > > I'm guessing Stephen merely worries about > > multiple data structures that need to stay in > > sync, and asks that you revisit > > using private hw address list in the bridge. > > > > What's the issue with walking fdb exactly? > > You say > > 1) I tried using the fdb table itself as main repository, but > > this caused difficulties in synchronizing this table with > > the interface filters later on. > > > > I'm guessing you refer to writing addresses out to ports > > directly when walking the hash being impossible > > since this datastructure uses rcu and spinlocks? > > Fair enough but the entries you care about > > seem to only be modified under RTNL so just > > copy them out to a temporary list. > > This might be less efficient, but will be simpler I think. > > > > There are 2 ways to populate the the ports uc list. > 1) We can use dev_uc_add() directly. The issue here is > how to know if a given entry has been written to port. > I've played with this is and we end completely replicating > the netdev_hw_addr functionality in fdb to support the Patch7 > (0 flooding ports). I think I begin to understand. But I still think dev_uc_add is the way to go here. Simply build the list twice, before and after the change. On a change, first dev_uc_add all addresses in the new list, then dev_uc_del all addresses in the old list. > 2) We can use dev_uc_sync() which is what this series does. > This api needs to keep track of sync counts so that things get > properly deleted. For that a temporary list will not work > since you'd be re-creating it every time. > > Now, I think I've come up with a way to remove the private address list > and use bridge->dev->uc, but that requires that we implement fdb-based > filtering for local addresses. > > The idea is to implement learning on the bridge device xmit path. This > will support things like vlans on top of the bridge that change their > mac, or even stack bridge configs that exist in the wild. > My guess, however, is that Stephen would have an even bigger issue with > this. ;) > > -vlad It sounds like a nifty feature without respect to whether it's the right thing to do for the non promisc feature. -- MST
