On Mon, Mar 27, 2023 at 05:31:26PM +0200, Hans Schultz wrote:
On Mon, Mar 27, 2023 at 14:52, Vladimir Oltean olteanv@gmail.com wrote:
By the way, there is a behavior change here.
Before:
$ ip link add br0 type bridge && ip link set br0 up $ ip link set swp0 master br0 && ip link set swp0 up $ bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic [ 70.010181] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 0 [ 70.019105] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 1 .... 5 minutes later [ 371.686935] mscc_felix 0000:00:00.5: felix_fdb_del: port 0 addr 00:01:02:03:04:05 vid 1 [ 371.695449] mscc_felix 0000:00:00.5: felix_fdb_del: port 0 addr 00:01:02:03:04:05 vid 0 $ bridge fdb | grep 00:01:02:03:04:05
After:
$ ip link add br0 type bridge && ip link set br0 up $ ip link set swp0 master br0 && ip link set swp0 up $ bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic [ 222.071492] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 0 flags 0x1 [ 222.081154] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 1 flags 0x1 .... 5 minutes later $ bridge fdb | grep 00:01:02:03:04:05 00:01:02:03:04:05 dev swp0 vlan 1 offload master br0 stale 00:01:02:03:04:05 dev swp0 offload master br0 stale 00:01:02:03:04:05 dev swp0 vlan 1 self 00:01:02:03:04:05 dev swp0 self
As you can see, the behavior is not identical, and it made more sense before.
I see this is Felix Ocelot and there is no changes in this patchset that affects Felix Ocelot. Thus I am quite sure the results will be the same without this patchset, ergo it must be because of another patch. All that is done here in the DSA layer is to pass on an extra field and add an extra check that will always pass in the case of this flag.
If mv88e6xxx is all you have, you can still sanity-check the equivalent effect of your patch set to other drivers by simply not acting upon the "flags" argument in mv88e6xxx_port_fdb_add()/mv88e6xxx_port_fdb_del(), and disabling the logic to treat Age Out interrupts. Then you should be able to notice exactly the behavior change I am talking about.
In your own commit message, it says:
Author: Hans J. Schultz netdev@kapio-technology.com
net: bridge: ensure FDB offloaded flag is handled as needed
Since user added entries in the bridge FDB will get the BR_FDB_OFFLOADED ~~~~~~~~~~~~~~~~~~~~ flag set, we do not want the bridge to age those entries and we want the ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ entries to be deleted in the bridge upon an SWITCHDEV_FDB_DEL_TO_BRIDGE ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ existing drivers do not emit this event.
Signed-off-by: Hans J. Schultz netdev@kapio-technology.com
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c index e69a872bfc1d..b0c23a72bc76 100644 --- a/net/bridge/br_fdb.c +++ b/net/bridge/br_fdb.c @@ -537,6 +537,7 @@ void br_fdb_cleanup(struct work_struct *work) unsigned long this_timer = f->updated + delay;
if (test_bit(BR_FDB_STATIC, &f->flags) || + test_bit(BR_FDB_OFFLOADED, &f->flags) || test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags)) { if (test_bit(BR_FDB_NOTIFY, &f->flags)) { if (time_after(this_timer, now)) @@ -1465,7 +1466,9 @@ int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p, spin_lock_bh(&br->hash_lock);
fdb = br_fdb_find(br, addr, vid); - if (fdb && test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)) + if (fdb && + (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags) || + test_bit(BR_FDB_OFFLOADED, &fdb->flags))) fdb_delete(br, fdb, swdev_notify); else err = -ENOENT;
A reasonable question you could ask yourself is: why do my BR_FDB_OFFLOADED entries have this flag in the software bridge in the first place? Did I add code for it? Is it because there is some difference between mv88e6xxx and ocelot/felix, or is it because dsa_fdb_offload_notify() gets called in both cases from generic code just the same?
And if dsa_fdb_offload_notify() gets called in both cases just the same, but no other driver except for mv88e6xxx emits the SWITCHDEV_FDB_DEL_TO_BRIDGE which you've patched the bridge to expect in this series, then what exactly is surprising in the fact that offloaded and dynamic FDB entries now become stale, but are not removed from the software bridge as they were before?