Re: [PATCH V3 net-next 1/4] net: bridge: add fdb flag to extent locked port feature

On tis, maj 31, 2022 at 17:23, Ido Schimmel [off-list ref] wrote:

On Tue, May 31, 2022 at 11:34:21AM +0200, Hans Schultz wrote:

quoted

quoted

Just to give you another data point about how this works in other
devices, I can say that at least in Spectrum this works a bit
differently. Packets that ingress via a locked port and incur an FDB
miss are trapped to the CPU where they should be injected into the Rx
path so that the bridge will create the 'locked' FDB entry and notify it
to user space. The packets are obviously rated limited as the CPU cannot
handle billions of packets per second, unlike the ASIC. The limit is not
per bridge port (or even per bridge), but instead global to the entire
device.

Btw, will the bridge not create a SWITCHDEV_FDB_ADD_TO_DEVICE event
towards the switchcore in the scheme you mention and thus add an entry
that opens up for the specified mac address?

It will, but the driver needs to ignore FDB entries that are notified
with locked flag. I see that you extended 'struct
switchdev_notifier_fdb_info' with the locked flag, but it's not
initialized in br_switchdev_fdb_populate(). Can you add it in the next
version?

An issue with sending the flag to the driver is that port_fdb_add() is
suddenly getting more and more arguments and getting messy in my
opinion, but maybe that's just how it is...

Another issue is that
bridge fdb add MAC dev DEV master static
seems to add the entry with the SELF flag set, which I don't think is
what we would want it to do or?
Also the replace command is not really supported properly as it is. I
have made a fix for that which looks something like this:

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 6cbb27e3b976..f43aa204f375 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c

@@ -917,6 +917,9 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
                if (flags & NLM_F_EXCL)
                        return -EEXIST;
 
+               if (flags & NLM_F_REPLACE)
+                       modified = true;
+
                if (READ_ONCE(fdb->dst) != source) {
                        WRITE_ONCE(fdb->dst, source);
                        modified = true;

The argument for always sending notifications to the driver in the case
of replace is that a replace command will refresh the entries timeout if
the entry is the same. Any thoughts on this?