Wed, Nov 26, 2014 at 02:48:04AM CET, sfeldma@gmail.com wrote:

On Tue, Nov 25, 2014 at 12:36 PM, Thomas Graf [off-list ref] wrote:

quoted

On 11/25/14 at 11:38am, Andy Gospodarek wrote:

quoted

On Tue, Nov 25, 2014 at 11:28:40AM +0100, Jiri Pirko wrote:

quoted

From: Scott Feldman <redacted>

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 <redacted>
Signed-off-by: Jiri Pirko <jiri@resnulli.us>

I like the simplicity of this. That said, given we'll have multiple
users of swdev including OVS, shouldn't this be a notifier or a
callback that depends on who is controlling the device?

I like the idea.  When the switch port joins Linux bridge or OVS
datapath, a callback is registered with the driver.  That way the
driver doesn't really care if the port is a bridge member or an OVS
vport in a datapath.  It's just passing up the FDB entry
(port/mac/vlan) details to the container device.  Can we hold this
idea until this patchset sticks?  I think once OVS support comes back
into the swdev model would be the time to address this.

Yep, I agree this is a good idea and I also vote for implemeting this as
a follow-up. Thanks.

quoted

quoted

quoted

+   spin_lock(&br->hash_lock);

(Since you asked to check locking...)

Most of the other fdb_add/delete/insert/update calls take this with
spin_lock_bh.  Did you try this with lockdep enabled just to see if that
is needed here?  I suspect that anytime br->hash_lock is taken it will
need to be with softirqs disabled from this point forward.

At least br_fdb_update() seems to be called from BH context so I would
agree and argue the lock in br_fdb_cleanup() and br_fdb_update() need a
fix too. I'll send a patch.