Message-ID: <20210719092625.tnbgghblfqiwtrwl@skbuf>
Date:   Mon, 19 Jul 2021 09:26:26 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Vladimir Oltean <vladimir.oltean@....com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Ido Schimmel <idosch@...sch.org>,
        Tobias Waldekranz <tobias@...dekranz.com>,
        Roopa Prabhu <roopa@...dia.com>,
        Nikolay Aleksandrov <nikolay@...dia.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        "bridge@...ts.linux-foundation.org" 
        <bridge@...ts.linux-foundation.org>,
        Grygorii Strashko <grygorii.strashko@...com>,
        Marek Behun <kabel@...ckhole.sk>,
        DENG Qingfang <dqfext@...il.com>,
        Vadym Kochan <vkochan@...vell.com>,
        Taras Chornyi <tchornyi@...vell.com>,
        Lars Povlsen <lars.povlsen@...rochip.com>,
        Steen Hegelund <Steen.Hegelund@...rochip.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>
Subject: Re: [PATCH v4 net-next 10/15] net: bridge: switchdev object replay
 helpers for everybody

On Mon, Jul 19, 2021 at 12:44:29AM +0300, Vladimir Oltean wrote:
> Starting with commit 4f2673b3a2b6 ("net: bridge: add helper to replay
> port and host-joined mdb entries"), DSA has introduced some bridge
> helpers that replay switchdev events (FDB/MDB/VLAN additions and
> deletions) that can be lost by the switchdev drivers in a variety of
> circumstances:
> 
> - an IP multicast group was host-joined on the bridge itself before any
>   switchdev port joined the bridge, leading to the host MDB entries
>   missing in the hardware database.
> - during the bridge creation process, the MAC address of the bridge was
>   added to the FDB as an entry pointing towards the bridge device
>   itself, but with no switchdev ports being part of the bridge yet, this
>   local FDB entry would remain unknown to the switchdev hardware
>   database.
> - a VLAN/FDB/MDB was added to a bridge port that is a LAG interface,
>   before any switchdev port joined that LAG, leading to the hardware
>   database missing those entries.
> - a switchdev port left a LAG that is a bridge port, while the LAG
>   remained part of the bridge, and all FDB/MDB/VLAN entries remained
>   installed in the hardware database of the switchdev port.
> 
> Also, since commit 0d2cfbd41c4a ("net: bridge: ignore switchdev events
> for LAG ports which didn't request replay"), DSA introduced a method,
> based on a const void *ctx, to ensure that two switchdev ports under the
> same LAG that is a bridge port do not see the same MDB/VLAN entry being
> replayed twice by the bridge, once for every bridge port that joins the
> LAG.
> 
> With so many ordering corner cases being possible, it seems unreasonable
> to expect a switchdev driver writer to get it right from the first try.
> Therefore, now that we are past the beta testing period for the bridge
> replay helpers used in DSA only, it is time to roll them out to all
> switchdev drivers.
> 
> To convert the switchdev object replay helpers from "pull mode" (where
> the driver asks for them) to a "push mode" (where the bridge offers them
> automatically), the biggest problem is that the bridge needs to be aware
> when a switchdev port joins and leaves, even when the switchdev is only
> indirectly a bridge port (for example when the bridge port is a LAG
> upper of the switchdev).
> 
> Luckily, we already have a hook for that, in the form of the newly
> introduced switchdev_bridge_port_offload() and
> switchdev_bridge_port_unoffload() calls. These offer a natural place for
> hooking the object addition and deletion replays.
> 
> Extend the above 2 functions with:
> - pointers to the switchdev atomic notifier (for FDB replays) and the
>   blocking notifier (for MDB and VLAN replays).
> - the "const void *ctx" argument required for drivers to be able to
>   disambiguate between which port is targeted, when multiple ports are
>   lowers of the same LAG that is a bridge port. Most of the drivers pass
>   NULL to this argument, except the ones that support LAG offload and have
>   the proper context check already in place in the switchdev blocking
>   notifier handler.
> 
> am65_cpsw and cpsw had the same name for the switchdev notifiers, so I
> renamed the am65_cpsw ones with an am65_ prefix.
> 
> Also unexport the replay helpers, since nobody except the bridge calls
> them directly now.
> 
> Note that:
> (a) we abuse the terminology slightly, because FDB entries are not
>     "switchdev objects", but we count them as objects nonetheless.
>     With no direct way to prove it, I think they are not modeled as
>     switchdev objects because those can only be installed by the bridge
>     to the hardware (as opposed to FDB entries which can be propagated
>     in the other direction too). This is merely an abuse of terms, FDB
>     entries are replayed too, despite not being objects.
> (b) the bridge does not attempt to sync port attributes to newly joined
>     ports, just the countable stuff (the objects). The reason for this
>     is simple: no universal and symmetric way to sync and unsync them is
>     known. For example, VLAN filtering: what to do on unsync, disable or
>     leave it enabled? Similarly, STP state, ageing timer, etc etc. What
>     a switchdev port does when it becomes standalone again is not really
>     up to the bridge's competence, and the driver should deal with it.
>     On the other hand, replaying deletions of switchdev objects can be
>     seen a matter of cleanup and therefore be treated by the bridge,
>     hence this patch.
> (c) I do not expect a lot of functional change introduced for drivers in
>     this patch, because:
>     - nbp_vlan_init() is called _after_ netdev_master_upper_dev_link(),
>       so br_vlan_replay() should not do anything for the new drivers on
>       which we call it. The existing drivers where there was even a
>       slight possibility for there to exist a VLAN on a bridge port
>       before they join it are already guarded against this: mlxsw and
>       prestera deny joining LAG interfaces that are members of a bridge.
>     - br_fdb_replay() should now notify of local FDB entries, but I
>       patched all drivers except DSA to ignore these new entries in
>       commit 2c4eca3ef716 ("net: bridge: switchdev: include local flag
>       in FDB notifications"). Driver authors can lift this restriction
>       as they wish.
>     - br_mdb_replay() should now fix the issue described in commit
>       2c4eca3ef716 ("net: bridge: switchdev: include local flag in FDB
>       notifications") for all drivers, I don't see any downside.
> 
> Cc: Vadym Kochan <vkochan@...vell.com>
> Cc: Taras Chornyi <tchornyi@...vell.com>
> Cc: Ioana Ciornei <ioana.ciornei@....com>
> Cc: Lars Povlsen <lars.povlsen@...rochip.com>
> Cc: Steen Hegelund <Steen.Hegelund@...rochip.com>
> Cc: UNGLinuxDriver@...rochip.com
> Cc: Claudiu Manoil <claudiu.manoil@....com>
> Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>
> Cc: Grygorii Strashko <grygorii.strashko@...com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>

Acked-by: Ioana Ciornei <ioana.ciornei@....com> # dpaa2-switch

Powered by blists - more mailing lists