Re: [PATCH net-next 00/10] DSA unicast filtering

From: "Alvin Šipraga" <ALSI@bang-olufsen.dk>
To: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Jakub Kicinski <kuba@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Vladimir Oltean <olteanv@gmail.com>,
	Ido Schimmel <idosch@nvidia.com>,
	Tobias Waldekranz <tobias@waldekranz.com>,
	Claudiu Manoil <claudiu.manoil@nxp.com>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	"UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>
Subject: Re: [PATCH net-next 00/10] DSA unicast filtering
Date: Thu, 3 Mar 2022 14:20:29 +0000	[thread overview]
Message-ID: <87h78fdsnn.fsf@bang-olufsen.dk> (raw)
In-Reply-To: <20220303131820.4qzrcrdtx7tlhaei@skbuf> (Vladimir Oltean's message of "Thu, 3 Mar 2022 13:18:21 +0000")

Vladimir Oltean <vladimir.oltean@nxp.com> writes:

> Hi Alvin,
>
> On Thu, Mar 03, 2022 at 12:16:26PM +0000, Alvin ┼áipraga wrote:
>> Hi Vladimir,
>> 
>> Vladimir Oltean <vladimir.oltean@nxp.com> writes:
>> 
>> > This series doesn't attempt anything extremely brave, it just changes
>> > the way in which standalone ports which support FDB isolation work.
>> >
>> > Up until now, DSA has recommended that switch drivers configure
>> > standalone ports in a separate VID/FID with learning disabled, and with
>> > the CPU port as the only destination, reached trivially via flooding.
>> > That works, except that standalone ports will deliver all packets to the
>> > CPU. We can leverage the hardware FDB as a MAC DA filter, and disable
>> > flooding towards the CPU port, to force the dropping of packets with
>> > unknown MAC DA.
>> >
>> > We handle port promiscuity by re-enabling flooding towards the CPU port.
>> > This is relevant because the bridge puts its automatic (learning +
>> > flooding) ports in promiscuous mode, and this makes some things work
>> > automagically, like for example bridging with a foreign interface.
>> > We don't delve yet into the territory of managing CPU flooding more
>> > aggressively while under a bridge.
>> >
>> > The only switch driver that benefits from this work right now is the
>> > NXP LS1028A switch (felix). The others need to implement FDB isolation
>> > first, before DSA is going to install entries to the port's standalone
>> > database. Otherwise, these entries might collide with bridge FDB/MDB
>> > entries.
>> >
>> > This work was done mainly to have all the required features in place
>> > before somebody starts seriously architecting DSA support for multiple
>> > CPU ports. Otherwise it is much more difficult to bolt these features on
>> > top of multiple CPU ports.
>> 
>> So, previously FDB entries were only installed on bridged ports. Now you
>> also want to install FDB entries on standalone ports so that flooding
>> can be disabled on standalone ports for the reasons stated in your cover
>> letter.
>
> Not "on standalone ports", but on the CPU ports, for the standalone
> ports' addresses. Otherwise, yes.
>
>> To implement FDB isolation in a DSA driver, a typical approach might be
>> to use a filter ID (FID) for the FDB entries that is unique per
>> bridge. That is, since FDB entries were only added on bridged ports
>> (through learning or static entries added by software), the DSA driver
>> could readily use the bridge_num of the bridge that is being offloaded
>> to select the FID. The same bridge_num/FID would be used by the hardware
>> for lookup/learning on the given port.
>
> Yes and no. "FID" is a double-edged sword, it will actually depend upon
> hardware implementation. FID in general is a mechanism for FDB
> partitioning. If the VID->FID translation is keyed only by VID, then the
> only intended use case for that is to support shared VLAN learning,
> where all VIDs use the same FID (=> the same database for addresses).
> This isn't very interesting for us.
> If the VID->FID translation is keyed by {port, VID}, it is then possible
> to make VIDs on different ports (part of different bridges) use
> different FIDs, and that is what is interesting.
>
>> If the above general statements are correct-ish, then my question here
>> is: what should be the FID - or other equivalent unique identifier used
>> by the hardware for FDB isolation - when the port is not offloading a
>> bridge, but is standalone? If FDB isolation is implemented in hardware
>> with something like FIDs, then do all standalone ports need to have a
>> unique FID?
>
> Not necessarily, although as far as the DSA core is concerned, we treat
> drivers supporting FDB isolation as though each port has its own database.
> For example, the sja1105 driver really has unique VIDs (=> FIDs) per
> standalone port, so if we didn't treat it that way, it wouldn't work.

I think I'm not quite grokking what it means for a port to have its own
database. Above you corrected me by stating that this patchset does not
install FDB entries "on standalone ports", but on the CPU ports. The way
I parse this is: you are adding an FDB entry to the database of the
CPU port. But how would that help with the "line side to CPU side"
direction? When a packet from the "line side" enters one of the
(standalone) user ports, the switch would appeal to the forwarding
database of that port, not the CPU port, no? So how does it help to
install FDB entries on the CPU port(s)?

Basically for a switch with two ports, swp0 and swp1, with MAC addresses
foo and bar respectively, we want the following FDB entries to be
installed when both ports are in standalone mode:

    MAC | VID | PORT
    ----+-----+-------------
    foo |  0  | cpu_port_num
    bar |  0  | cpu_port_num

Such that, when swp0 receives a packet with MAC DA 'foo', it knows to
forward that packet to cpu_port_num, i.e. towards the CPU port, instead
of flooding/dropping/trapping it. And ditto for swp2 and 'bar'. Right?

Now DSA assumes that each port has its own forwarding database, so let
me annotate how I see that by adding another column "PORT DB":

    MAC | VID | PORT         | PORT DB
    ----+-----+--------------+--------------
    foo |  0  | cpu_port_num | swp0_port_num  [*]
    bar |  0  | cpu_port_num | swp1_port_num  [**]

This codifies better that the entry for 'foo' should, ideally, only be
used by the switch when it receives a packet from 'foo' on swp0, but not
if it's received on swp1, etc. Internally this might be weakened to the
same underlying FID, but as you already pointed out, this is acceptable.

Overall this seems logical and OK to me, but I can't reconcile it with
your statement that the FDB entries in question are installed "on the
CPU port". Wouldn't that look like this?

    MAC | VID | PORT         | PORT DB
    ----+-----+--------------+-------------
    foo |  0  | cpu_port_num | cpu_port_num
    bar |  0  | cpu_port_num | cpu_port_num

... which seems wrong.

I'm suspecting/hoping that my above misunderstanding is just a matter
of terminology. For the sake of emphasis, allow me to describe in plain
English the FDB entries [*] and [**] above:

 [*] add an FDB entry on swp0 where [the host/station? with] MAC address
     'foo' is behind the CPU port on VID 0
[**] add an FDB entry on swp1 where [the host/station? with] MAC address
     'bar' is behind the CPU port on VID 0

Merely an attempt, and perhaps not idiomatic, but now you can tell me
what's wrong in describing it that way :-)

>
> It is important to visualize that there are only 2 directions for a
> packet to travel in a standalone port's FID: either from the line side
> to the CPU, or from the CPU to the line side.
>
> If the "CPU to line side" direction is unimpeded by FDB lookups, then
> only the direction towards the CPU matters.
>
> Further assuming that there is a single CPU port (you didn't ask about
> multiple CPU ports, so I won't tell), the only practical implication of
> standalone ports sharing the same FID is that they will, in effect,
> share the same MAC DA filters. So, if you have 4 slave interfaces, each
> with its own unicast and multicast address lists, what will happen is
> that each port will accept the union of all others too. It's not
> perfect, in the sense that unnecessary packets would still reach the
> CPU, but they would still be dropped there.

Right, this is the scenario I had in mind. Fine by me if this is
considered acceptable. I see how it is still better than flooding every
packet.

> For the felix/ocelot driver, I was ok with that => that driver uses the
> same VID/FID of 0 for all standalone ports.
>
>> For some context: I have been working on implementing offload features
>> for the rtl8365mb driver and I can also support FDB isolation between
>> bridged ports. The number of offloaded bridges is bounded by the number
>> of FIDs available, which is 8. For standalone ports I use a reserved
>> FID=0 which currently would never match any entries in the FDB, because
>> learning is disabled on standalone ports and Linux does not install any
>> FDB entries. When placed in a bridge, the FID of that port is then set
>> to bridge_num, which - rather conveniently - is indexed by 1.
>> 
>> Your change seems to introduce a more generic concept of per-port
>> FDB. How should one model the per-port FDB in hardware which uses FIDs?
>> Should I ensure that all ports - standalone by default - start with a
>> unique FID? That will be OK for switches with up to 8 ports, but for
>> switches with more ports, I'm a but puzzled as to what I can do. Do I
>> then have to declare that FDB isolation is unsupported
>> (fdb_isolation=0)?
>> 
>> Hope the question makes sense.
>
> As long as
> (a) tag_rtl8_4.c sends packets to standalone ports in a way in which FDB
>     lookups are bypassed

Yes, it does.

> (a) you're not concerned about the hardware MAC DA filters being a bit
>     more relaxed than software expects

Since they are dropped in software, it doesn't matter to me.

> then I wouldn't worry too much about it, and keep using FID 0 for all
> standalone ports.

Great, thanks a lot for clarifying.

Kind regards,
Alvin