Re: [Bridge] [PATCH 3/7] bridge: Add addresses from static fdbs to bridge address list

[Vlad Yasevich <vyasevic@redhat.com>]

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@redhat.com> 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@redhat.com>
>>>
>>> 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).
  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