Re: [patch net-next v4 5/7] bridge: defer switchdev fdb del call in fdb_del_external_learn

[John Fastabend <john.fastabend@gmail.com>]

On 15-10-12 09:19 PM, Scott Feldman wrote:
> On Mon, Oct 12, 2015 at 8:31 PM, John Fastabend
> <john.fastabend@gmail.com> wrote:
>> On 15-10-12 08:28 PM, Scott Feldman wrote:
>>> On Mon, Oct 12, 2015 at 11:03 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> From: Jiri Pirko <jiri@mellanox.com>
>>>>
>>>> Since spinlock is held here, defer the switchdev operation.
>>>>
>>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>>> ---
>>>>  net/bridge/br_fdb.c | 5 ++++-
>>>>  net/bridge/br_if.c  | 3 +++
>>>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>>>> index f5e7da0..c88bd8e 100644
>>>> --- a/net/bridge/br_fdb.c
>>>> +++ b/net/bridge/br_fdb.c
>>>> @@ -134,7 +134,10 @@ static void fdb_del_hw_addr(struct net_bridge *br, const unsigned char *addr)
>>>>  static void fdb_del_external_learn(struct net_bridge_fdb_entry *f)
>>>>  {
>>>>         struct switchdev_obj_port_fdb fdb = {
>>>> -               .obj.id = SWITCHDEV_OBJ_ID_PORT_FDB,
>>>> +               .obj = {
>>>> +                       .id = SWITCHDEV_OBJ_ID_PORT_FDB,
>>>> +                       .flags = SWITCHDEV_F_DEFER,
>>>> +               },
>>>>                 .vid = f->vlan_id,
>>>>         };
>>>>
>>>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>>>> index 934cae9..09147cb 100644
>>>> --- a/net/bridge/br_if.c
>>>> +++ b/net/bridge/br_if.c
>>>> @@ -24,6 +24,7 @@
>>>>  #include <linux/slab.h>
>>>>  #include <net/sock.h>
>>>>  #include <linux/if_vlan.h>
>>>> +#include <net/switchdev.h>
>>>>
>>>>  #include "br_private.h"
>>>>
>>>> @@ -249,6 +250,8 @@ static void del_nbp(struct net_bridge_port *p)
>>>>         list_del_rcu(&p->list);
>>>>
>>>>         br_fdb_delete_by_port(br, p, 0, 1);
>>>> +       switchdev_flush_deferred();
>>>> +
>>>
>>> This potentially flushes other (valid) work on the deferred queue not
>>> related to FDB del.
>>>
>>> I wonder if this flush step is necessary at all?  The work we deferred
>>> to delete the FDB entry can still happen after the port has been
>>> removed (del_nbp).  If the port driver/device find the FDB entry, then
>>> delete it, otherwise ignore it.
>>>
>>
>> Just the first thing that springs to mind reading this comment is,
>>
>>   - del gets deffered
>>   - add fdb
>>   - del runs
>>
>> Is there an issue here? Sorry I'll do a more thorough review now just
>> thought I would toss it out there before I forget.
> 
> It's a valid thought to consider, for sure.  The context is these are
> only FDB entries added by an external learn event.  So I believe in
> your sequence, the second step to add fdb entry wouldn't happen as the
> fdb entry already exists at that point (in other words, the entry has
> already been learned on external device and pushed up via notifier to
> bridge).  So I think we're OK in regards to your question.
> 

ah I see so the take away is we need to be very careful about who/what
sets the deferred bit or you might get yourself in a world of hurt.

Here you are just ensuring you get all the fdb addr's out of the device.
Seems OK to me just be sure you don't try to set the deferred bit on
the attributes setting the state to DISABLED so we don't get a race
there.

Thanks,
John