On 23/06/2021 16:34, Alexandra Winter wrote:
> Whenever a unicast fdb entry is added or deleted in the software
> bridge's fdb, synchronize it to the hardware fdb of a bridgeport
> device, if the bridgeport has the attribute LEARNING_SYNC and is not
> isolated from the target of the changed fdb entry.
>
> To inform HW, that messages with a specific unicast target address
> should be sent to the software bridge via this bridgeport, simply
> register this address with the device.
>
> Without this patch smart NICs attached to a bridgeport of a software
> bridge can already do their own learning on the messages that the
> SW bridge sends out via this port. And otherwise accept/flood all
> unknown target messages to the SW bridge (promiscuous port).
> This patch gives the attached HW the chance to update its fdb, even
> when it does not see the respective message, because it is forwarded
> to another piece of HW attached to another bridgeport. Or when the NIC
> is not capable of learning or flooding.
>
> An alternative solution would be to subscribe to the
> SWITCHDEV_FDB_ADD/DEL_TO_DEVICE switchdev notifiers in the respective
> device drivers. But as there's no HW-specific part in this
> implementation, it was felt that this should rather be implemented in
> the common layer of the bridge code.
>
> Signed-off-by: Alexandra Winter <wintera@xxxxxxxxxxxxx>
> ---
> net/bridge/br_fdb.c | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
Absolutely no way, I'm sorry but br_fdb_update() is called when learning on every packet,
walking the port list on every fdb port change is a disastrous overkill.
You already have specified the alternative yourself, please use the switchdev notifiers
that are there and already take up the necessary processing in the fast paths.
Nacked-by: Nikolay Aleksandrov <nikolay@xxxxxxxxxx>
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 698b79747d32..2075b5da6db3 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -567,6 +567,32 @@ int br_fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
> return ret;
> }
>
> +static void br_fdb_learning_sync(struct net_bridge *br,
> + const struct net_bridge_fdb_entry *fdb,
> + int type)
> +{
> + struct net_bridge_port *p;
> +
> + if (!fdb->dst)
> + return;
> + list_for_each_entry(p, &br->port_list, list) {
You can't just walk the port list without any synchronization.
> + if ((p->flags & BR_LEARNING_SYNC) && p != fdb->dst &&
> + (!(p->flags & BR_ISOLATED) ||
These flags can change while running...
> + !(fdb->dst->flags & BR_ISOLATED))) {
> + switch (type) {
> + case RTM_DELNEIGH:
> + dev_uc_del(p->dev, fdb->key.addr.addr);
> + break;
> + case RTM_NEWNEIGH:
> + dev_uc_add(p->dev, fdb->key.addr.addr);
> + break;
... and you can end up having programmed a lot of dynamic fdbs that will never get removed.
> + default:
> + break;
> + }
> + }
> + }
> +}
> +
> /* returns true if the fdb was modified */
> static bool __fdb_mark_active(struct net_bridge_fdb_entry *fdb)
> {
> @@ -603,6 +629,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
> if (unlikely(source != fdb->dst &&
> !test_bit(BR_FDB_STICKY, &fdb->flags))) {
> br_switchdev_fdb_notify(fdb, RTM_DELNEIGH);
> + br_fdb_learning_sync(br, fdb, RTM_DELNEIGH);
> fdb->dst = source;
> fdb_modified = true;
> /* Take over HW learned entry */
> @@ -799,6 +826,7 @@ static void fdb_notify(struct net_bridge *br,
> goto errout;
> }
> rtnl_notify(skb, net, 0, RTNLGRP_NEIGH, NULL, GFP_ATOMIC);
> + br_fdb_learning_sync(br, fdb, type);
> return;
> errout:
> rtnl_set_sk_err(net, RTNLGRP_NEIGH, err);
>
