On Mon, Oct 12, 2015 at 8:31 PM, John Fastabend
<john.fastab...@gmail.com> wrote:
> On 15-10-12 08:28 PM, Scott Feldman wrote:
>> On Mon, Oct 12, 2015 at 11:03 AM, Jiri Pirko <j...@resnulli.us> wrote:
>>> From: Jiri Pirko <j...@mellanox.com>
>>>
>>> Since spinlock is held here, defer the switchdev operation.
>>>
>>> Signed-off-by: Jiri Pirko <j...@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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
