Re: [patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device

[Florian Fainelli <f.fainelli@gmail.com>]

On 25/11/14 18:03, Scott Feldman wrote:
> On Tue, Nov 25, 2014 at 12:44 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 25/11/14 02:28, Jiri Pirko wrote:
>>> From: Scott Feldman <sfeldma@gmail.com>
>>>
>>> When the swdev device learns a new mac/vlan on a port, it sends some async
>>> notification to the driver and the driver installs an FDB in the device.
>>> To give a holistic system view, the learned mac/vlan should be reflected
>>> in the bridge's FBD table, so the user, using normal iproute2 cmds, can view
>>> what is currently learned by the device.  This API on the bridge driver gives
>>> a way for the swdev driver to install an FBD entry in the bridge FBD table.
>>> (And remove one).
>>>
>>> This is equivalent to the device running these cmds:
>>>
>>>   bridge fdb [add|del] <mac> dev <dev> vid <vlan id> master
>>>
>>> This patch needs some extra eyeballs for review, in paricular around the
>>> locking and contexts.
>>>
>>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>> ---
>>
>> [snip]
>>
>>> +     head = &br->hash[br_mac_hash(addr, vid)];
>>> +     fdb = fdb_find(head, addr, vid);
>>> +     if (!fdb) {
>>> +             fdb = fdb_create(head, p, addr, vid);
>>> +             if (!fdb) {
>>> +                     err = -ENOMEM;
>>> +                     goto err_unlock;
>>> +             }
>>> +             fdb->added_by_external_learn = 1;
>>> +             fdb_notify(br, fdb, RTM_NEWNEIGH);
>>> +     } else if (fdb->added_by_external_learn) {
>>> +             /* Refresh entry */
>>> +             fdb->updated = fdb->used = jiffies;
>>> +     } else if (!fdb->added_by_user) {
>>> +             /* Take over SW learned entry */
>>> +             fdb->added_by_external_learn = 1;
>>> +             fdb->updated = jiffies;
>>> +             fdb_notify(br, fdb, RTM_NEWNEIGH);
>>> +     }
>>
>> Is there any case where this fdb entry gets re-used and is no longer
>> added by an external learning? Should we clear this flag somewhere?
> 
> Once the FDB entry is marked "added_by_external_learn" it stays marked
> as such until removed by aging cleanup process (or flushed due to
> interface down, etc).  If aged out (and now deleted), the FDB entry
> may come back either by SW learn or by HW learn.  If SW learn comes
> first, and then HW learn, HW learn will override and mark the existing
> FDB entry "added_by_external_learn".  So there is take-over by HW but
> no give-back to SW.  And there is no explicit clearing of the mark
> short of deleting the FDB entry.  The mark is mostly for letting
> user's know which FDB entries where learned by HW and synced to the
> bridge's FDB.

Thanks, makes sense now. This is probably obvious in this context, but
maybe it would not hurt to come up with a documentation that describe
the offload API, FDB entry lifetime and HW/SW ownership etc...

> 
>> [snip]
>>
>>> +EXPORT_SYMBOL(br_fdb_external_learn_del);
>>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>>> index 4f577c4..02cd63b 100644
>>> --- a/net/bridge/br_private.h
>>> +++ b/net/bridge/br_private.h
>>> @@ -101,6 +101,7 @@ struct net_bridge_fdb_entry
>>>       unsigned char                   is_local;
>>>       unsigned char                   is_static;
>>>       unsigned char                   added_by_user;
>>> +     unsigned char                   added_by_external_learn;
>>
>> Pheww, we could be saving some memory footprint here by using different
>> types here ;)
>> --
>> Florian